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

Dynamic Sampling Context propagates correctly for HttpClient spans #3208

Merged
merged 3 commits into from
Mar 12, 2024

Conversation

jamescrosswell
Copy link
Collaborator

@jamescrosswell jamescrosswell commented Mar 12, 2024

Resolves #3187

Propagate context after creating HttpClient request spans

The primary cause of the problem appears to be because we set the outbound trace headers before we create the span that represents the outgoing HttpRequest both here:

PropagateTraceHeaders(request, url);
var span = ProcessRequest(request, method, url);

And here:

PropagateTraceHeaders(request, url);
var span = ProcessRequest(request, method, url);

Propagate context for SpanTracers as well as TransactionTracers

There's a secondary problem however with the Hub.GetBaggage method:

public BaggageHeader GetBaggage()
{
if (GetSpan() is TransactionTracer { DynamicSamplingContext: { IsEmpty: false } dsc })
{
return dsc.ToBaggageHeader();
}
var propagationContext = ScopeManager.GetCurrent().Key.PropagationContext;
return propagationContext.GetOrCreateDynamicSamplingContext(_options).ToBaggageHeader();
}

This works when the current span is a TransactionTracer... in that case the DSC is read correctly from the current span and it looks like this:

  [0] = {KeyValuePair<string, string>} {Key: "trace_id"}
  [1] = {KeyValuePair<string, string>} {Key: "public_key"}
  [2] = {KeyValuePair<string, string>} {Key: "sample_rate"}

On the other hand, when the current span is a SpanTracer then GetOrCreateDynamicSamplingContext is called using the propagation context from the current scope, which does not contain a DSC... so a new one gets created, which looks like this:

[0] = {KeyValuePair<string, string>} {Key: "trace_id"}
[1] = {KeyValuePair<string, string>} {Key: "public_key"}
[2] = {KeyValuePair<string, string>} {Key: "release"}
[3] = {KeyValuePair<string, string>} {Key: "environment"}

It's got a release and an environment, but not a sample rate.

@bitsandfoxes I think the solution I've come up with for this makes sense, but would be good to get your eyes on it. I'm not sure I understand what the purpose of the default/fallback is and why this has a release/environment but not a sample rate.

@bitsandfoxes
Copy link
Contributor

I'm not sure I understand what the purpose of the default/fallback is and why this has a release/environment but not a sample rate.

The propagationContext relates to "Tracing without Performance". So this is in a state when there is no span, no transaction but we're still tracing. No sample rate makes sense in that case because there is nothing to sample.

Copy link
Contributor

@bitsandfoxes bitsandfoxes left a comment

Choose a reason for hiding this comment

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

🔥

@jamescrosswell jamescrosswell merged commit 0ee3904 into main Mar 12, 2024
32 checks passed
@jamescrosswell jamescrosswell deleted the fix/propagate-dsc-for-spans branch March 12, 2024 08:32
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.

Tracing has the wrong parent
2 participants