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

feat: clean up trace SDK APIs #1755

Merged
merged 9 commits into from
May 30, 2024

Conversation

TommyCpp
Copy link
Contributor

@TommyCpp TommyCpp commented May 14, 2024

Close some API as they are not intended to expose to users. This helps us control the scope of GA release.

Changes

  • Clean up provider() method as it's not suppose to expose to users
  • Clean up config() method in favor of Default implementation

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)

@cijothomas cijothomas mentioned this pull request May 14, 2024
3 tasks
@TommyCpp TommyCpp marked this pull request as ready for review May 27, 2024 21:16
@TommyCpp TommyCpp requested a review from a team May 27, 2024 21:16
Copy link

codecov bot commented May 27, 2024

Codecov Report

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

Project coverage is 74.4%. Comparing base (84c23a3) to head (63a8b66).

Files Patch % Lines
opentelemetry-zipkin/src/exporter/mod.rs 0.0% 2 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1755   +/-   ##
=====================================
  Coverage   74.4%   74.4%           
=====================================
  Files        122     122           
  Lines      19807   19800    -7     
=====================================
  Hits       14750   14750           
+ Misses      5057    5050    -7     

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

@TommyCpp TommyCpp changed the title feat: clean up trace SDK APIs and remove integration tests for jaeger feat: clean up trace SDK APIs May 27, 2024
@lalitb
Copy link
Member

lalitb commented May 28, 2024

I think this update to opentelemetry-proto submodule is not intended, as we are already using the latest version :)

@@ -61,13 +65,7 @@
- After `shutdown`, `LogProcessor` will not process any new logs
- Moving LogRecord implementation to the SDK. [1702](https://github.com/open-telemetry/opentelemetry-rust/pull/1702).
- Relocated `LogRecord` struct to SDK, as an implementation for the trait in the API.
- **Breaking** [#1729](https://github.com/open-telemetry/opentelemetry-rust/pull/1729)
Copy link
Member

Choose a reason for hiding this comment

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

we cannot remove this as it is already part of shipped code!

@cijothomas cijothomas merged commit 82fa485 into open-telemetry:main May 30, 2024
22 checks passed
@lalitb lalitb mentioned this pull request May 31, 2024
4 tasks
@djc
Copy link
Contributor

djc commented Jul 13, 2024

It seems unfortunate that this PR doesn't provide any kind of context or rationale in the description. This removes the opentelemetry_sdk::trace::Tracer::provider() method, which tracing-opentelemetry relies upon in its PreSampledTracer. I understand that this is a niche use case but as I understand it it's pretty important to tracing-opentelemetry. From what I can see there is no obvious alternative to this API -- how might we make PreSampledTracer work in the next release?

See the code here: https://github.com/tokio-rs/tracing-opentelemetry/blob/v0.1.x/src/tracer.rs#L67. This ends up accessing provider.config().id_generator.new_span_id() and provider.config().id_generator.new_trace_id().

@TommyCpp
Copy link
Contributor Author

Sorry for missing content in PR descriptions.

As for way forward, we can either:

  1. Re-open provider() method on tracer.
  2. Or it seems the tracing-opentelemetry cares more about the configuration associated with the tracer provider. Maybe it's better if Tracer return a reference to current configuration. One benifit we get is we can bound the lifetime of the configuration so if the configuration won't change while the users are reading it(TracerProvider may update the config if needed, we don't support it today but may support it in the future?)

@cijothomas
Copy link
Member

The rationale is mentioned in the changelog: https://github.com/open-telemetry/opentelemetry-rust/pull/1755/files#diff-14dfb7552119b2eca23adb12f587f7afba4f354c8dce5bcd2ae0e8a6a258e932R30-R31
PreSampledTracer is not a concept in OpenTelemetry, so I can't give a workaround for that use case, unfortunately.

It was unintentional to break tracing-opentelemetry, sorry about that! At the same time, it is also undesirable to expose public APIs not warranted by the spec.

@TommyCpp I am okay to revert the provider() part to prevent breaking tracing-opentelemetry this time. I'll hold off tagging and publishing to crates.io to resolve this. Can you make a PR reverting provider() being marked crate only?

The only way I see such issues from occurring again is by resolving #1571

@NOBLES5E
Copy link
Contributor

NOBLES5E commented Jul 13, 2024

By the way, TracerProvider::config() also needs to be marked public to prevent breaking tracing-opentelemetry.

It could be a good idea to doc comment the special accommodations for tokio tracing as mentioned in #1043, so that we will not unintentionally break them in the future.

@djc
Copy link
Contributor

djc commented Jul 13, 2024

Alternatively if we want to provide the minimal capability via the Tracer API we could only export access to the id generator or even specifically new_trace_id() and new_span_id() methods.

@djc djc mentioned this pull request Jul 15, 2024
4 tasks
@djc
Copy link
Contributor

djc commented Jul 15, 2024

Submitted #1934.

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.

5 participants