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.

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

Inline incubating attributes + central semconv-incubating dependency #1298

Merged
merged 16 commits into from
May 28, 2024

Conversation

SylvainJuge
Copy link
Contributor

@SylvainJuge SylvainJuge commented May 2, 2024

Initial intent:

  • use semconv-incubating for code.stacktrace in span-stacktrace module
  • centralize version dependency of semconv-incubating across modules.

The non-incubating semconv dependency is transitively provided by the opentelemetry-instrumentation-bom-alpha BOM, which unfortunately does not include the semconv-incubating artifact.

As a possible follow-up improvement, we could include it in the instrumentation BOM as it should align with the version of semconv by default, also it would help the maintainers of distributions by allowing to directly pull both the semconv and semconv-incubating dependencies from the BOM instead of having to keep two versions in sync.


Updated description:

  • inline all the incubating semconv attributes in production code
  • keep a single centralized semconv version
  • only depend on incubating semconv in tests, this implicitly validates inlined attributes fit the incubating semantic conventions.

@@ -15,7 +15,7 @@ dependencies {
implementation("com.google.cloud.opentelemetry:detector-resources-support:0.28.0")

implementation("io.opentelemetry.semconv:opentelemetry-semconv")
implementation("io.opentelemetry.semconv:opentelemetry-semconv-incubating:1.25.0-alpha")
implementation("io.opentelemetry.semconv:opentelemetry-semconv-incubating")
Copy link
Member

Choose a reason for hiding this comment

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

All these usages of opentelemetry-semconv-incubating go against our stated advice:

Library instrumentation SHOULD NOT depend on this.

Libraries can use this for testing, but should make copies of the attributes to avoid possible runtime errors from version conflicts.

I think we should follow our own advice to avoid unresolvable transitive version conflicts.

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 for the reminder, I completely missed it.

I'm definitely missing the context on the intent of this advice, so maybe there is something to clarify here.

For now, the way I understand things:

The opentelemetry-semconv artifact only contains stable semantic conventions.
The opentelemetry-semconv-incubating artifact only contains incubating semantic conventions.

Consumers of semantic conventions who need/want to avoid breaking change:

  • should have a direct dependency on opentelemetry-semconv.
  • might have a test dependency on opentelemetry-semconv-incubating.
  • should inline the incubating semconv attributes in the production code, this is where they trade maintainability for stability.

Consumers of semantic conventions who can tolerate/handle breaking changes:

  • should have a direct dependency on opentelemetry-semconv
  • should have a direct dependency on opentelemetry-semconv-incubating which provides an easy way to find usages and refactor in the IDE.

For example:

  • in the instrumentation agent, we already use the incubating artifact and can embrace breaking changes quite easily due to the large set of contributors who can help. However, the goal is to have as much stable as possible and sometime the incubating metrics (like the JVM metrics) are opt-in through configuration, however this option is not always possible at attribute level.
  • for a module in contrib that has only two contributors and is rarely updated, keeping it stable is probably a better option, so having some duplication is a good compromise.

To me, by adding a dependency on opentelemetry-semconv-incubating on a module in contrib, you opt-in for some breaking changes.

Maybe here having a single version used for all the contrib modules is a bold move and having each module handle its own version (even if they are currently aligned) is a better compromise, so each module can be updated at its own pace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a bit of research I found that it comes from open-telemetry/semantic-conventions-java#41, and open-telemetry/semantic-conventions-java#22 (comment) provides some background on the issue with transitive dependencies.

However, this is something that we need to solve in a separate issue/PR, and maybe implementing a "per version namespace for the non-stable attributes" as suggested here in https://github.com/open-telemetry/semantic-conventions-java/ could help.

From what I understand the proposal is to have the generated semconv classes with a dedicated package per version (for example io.opentelemetry.semconv.v1_25_0) and to only keep a few older versions in the incubating artifact to prevent breaking existing usages.

@SylvainJuge
Copy link
Contributor Author

@jack-berg I now understand that having direct dependencies on the incubating attributes artifact can be problematic and why we'd like to avoid direct references to them in the published artifacts.

However, for the developer/maintainer in this repository there are definitely some benefits to have it, and I don't think that manually duplicating the incubating attributes is something we can enforce as it makes later maintenance more tedious: whenever new attributes are promoted/modified in a new semconv version, you have to grep the code to modify and update it, whereas with direct dependencies we can get automatic upgrades or compile time errors that are easier to manage.

Now that we have a separate artifact for incubating dependencies, we could maybe shade it in every sub-project of this repo where it's used, so it would make any artifact depending on it effectively immune to be used with a different incubating attributes artifact in the classpath.

@SylvainJuge
Copy link
Contributor Author

After failing to find an "easier" solution with the annotation processor (#1322) I think that duplicating the incubating attributes and testing with the semconv-incubating artifact is the best option for now.

I have now updated this PR to implement this strategy for all the modules in contrib.

@SylvainJuge SylvainJuge changed the title Use incubabing semconv for code.stacktrace + central semconv-incubating dependency Inline incubating attributes + central semconv-incubating dependency May 24, 2024
@laurit laurit added this to the v1.36.0 milestone May 28, 2024
@github-actions github-actions bot requested a review from JonasKunz May 28, 2024 07:40
@github-actions github-actions bot requested a review from JonasKunz May 28, 2024 08:52
@trask trask merged commit aa5b65a into open-telemetry:main May 28, 2024
14 checks passed
@SylvainJuge SylvainJuge deleted the span-stacktrace-semconv branch September 6, 2024 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.