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

[API] Propagators: do not overwrite the active span with a default invalid span #2511

Merged

Conversation

ecourreges-orange
Copy link
Contributor

@ecourreges-orange ecourreges-orange commented Jan 25, 2024

[API] Fix b3, w3c and jaeger propagators: they will not overwrite the active span with a default invalid span
This is especially useful when used with CompositePropagator (#2504)

[TEST] Change wrong string "current-span" in unit tests to proper trace:kSpan which is "active_span"

Fixes #2504

Changes

This changes how Extract returns when invalid or missing headers are provided: it now returns the original context with potentially no active_span (or not overwritten if one was present).
But this should not have side effects on client code which calls GetSpan(context), which answers a default invalid span when it does not find an active_span

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@ecourreges-orange ecourreges-orange requested a review from a team January 25, 2024 15:26
@ecourreges-orange ecourreges-orange changed the title [WIP] [API] Fix b3, w3c and jaeger propagators: they will not overwrite the active span with a default invalid span [API] Fix b3, w3c and jaeger propagators: they will not overwrite the active span with a default invalid span Jan 25, 2024
@marcalff
Copy link
Member

Bonjour @ecourreges-orange

Thanks for the PR, CI build in progress.

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2320636) 87.08% compared to head (cf7a4fd) 87.12%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2511      +/-   ##
==========================================
+ Coverage   87.08%   87.12%   +0.05%     
==========================================
  Files         200      200              
  Lines        6103     6109       +6     
==========================================
+ Hits         5314     5322       +8     
+ Misses        789      787       -2     
Files Coverage Δ
...de/opentelemetry/trace/propagation/b3_propagator.h 93.66% <100.00%> (+0.21%) ⬆️
...entelemetry/trace/propagation/http_trace_context.h 95.59% <100.00%> (+1.65%) ⬆️
...i/include/opentelemetry/trace/propagation/jaeger.h 97.78% <100.00%> (+0.11%) ⬆️

... and 1 file with indirect coverage changes

… active span with a default invalid span, which is especially useful when used with CompositePropagator (open-telemetry#2504)

[TEST] Change wrong string "current-span" in unit tests to proper trace:kSpan which is "active_span"
@ecourreges-orange
Copy link
Contributor Author

Bonjour @ecourreges-orange

Thanks for the PR, CI build in progress.

You're welcome, happy to be contributing!

@marcalff marcalff changed the title [API] Fix b3, w3c and jaeger propagators: they will not overwrite the active span with a default invalid span [API] Propagators: do not overwrite the active span with a default invalid span Jan 25, 2024
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for everything:

  • the initial report
  • the analysis
  • the fix
  • the unit tests

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.

LGTM. Thanks for the fix.

@marcalff marcalff merged commit 7cc781e into open-telemetry:main Jan 29, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants