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

Upgrade Semconv to 1.26 #11792

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

crossoverJie
Copy link
Contributor

v1.26.0 has been released.

Replace the deprecated attributes with new ones.

@crossoverJie crossoverJie requested a review from a team July 10, 2024 13:46
@github-actions github-actions bot requested a review from theletterf July 10, 2024 13:51
@crossoverJie
Copy link
Contributor Author

@trask @laurit Please take a look.

Copy link
Contributor

@SylvainJuge SylvainJuge left a comment

Choose a reason for hiding this comment

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

LGTM, just a few minor (non blocking) typos.

@laurit
Copy link
Contributor

laurit commented Jul 12, 2024

Unfortunately, they way I see it, this PR can not be merged as it is. Have a look at https://opentelemetry.io/docs/specs/semconv/database/ Pay attention to

Warning Existing database instrumentations that are using v1.24.0 of this document (or prior) SHOULD NOT change the version of the database conventions that they emit until a transition plan to the (future) stable semantic conventions has been published. Conventions include, but are not limited to, attributes, metric and span names, and unit of measure.

Messaging semantic conventions https://opentelemetry.io/docs/specs/semconv/messaging/messaging-spans/ have a similar warning.
I believe that at least for the database semantic conventions the plan is to use a similar opt-in mechanism that we used for stabilizing the http semantic conventions. @trask is this correct? So just changing the code to use new semantic conventions won't work. You'll need to use the otel.semconv-stability.opt-in flag so users can choose whether they want their telemetry with old, new or both semantic conventions. Tests will also need to be modified so that both old and new semantic conventions are tested. If you wish to work on this first have a look at how this was handled in http semantic conventions.

@SylvainJuge
Copy link
Contributor

For otel.semconv-stability.opt-in we will have to re-introduce it as it was removed in #11257 since it was no longer used after the JVM metrics and HTTP conventions were made stable.

From what I remember we plan to make the stable database conventions the default for next major release of instrumentation (so 3.x). Do we have similar plans for other attributes that are covered by this PR (rpc and messsaging) ?

crossoverJie and others added 5 commits July 13, 2024 16:17
…nstrumentation/api/incubator/semconv/db/DbClientAttributesExtractor.java

Co-authored-by: SylvainJuge <[email protected]>
…nstrumentation/api/incubator/semconv/db/DbClientAttributesExtractor.java

Co-authored-by: SylvainJuge <[email protected]>
…nstrumentation/api/incubator/semconv/db/SqlClientAttributesExtractor.java

Co-authored-by: SylvainJuge <[email protected]>
…nstrumentation/api/incubator/semconv/db/SqlClientAttributesExtractor.java

Co-authored-by: SylvainJuge <[email protected]>
@crossoverJie
Copy link
Contributor Author

@laurit @SylvainJuge Thank you for your responses.

I noticed that SemconvStability has been removed.

Should we add SemconvStability back and include these values:

  • db
  • db/dup
  • messaging
  • messaging/dup

@heyams
Copy link
Contributor

heyams commented Aug 27, 2024

@laurit @SylvainJuge Thank you for your responses.

I noticed that SemconvStability has been removed.

Should we add SemconvStability back and include these values:

  • db
  • db/dup
  • messaging
  • messaging/dup

Yes, i am adding it back while prototyping stable database semconv (#11575). however, it's for db, db/dup only. message and messaging/dup can be added later when applicable.

@crossoverJie
Copy link
Contributor Author

@laurit @SylvainJuge Thank you for your responses.
I noticed that SemconvStability has been removed.
Should we add SemconvStability back and include these values:

  • db
  • db/dup
  • messaging
  • messaging/dup

Yes, i am adding it back while prototyping stable database semconv (#11575). however, it's for db, db/dup only. message and messaging/dup can be added later when applicable.

Thank you. I will continue to push this forward when your PR is completed, although by then, we might be able to upgrade to version 1.27 🤣 .

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.

4 participants