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

Deprecate semconv module #5786

Merged
merged 5 commits into from
Sep 8, 2023
Merged

Conversation

jack-berg
Copy link
Member

@jack-berg jack-berg commented Sep 1, 2023

Resolves #5463, #3764, #5455.

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 jack-berg requested a review from a team September 1, 2023 17:23

otelBom.addExtra("io.opentelemetry.semconv", "opentelemetry-semconv", "1.21.0-alpha")
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@brunobat brunobat Sep 8, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

Congrats!

implementation(project(":semconv"))
implementation("io.opentelemetry.semconv:opentelemetry-semconv")
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

bom-alpha/build.gradle.kts Outdated Show resolved Hide resolved
bom-alpha/build.gradle.kts Outdated Show resolved Hide resolved
@@ -36,6 +33,12 @@
*/
final class OtelToZipkinSpanTransformer {

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -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"));
Copy link
Contributor

@brunobat brunobat Sep 8, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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.

@jack-berg jack-berg merged commit a438127 into open-telemetry:main Sep 8, 2023
thll added a commit to Graylog2/graylog2-server that referenced this pull request Sep 27, 2023
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.
thll added a commit to Graylog2/graylog2-server that referenced this pull request Sep 27, 2023
* 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>
maxiadlovskii pushed a commit to Graylog2/graylog2-server that referenced this pull request Oct 4, 2023
* 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>
Mahoney added a commit to Mahoney-forks/openfeature-java-sdk-contrib that referenced this pull request Oct 24, 2024
Mahoney added a commit to Mahoney-forks/openfeature-java-sdk-contrib that referenced this pull request Oct 24, 2024
[`io.opentelemetry:opentelemetry-semconv`](https://search.maven.org/artifact/io.opentelemetry/opentelemetry-semconv)
was moved to [`io.opentelemetry.semconv:opentelemetry-semconv`](https://search.maven.org/artifact/io.opentelemetry.semconv/opentelemetry-semconv)
and the former [will be deprecated](open-telemetry/opentelemetry-java#5786).

Signed-off-by: Robert Elliot <rob@lidalia.org.uk>
Mahoney added a commit to Mahoney-forks/openfeature-java-sdk-contrib that referenced this pull request Oct 24, 2024
[`io.opentelemetry:opentelemetry-semconv`](https://search.maven.org/artifact/io.opentelemetry/opentelemetry-semconv)
was moved to [`io.opentelemetry.semconv:opentelemetry-semconv`](https://search.maven.org/artifact/io.opentelemetry.semconv/opentelemetry-semconv)
and the former [will be deprecated](open-telemetry/opentelemetry-java#5786).

Signed-off-by: Robert Elliot <rob@lidalia.org.uk>
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.

Split out semantic convention into dedicated repo
5 participants