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

Add add_link API #1515

Merged
merged 15 commits into from
Mar 19, 2024
Merged

Add add_link API #1515

merged 15 commits into from
Mar 19, 2024

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Feb 7, 2024

Changes

The API is already supported as experimental in specification:
https://github.com/open-telemetry/opentelemetry-specification/blob/v1.29.0/specification/trace/api.md#add-link

And planned to make stable soon - open-telemetry/opentelemetry-specification#3865. So not added under experimental flag.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@lalitb lalitb requested a review from a team February 7, 2024 21:23
Copy link

codecov bot commented Feb 7, 2024

Codecov Report

Attention: Patch coverage is 61.53846% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 68.8%. Comparing base (13c9dc5) to head (5cbc2c3).

Files Patch % Lines
opentelemetry-sdk/src/trace/span.rs 80.4% 9 Missing ⚠️
opentelemetry/src/trace/mod.rs 33.3% 8 Missing ⚠️
opentelemetry/src/global/trace.rs 0.0% 6 Missing ⚠️
opentelemetry/src/trace/noop.rs 0.0% 4 Missing ⚠️
opentelemetry-sdk/src/trace/links.rs 0.0% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1515     +/-   ##
=======================================
- Coverage   68.9%   68.8%   -0.1%     
=======================================
  Files        137     137             
  Lines      19799   19844     +45     
=======================================
+ Hits       13658   13671     +13     
- Misses      6141    6173     +32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@hdost hdost left a comment

Choose a reason for hiding this comment

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

Looks good for the most part. I would have to look at the commit log but if this Link api was released in a previous version of the opentelemetry crate that we should have a CHANGELOG entry for the signature change and addition of the with_context

opentelemetry-sdk/src/trace/span.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

We need to wrap this API with otel_unstable as it's experimental

@cijothomas
Copy link
Member

We need to wrap this API with otel_unstable as it's experimental

Do we need to? The spec is about to get stable soon, so its not worth the effort to hide it under feature-flag now and later undo that.

@TommyCpp
Copy link
Contributor

TommyCpp commented Feb 8, 2024

Do we need to? The spec is about to get stable soon, so its not worth the effort to hide it under feature-flag now and later undo that.

Emm shall we wait for the spec PR to merge before we merge this one then? I am mostly worried we merge this and then spec made some changes before stablizing. Not sure how long it usually takes for spec to stablize

@cijothomas
Copy link
Member

Do we need to? The spec is about to get stable soon, so its not worth the effort to hide it under feature-flag now and later undo that.

Emm shall we wait for the spec PR to merge before we merge this one then? I am mostly worried we merge this and then spec made some changes before stablizing. Not sure how long it usually takes for spec to stablize

We are still pre 1.0, so its still okay? Or we can hold off from merging for few days as well and see if the spec actually gets stable.

@lalitb lalitb added the S-blocked-spec Status: Blocked on open or unresolved spec label Feb 8, 2024
@lalitb
Copy link
Member Author

lalitb commented Feb 8, 2024

We are still pre 1.0, so its still okay? Or we can hold off from merging for few days as well and see if the spec actually gets stable.

Yeah, will add the otel_unstable flag if it is not stable after (say) a couple of weeks.

@lalitb
Copy link
Member Author

lalitb commented Mar 11, 2024

The specs is marked as stable now - open-telemetry/opentelemetry-specification#3887. Should be good to review it now.

@lalitb lalitb removed the S-blocked-spec Status: Blocked on open or unresolved spec label Mar 11, 2024
Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

one nit otherwise LGTM

Co-authored-by: Zhongyang Wu <zhongyang.wu@outlook.com>
@cijothomas cijothomas merged commit ab39fd2 into open-telemetry:main Mar 19, 2024
16 of 17 checks passed
@lalitb lalitb mentioned this pull request Apr 28, 2024
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