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

Simplify expression and improve perf to get parent span id #1088

Closed
wants to merge 1 commit into from

Conversation

shaun-cox
Copy link
Contributor

@shaun-cox shaun-cox commented May 31, 2023

Changes

Avoids calling Context.has_active_span() followed by Context.span(), which is two lookups,
and instead relies on checking for an invalid span context on the return value of the latter.

Code is a little easier to understand and shows about 4-6% perf improvement in start-end-span benchmarks.

My assumption here is that a span with an invalid span context could never be made active in the first place.

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)

- code is a little easier to understand and shows about 4-6%
  perf improvement in start-end-span benchmarks
@shaun-cox shaun-cox requested a review from a team as a code owner May 31, 2023 17:36
@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Patch coverage: 100.0% and project coverage change: -0.2 ⚠️

Comparison is base (0a893ce) 50.9% compared to head (52860aa) 50.7%.

❗ Current head 52860aa differs from pull request most recent head 0659f4c. Consider uploading reports for the commit 0659f4c to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1088     +/-   ##
=======================================
- Coverage   50.9%   50.7%   -0.2%     
=======================================
  Files        165     165             
  Lines      19689   19789    +100     
=======================================
+ Hits       10025   10040     +15     
- Misses      9664    9749     +85     
Impacted Files Coverage Δ
opentelemetry-sdk/src/trace/tracer.rs 93.4% <100.0%> (ø)

... and 21 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@djc
Copy link
Contributor

djc commented Jun 1, 2023

I think this is a good direction, but IMO it doesn't quite go far enough, so submitted #1089. Please have a look -- would be great to hear how that change affects your benchmarks.

opentelemetry-sdk/Cargo.toml Outdated Show resolved Hide resolved
SpanId::INVALID,
builder
.trace_id
.unwrap_or_else(|| config.id_generator.new_trace_id()),
Copy link
Member

Choose a reason for hiding this comment

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

Hm I don't think this will have identical behavior to the previous implementation as children of invalid spans may be valid (have valid trace ids if not explicitly set to invalid in builder).

@jtescher
Copy link
Member

jtescher commented Jun 1, 2023

Is the perf similar if you get the parent span context once and then check if it's valid subsequently, rather than checking if there is an active span before extracting the span context?

@hdost
Copy link
Contributor

hdost commented Aug 15, 2023

Superceded by #1089

@hdost hdost closed this Aug 15, 2023
@shaun-cox shaun-cox deleted the span-perf branch September 14, 2023 18:41
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.

None yet

4 participants