opentelemetry-rust icon indicating copy to clipboard operation
opentelemetry-rust copied to clipboard

Add add_link API

Open lalitb opened this issue 1 year ago • 5 comments

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 - https://github.com/open-telemetry/opentelemetry-specification/issues/3865. So not added under experimental flag.

Merge requirement checklist

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

lalitb avatar Feb 07 '24 21:02 lalitb

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 :warning:
opentelemetry/src/trace/mod.rs 33.3% 8 Missing :warning:
opentelemetry/src/global/trace.rs 0.0% 6 Missing :warning:
opentelemetry/src/trace/noop.rs 0.0% 4 Missing :warning:
opentelemetry-sdk/src/trace/links.rs 0.0% 3 Missing :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 07 '24 21:02 codecov[bot]

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.

cijothomas avatar Feb 08 '24 00:02 cijothomas

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

TommyCpp avatar Feb 08 '24 01:02 TommyCpp

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.

cijothomas avatar Feb 08 '24 01:02 cijothomas

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 avatar Feb 08 '24 17:02 lalitb

The specs is marked as stable now - https://github.com/open-telemetry/opentelemetry-specification/pull/3887. Should be good to review it now.

lalitb avatar Mar 11 '24 17:03 lalitb