Deprecate semconv module #5786

jack-berg · 2023-09-01T17:23:42Z

Semantic convention generated classes have moved to open-telemetry/semantic-conventions-java and published its version first: io.opentelemetry.semconv:opentelemetry-semconv:1.21.0-alpha.

jack-berg · 2023-09-01T17:24:22Z

bom-alpha/build.gradle.kts

+
+  otelBom.addExtra("io.opentelemetry.semconv", "opentelemetry-semconv", "1.21.0-alpha")


Should we include the new io.opentelemetry.semconv:opentelemetry-semconv artifact in the bom? I think yes.

I think yes. Including it means a smaller impact (none?) to users, right?

Not none because the artifact coordinates change from io.opentelemetry:opentelemetry-semconv to io.opentelemetry.semconv:opentelemetry-semconv.

There is some complexity with this resulting from the core component dependencies on semconv. For example, opentelemetry-sdk-common depends on semconv for resource attributes which are included by default. Suppose someone is using an old version of the sdk which relies on io.opentelemetry.semconv:opentelemetry-semconv:1.21.0-alpha, but separately wants to bring in a future semconv version 1.30.0-alpha to produce new telemetry with new semantic conventions. And suppose between versions 1.21.0-alpha and 1.30.0-alpha we make breaking changes to the semconv code generation format which break opentelemetry-sdk-common. The user is essentially forced to either update to a later version of the SDK, or downgrade to a old version of semconv.

My take away from this is:

We should be careful about breaking changes to code generation logic in io.opentelemetry.semconv:opentelemetry-semconv, even while we still publish alpha artifacts and are allowed to make breaking changes

We should try to produce a stable io.opentelemetry.semconv:opentelemetry-semconv sooner than later, to avoid the temptation / footgun of making breaking changes

I agree with opentelemetry-semconv being included in the bom.

We spoke in the 9/7 Java SIG about this and concluded that we want to completely decouple this repository from semantic-conventions-java. This means inlining all current references to semantic conventions, and not including io.opentelemetry.semconv:opentelemetry-semconv in the bom.

There's too much opportunity for version conflict given the instability of io.opentelemetry.semconv:opentelemetry-semconv and the likelihood of breaking changes, either stemming from changes to code gen logic or from changes to semantic-conventions.

We are planning to include the semconv module in the instrumentation bom though.

We are planning to include the semconv module in the instrumentation bom though.

Playing devil's advocate here... Wouldn't that open the door for inconsistencies and introduce further instabilities in the alpha bom?
I say that because I import both... always.

The instrumentation bom is different because the instrumentation apis are actually coupled to the conventions of a particular version. Its not reasonable to use a version of the instrumentation API built for a particular version of semantic conventions with a different version.

codecov · 2023-09-01T17:43:29Z

Codecov Report

Patch coverage is 96.66% of modified lines.

Files Changed	Coverage
...emconv/resource/attributes/ResourceAttributes.java	`ø`
...porter/jaeger/thrift/JaegerThriftSpanExporter.java	`66.66%`
...pentelemetry/exporter/jaeger/ProcessMarshaler.java	`66.66%`
...y/exporter/zipkin/OtelToZipkinSpanTransformer.java	`100.00%`
...opentelemetry/opentracingshim/SpanBuilderShim.java	`100.00%`
...ava/io/opentelemetry/opentracingshim/SpanShim.java	`100.00%`
...metry/sdk/autoconfigure/ResourceConfiguration.java	`100.00%`
...ension/incubator/fileconfig/AttributesFactory.java	`100.00%`
.../java/io/opentelemetry/sdk/resources/Resource.java	`100.00%`
...ntelemetry/sdk/testing/assertj/SpanDataAssert.java	`100.00%`
... and 1 more

📢 Thoughts on this report? Let us know!.

breedx-splk

Congrats!

breedx-splk · 2023-09-01T21:45:10Z

sdk/common/build.gradle.kts

-  implementation(project(":semconv"))
+  implementation("io.opentelemetry.semconv:opentelemetry-semconv")


bom-alpha/build.gradle.kts

…y-java into deprecate-semconv

bom-alpha/build.gradle.kts

jkwatson · 2023-09-07T20:06:20Z

...rters/zipkin/src/main/java/io/opentelemetry/exporter/zipkin/OtelToZipkinSpanTransformer.java

