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

feature: expose SimpleSpanProcessor::new #2119

Merged

Conversation

demurgos
Copy link
Contributor

@demurgos demurgos commented Sep 16, 2024

Fixes #2060

Changes

This commit update the visibility of the method SimpleSpanProcessor::new from pub(crate) to pub. This enables consumers to create SimpleSpanProcessor values for use with trace::provider::Builder::with_span_processor.

In particular, this fixes a consistency issue: the BatchSpanProcessor struct may already be built thanks to its pub builder. With this change, both processors in this repo may be built manually.

Merge requirement checklist

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

@demurgos demurgos requested a review from a team September 16, 2024 00:27
This commit update the visibility of the method `SimpleSpanProcessor::new` from `pub(crate)` to `pub`. This enables consumers to create `SimpleSpanProcessor` values for use with `trace::provider::Builder::with_span_processor`.

In particular, this fixes a consistency issue: the `BatchSpanProcessor` struct may already be built thanks to its `pub` builder. With this change, both processors in this repo may be built manually.

Closes open-telemetry#2060
@demurgos demurgos force-pushed the fix/2060-pub-simple-span-processor-new branch from 1e099bc to 6b40f20 Compare September 16, 2024 00:29
Copy link

codecov bot commented Sep 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.3%. Comparing base (7ab5e0f) to head (6b40f20).
Report is 1 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2119   +/-   ##
=====================================
  Coverage   78.3%   78.3%           
=====================================
  Files        121     121           
  Lines      20815   20815           
=====================================
+ Hits       16308   16309    +1     
+ Misses      4507    4506    -1     

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

Copy link
Member

@lalitb lalitb 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, specifically that it allows us to remove with_simple_processor and with_batch_processor methods in future as reasoned here.

@TommyCpp TommyCpp merged commit 7a074b5 into open-telemetry:main Sep 16, 2024
25 checks passed
@cijothomas
Copy link
Member

While I agree with this change (Thanks for fixing the inconsistency), I just want to call out that SimpleSpanProcessor is not going to scale well if you use it on any PROD scenarios. It makes blocking calls, so a Span.End might be blocked waiting for an http response, so Simpleprocessor is best left for debug/learning purposes. As long as you are leveraging it for test purposes no issues!

@demurgos
Copy link
Contributor Author

@cijothomas Thanks, I confirm that I only use it in development. In production I use the bathed span processor.

The simple span processor has the benefit of being immediate, which helps when debugging locally with traces sent to stdout.

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.

[Regression]: opentelemetry_sdk::TracerProvider::span_processors no longer public
4 participants