fix: spans built from non-SpanContext parents crash or corrupt trace continuity#514
Open
reevejd wants to merge 2 commits into
Open
fix: spans built from non-SpanContext parents crash or corrupt trace continuity#514reevejd wants to merge 2 commits into
reevejd wants to merge 2 commits into
Conversation
Signed-off-by: James Reeve <james.reeve@ibm.com>
Contributor
|
Hi @reevejd A couple of minor suggestions before we merge:
Everything else looks good. I want to spend a bit more time verifying the fix to make sure we're not missing any other scenarios or code paths where this could surface — I'll post an update once I'm done. FYI - I closed and reopened the PR to trigger the test pipeline. |
Signed-off-by: James Reeve <james.reeve@ibm.com>
Author
|
Thanks @arjun-rajappa! I've made the changes you suggested. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #513
Problem
non_recording_spanwraps any object and exposes it as the span's#context, which thenext span start reads back as its parent. Three paths hand it something other than a
SpanContext, andshould_sample?then crashes the request withNoMethodError: undefined method 'tracestate':start_span's agent-not-ready path passes the rawOpenTelemetry::Context(theagent-warmup crash from the issue)
{}for any request without trace headers{}for connections processed outside a tracenon_recording_span(nil)also fabricates a random but validSpanContext, so a warmup request with no parent at all records later spans under a trace
id that was never reported.
Fix
I considered just adding a guard like my monkey-patch in the issue, which is enough to
stop the 500s, but it only changes the symptom: malformed parents keep flowing and requests
silently produce broken traces (dropped continuity, fabricated trace ids) instead of
crashing. So this PR fixes the producers and keeps the guard as another layer of defense:
tracer.rb: the not-ready path extracts the current span'sSpanContextfromwith_parent(defaulting to the current context, like the recording path), yieldingSpanContext::INVALIDwhen there is no parentsamplers.rb: readstracestateonly when the parent supports it, falling back toTracestate::DEFAULTinstead ofnil(the SpanContext contract requires a Tracestate)grpc.rb: headerless requests usenilinstead of{}and start a root spanaction_cable.rb: storesSpanContext::INVALIDinstead of{}Tests
Five of the six new tests fail against master, reproducing each defect: the raw-Context
parent, the fabricated trace ids, the sampler
NoMethodError, and a headerless gRPC callthat errors the RPC. The sixth pins tracestate propagation (with a tracestate distinct
from the default) so the new fallback can't silently drop real incoming tracestate.