@@ -36,6 +33,12 @@
 */
 final class OtelToZipkinSpanTransformer {

+  private static final AttributeKey<String> SERVICE_NAME = AttributeKey.stringKey("service.name");


do we have any common exporter module this could be put into? Probably not a big deal, but seeing the same key repeated over and over for every exporter is a little weird.

Sort of.. there's :exporters:common, which works for :exporters:zipkin and :exporter:jaeger, but not for :exporters:jaeger-thrift. And that would only cover the duplicate references in the exporters. The other references to service.name would have to use something like :sdk:common as the common dependency, which wouldn't be terrible but not my favorite.

We can always revisit this later since its just an implementation detail.

jkwatson

Thanks!

brunobat · 2023-09-08T08:58:22Z

...s/zipkin/src/test/java/io/opentelemetry/exporter/zipkin/OtelToZipkinSpanTransformerTest.java

@@ -138,7 +133,7 @@ void generateSpan_ProducerKind() {
  @Test
  void generateSpan_ResourceServiceNameMapping() {
    Resource resource =
-        Resource.create(Attributes.of(ResourceAttributes.SERVICE_NAME, "super-zipkin-service"));
+        Resource.create(Attributes.of(stringKey("service.name"), "super-zipkin-service"));


I really fail to see how this can be an improvement.
Overall, not providing an artefact with semantic conventions, even if not complete or perfectly aligned with the spec is a devolution.
Are we really expecting someone to change the convention for service.name? There has to be a subset that is considered stable.

There is an artifact with semantic conventions: io.opentelemetry.semconv:opentelemetry-semconv:1.21.0-alpha

There are a few semantic conventions which are stable, but the tooling for semantic-conventions-java (previously in this repo but deprecated as of this PR) isn't currently able to differentiate between stable / unstable. As a consequence, the whole artifact is marked as alpha, and has the ability to make breaking changes to the code generation logic.

If the code in this repository adds a dependency on io.opentelemetyr.semconv:opentelemetry-semconv and imports a constant, then later an (allowed) breaking change occurs which moves the location of that constant, users will have to deal with a version conflict:

app depends on opentelemetry-exporter-zipkin:1.30.0 which depends on opentelemetry-semconv:1.21.0-alpha

app depends on opentelemetry-semconv:1.25.0, which made a breaking change to the code generation logic

user must choose to upgrade to a later version of opentelemetry-exporter-zipkin which shares the opentelemetry-semconv:1.25.0 version, or downgrade opentelemetry-semconv to 1.21.0-alpha.

This wouldn't be a problem is io.opentelemetry.semconv:opentelemetry-semconv is stable, but there are open problems to work through before thats possible (see #22, #6, #5).

When there is a stable artifact, we can consume that in this repository without worrying that we'll create potential dependency conflicts in the future. But for now, its perfectly safe for these repos to inline the attributes they need. They're just strings after all.

It's just that I had recently replaced all the string in Quarkus by these conventions and now they will vanish...
I guess I need to get the conventions anyway from somewhere.
Using strings for this can lead to insidious bugs.

jack-berg · 2023-09-08T15:43:38Z

Going to go ahead and merge to proceed with the release. This is low impact at the moment since all the changes are internal and do not impact users. We can go back and change these implementation details later if needed. The exception is that io.opentelemetry:opentelemetry-semconv is deprecated, which is unlikely to change.

The artifact has been moved (see open-telemetry/opentelemetry-java#5786). As we are not using it at the moment, remove the dependency rather than adjusting it to use the new group id.

* Bump opentelemetry.version from 1.24.0 to 1.30.0 Bumps `opentelemetry.version` from 1.24.0 to 1.30.0. Updates `io.opentelemetry:opentelemetry-bom` from 1.24.0 to 1.30.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-java/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-java/blob/main/CHANGELOG.md) - [Commits](open-telemetry/opentelemetry-java@v1.24.0...v1.30.0) Updates `io.opentelemetry.instrumentation:opentelemetry-instrumentation-annotations` from 1.24.0 to 1.30.0 - [Release notes](https://github.com/open-telemetry/opentelemetry-java-instrumentation/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/CHANGELOG.md) - [Commits](open-telemetry/opentelemetry-java-instrumentation@v1.24.0...v1.30.0) --- updated-dependencies: - dependency-name: io.opentelemetry:opentelemetry-bom dependency-type: direct:production update-type: version-update:semver-minor - dependency-name: io.opentelemetry.instrumentation:opentelemetry-instrumentation-annotations dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * Remove unused opentelemetry-semconv dependency The artifact has been moved (see open-telemetry/opentelemetry-java#5786). As we are not using it at the moment, remove the dependency rather than adjusting it to use the new group id. --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Othello Maurer <othello@graylog.com>

Deprecate semconv module

8ef8e6b

jack-berg requested a review from a team as a code owner September 1, 2023 17:23

jack-berg commented Sep 1, 2023

View reviewed changes

breedx-splk approved these changes Sep 1, 2023

View reviewed changes

mateuszrzeszutek approved these changes Sep 4, 2023

View reviewed changes

bom-alpha/build.gradle.kts Outdated Show resolved Hide resolved

Get bom semconv version from dependencyManagement

7b33d16

mateuszrzeszutek approved these changes Sep 6, 2023

View reviewed changes

jonatan-ivanov mentioned this pull request Sep 7, 2023

Match OTel semconv version to the SDK version micrometer-metrics/tracing#340

Closed

jack-berg added 2 commits September 7, 2023 14:49

Inline all semantic conventino references

8fc11d8

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

e81ba77

…y-java into deprecate-semconv

jkwatson reviewed Sep 7, 2023

View reviewed changes

bom-alpha/build.gradle.kts Outdated Show resolved Hide resolved

Fix typo

405c0ff

jkwatson reviewed Sep 7, 2023

View reviewed changes

jkwatson approved these changes Sep 7, 2023

View reviewed changes

mateuszrzeszutek approved these changes Sep 8, 2023

View reviewed changes

brunobat suggested changes Sep 8, 2023

View reviewed changes

jack-berg merged commit a438127 into open-telemetry:main Sep 8, 2023
18 checks passed

This was referenced Sep 8, 2023

Release note not including the spec version #5455

Closed

Make semconv packages explicitly versioned #3764

Closed

shakuzen mentioned this pull request Sep 8, 2023

Move to io.opentelemetry.semconv:opentelemetry-semconv micrometer-metrics/tracing#343

Closed

deki mentioned this pull request Sep 19, 2023

Move to io.opentelemetry.semconv:opentelemetry-semconv open-telemetry/opentelemetry-java-contrib#1050

Merged

This was referenced Sep 26, 2023

Fix import for SemantcAttributes class from opentelemetry aws-samples/java-on-aws#78

Closed

Move to io.opentelemetry.semconv:opentelemetry-semconv open-telemetry/opentelemetry-java-contrib#1055

Closed

jonatan-ivanov mentioned this pull request Nov 25, 2023

v1.2.0 seems to break compatibility with Spring Boot 3.1.5 micrometer-metrics/tracing#456

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate semconv module #5786

Deprecate semconv module #5786

jack-berg commented Sep 1, 2023 •

edited

jack-berg Sep 1, 2023

breedx-splk Sep 1, 2023

jack-berg Sep 1, 2023

brunobat Sep 7, 2023

jack-berg Sep 7, 2023

mateuszrzeszutek Sep 8, 2023

brunobat Sep 8, 2023 •

edited

jack-berg Sep 8, 2023

codecov bot commented Sep 1, 2023 •

edited

breedx-splk left a comment

breedx-splk Sep 1, 2023

jkwatson Sep 7, 2023

jack-berg Sep 7, 2023

jkwatson left a comment

brunobat Sep 8, 2023 •

edited

jack-berg Sep 8, 2023

brunobat Sep 11, 2023

jack-berg commented Sep 8, 2023


		otelBom.addExtra("io.opentelemetry.semconv", "opentelemetry-semconv", "1.21.0-alpha")

		implementation(project(":semconv"))
		implementation("io.opentelemetry.semconv:opentelemetry-semconv")

Deprecate semconv module #5786

Deprecate semconv module #5786

Conversation

jack-berg commented Sep 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunobat Sep 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 1, 2023 • edited

Codecov Report

breedx-splk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwatson left a comment

Choose a reason for hiding this comment

brunobat Sep 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-berg commented Sep 8, 2023

jack-berg commented Sep 1, 2023 •

edited

brunobat Sep 8, 2023 •

edited

codecov bot commented Sep 1, 2023 •

edited

brunobat Sep 8, 2023 •

edited