Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Generate semantic conventions using sbt #336

Merged
merged 6 commits into from
Oct 20, 2023
Merged

Generate semantic conventions using sbt #336

merged 6 commits into from
Oct 20, 2023

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented Oct 11, 2023 •
edited
Loading

Motivation

The otel4s-semconv module has been out of sync for a while. I see two options:

  1. Subscribe to the releases of https://github.com/open-telemetry/semantic-conventions/ and update files manually
  2. Automate the process

Here is my attempt to semi-automate the process.

The workflow

  • Semantic Conventions releases a new version
  • Semantic Conventions Java releases a new version based on the new opentelemetry-semconv
  • Scala Steward bumps a version and runs the sbt semanticConventionsGenerate command
  • Our semconv module is in sync

A question

Will -alpha suffix in the version prevent Scala Steward from picking new versions?~

Do we need to allow pre-release updates in .scala-steward.conf?:

updates.allowPreReleases  = [ { groupId = "io.opentelemetry.semconv", artifactId = "opentelemetry-semconv" } ]

@iRevive iRevive added this to the 0.3 milestone Oct 11, 2023
@armanbilge
Copy link
Member

Will -alpha suffix in the version prevent Scala Steward from picking new versions?

No, I don't think so. If we are already on an alpha version, Steward should suggest newer alphas.

Do we need to allow pre-release updates in .scala-steward.conf?:

Allowing pre-release updates means that it will propose updates to pre-releases even if you are currently on a stable version.

@armanbilge
Copy link
Member

To test this, can just setup the infra in this PR, but not do the actual update? As soon as it merges Steward should make a PR, and we can see if its working.

armanbilge
armanbilge reviewed Oct 11, 2023
View reviewed changes
project/SemanticConventionsGenerator.scala
Comment on lines +28 to +31
val genAttributes = List(
"docker",
"run",
"--rm",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Is Scala Steward able to run docker images? 🤔 I think it may be running in a docker itself.

Copy link
Contributor Author

@iRevive iRevive Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I forgot Scala Steward runs in docker. Then it won't work. 🙁

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I might be wrong about this. Maybe the public steward is running in docker, but I think the Typelevel steward is just running in ordinary GHA.

https://github.com/scala-steward-org/scala-steward-action/blob/a80ff5bf69947d9be97d9eda20d5e1eb9faba209/src/action/main.ts#L54

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 let's give it a try.

Currently, there is only one release in the https://github.com/open-telemetry/semantic-conventions-java.

A new release of the Semantic Conventions should be published soon open-telemetry/semantic-conventions#380. Java release will follow eventually.

Then, Scala Steward will try to do its work.

@iRevive iRevive removed this from the 0.3 milestone Oct 12, 2023
@iRevive
Copy link
Contributor Author

iRevive commented Oct 12, 2023

Alright, I removed Scala Steward's hook.

Scala Steward will let us know if there is a new version and then we can run the generator manually.

@armanbilge
Copy link
Member

Scala Steward will let us know if there is a new version and then we can run the generator manually.

Can we add some sort of check to CI so that the PR fails if the version is not matching the generated code?

@iRevive
Copy link
Contributor Author

iRevive commented Oct 12, 2023

Can we add some sort of check to CI so that the PR fails if the version is not matching the generated code?

Good point, I added tests for this particular case.

NthPortal
NthPortal reviewed Oct 16, 2023
View reviewed changes
Copy link
Contributor

@NthPortal NthPortal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks correct(ish) to me, though I confess not to be super well-versed in docker

@iRevive
Copy link
Contributor Author

iRevive commented Oct 17, 2023 •
edited
Loading

According to this discussion open-telemetry/semantic-conventions-java#27 (comment), we occasionally need to manually add deprecated attributes to the template.

On the bright side, MiMa will let us know if we forgot to add an attribute.

Unless we can automate the update of the template, for example, by parsing rename_attribute from the schema, there is no reason to generate attributes automatically.

So, the workflow will be the following:

  • Semantic Conventions releases a new version
  • Semantic Conventions Java releases a new version based on the new opentelemetry-semconv
  • Scala Steward bumps a version and runs the sbt semanticConventionsGenerate command
  • If some attributes in the semantic convention have been deprecated and the SemanticAttributes.scala.j2 template is out of sync, the mima check will fail
  • [optional] We update the template and generate the attributes manually

@iRevive
Copy link
Contributor Author

iRevive commented Oct 17, 2023

I also explicitly set tlMimaPreviousVersions := Set("0.3.0-RC2") and ran mima. It turned out I forgot to add deprecated attributes to the template.

I've updated the template and the attributes are in sync now.

iRevive
iRevive commented Oct 17, 2023
View reviewed changes
buildscripts/semantic-convention/templates/SemanticAttributes.scala.j2 Outdated
*/
@deprecated("Use SemanticAttributes.HttpResponseContentLength instead", "")
@deprecated("Use SemanticAttributes.HttpResponseContentLength instead", "Semantic Conventions 1.13.0")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth mentioning in which version of the semantic conventions the attribute has been deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, since refers to the version of the library. In contrast, I mention the version of a third-party semantic convention.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was previously discussed in:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I reverted this change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we put an otel4s version in there rather than an empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see reasons why we shouldn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semconv module wasn't published yet. Can we set since = 0.3.0 everywhere?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that sounds right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the deprecation annotation.

iRevive
iRevive commented Oct 17, 2023
View reviewed changes
build.sbt Outdated Show resolved Hide resolved
@iRevive iRevive added this to the 0.3 milestone Oct 18, 2023
armanbilge
armanbilge approved these changes Oct 20, 2023
View reviewed changes
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, this was a really good idea. I think it looks good! we will see when the next update rolls in 😅

@iRevive iRevive merged commit b38d583 into typelevel:main Oct 20, 2023
10 checks passed
@iRevive iRevive deleted the semantic-convention-gen branch October 20, 2023 13:50
@NthPortal
Copy link
Contributor

it seems like scala steward did not notify us, and we're behind

@iRevive iRevive mentioned this pull request Dec 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NthPortal NthPortal NthPortal left review comments

@armanbilge armanbilge armanbilge approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.3
Development

Successfully merging this pull request may close these issues.
3 participants
@iRevive @armanbilge @NthPortal