Trace id propagation pitfall

[bruno-garcia]

Take the following snippet:

  var tran = sentryTrace is not null
             && SentryTraceHeader.Parse(sentryTrace) is { } trace
      ? SentrySdk.StartTransaction("AppStart", "activity.load", trace)
      : SentrySdk.StartTransaction("AppStart", "activity.load");

I marked this issue as a bug because we're crashing apps in case of a very "normal" (albeit not common) looking usage of the SDK.
One issue here is that if the service starting the trace had a DIsabled Sentry SDK, you could get all zeros here.
Repro: SDK starting the trace has a disabled DSN and does span.GetTraceHeader()

sentry-dotnet/src/Sentry/Internal/NoOpSpan.cs

Line 79 in 0e11682

public virtual SentryTraceHeader GetTraceHeader() => SentryTraceHeader.Empty;

So sentryTrace is not null or empty. And SentryTraceHeader.Parse successfully parses it.

But trying to start a trace, crashes my app.

So to use this API properly, one has to:

var tran = sentryTrace is not null
           && SentryTraceHeader.Parse(sentryTrace) is { } trace
           && trace.TraceId != SentryId.Empty
    ? SentrySdk.StartTransaction("AppStart", "activity.load", trace)
    : SentrySdk.StartTransaction("AppStart", "activity.load");

Some ideas:

1. StartTransaction becomes more lenient. It shouldn't crash! So if the id provided is invalid. Just start a new one
2. SentryTraceHeader.Parse returns null if the parsed trace header is all 0's

I rather we go with 1. And I'd also argue we should change the API to take a trace that is nullable.
So using the API becomes a LOT easier:

var tran = SentrySdk.StartTransaction("AppStart", "activity.load", SentryTraceHeader.Parse(sentryTrace))

Alternatively, we could take a string overload for sentryTrace. But we need to consider that having 3 string parameters in a row and other overloads might make the API confusing. (for example, see this case)

[jamescrosswell]

@bruno-garcia I'm trying to reproduce this but I'm not sure I follow how. I tried this:

var transaction = SentrySdk.StartTransaction("Program Main", "function");
var traceHeader = SentrySdk.GetTraceHeader();
WriteLine($"Trace header: {traceHeader}");

var sentryTrace = traceHeader?.ToString();

var tran = sentryTrace is not null
           && SentryTraceHeader.Parse(sentryTrace) is { } trace
    ? SentrySdk.StartTransaction("AppStart", "activity.load", trace)
    : SentrySdk.StartTransaction("AppStart", "activity.load");

WriteLine("Press any key to exit...");
ReadKey();

However traceHeader above is null:

sentry-dotnet/src/Sentry/Extensibility/DisabledHub.cs

Line 95 in 6e50eaa

public SentryTraceHeader? GetTraceHeader() => null;

Where does sentryTrace come from in your scenario? It looks like it would be a string that you get from somewhere...

[bruno-garcia]

It looks like it would be a string that you get from somewhere...

yeah it's a string I pass 'by hand'. In my case it's Appium and Android's Intent, but it could a message bus, or any other transport. We do that built-in via HTTP headers but there are other use cases

[jamescrosswell]

yeah it's a string I pass 'by hand'. In my case it's Appium and Android's Intent, but it could a message bus, or any other transport.

OK, and is that coming in via the header as all zeros? So we're getting a valid but empty traceid from somewhere upstream?

I'm trying to reproduce it basically so that I can validate whatever fix we put in place (and ideally have a unit test to prevent a regression).

[bruno-garcia]

Oh I see. Right, I fixed this on the caller side by checking if the Id I successfully parsed isn't an empty id: !trace.TraceId.Equals(SentryId.Empty).

The issue is that pan.GetTraceHeader() doesn't return null, it returns 0000. I believe it does so if tracing is disabled (don't recall exactly).

https://github.com/getsentry/symbol-collector/blob/4be060029975ae17ca1d881d0c0d5bbfbe33e38e/src/SymbolCollector.Runner/Program.cs#L123

[jamescrosswell]

Hm. I can't see anywhere in that SymbolCollector repo where it's trying to parse the string representation of a trace header... It gets the trace header in 3 places:

1. https://github.com/getsentry/symbol-collector/blob/de46301b0cea1d75746f4007af439f97608d6aec/src/SymbolCollector.Server/BatchFinalizer.cs#L80-L86
   • Used in StartTransaction
2. https://github.com/getsentry/symbol-collector/blob/c45c22fdb4962f4a04fd35a0ae729373a0ca57f2/src/SymbolCollector.Runner/Program.cs#L123-L126
   • Used in a string interpolation
3. https://github.com/getsentry/symbol-collector/blob/e98beaecaee0d0ff11b4e88f0a5fba17aaef9d07/src/SymbolCollector.Android/MainActivity.cs#L40
   • Eventually gets used in a StartTransaction

I still don't understand how we can reproduce the issue you ran into then.

I also tried this (just forcing the TraceHeader to be empty):

// This starts a new transaction and attaches it to the scope.
var traceHeader = new SentryTraceHeader(SentryId.Empty, SpanId.Empty, null);
WriteLine($"Trace header: {traceHeader}");
var transaction = SentrySdk.StartTransaction("Program Main", "function", traceHeader);
try
{
    WriteLine("Press any key to exit...");
    ReadKey();
}
finally
{
    transaction.Finish();
}

But this runs fine (no crash).

I'm not sure how we can go about fixing it then, since I'm not sure how/if/where there is a problem in the first place 😕

I think we close this issue for the time being and revisit if we ever find a way to reproduce.

[bruno-garcia]

I can show you exactly what happened, sorry I thought the issue description was clear enough.

https://github.com/getsentry/symbol-collector/blob/4be060029975ae17ca1d881d0c0d5bbfbe33e38e/src/SymbolCollector.Runner/Program.cs#L123-L126

As I wrote above, here is where I get the trace header as a string and pass it through 'my own transport' (Appium).

The issue is that pan.GetTraceHeader() doesn't return null, it returns 0000. I believe it does so if tracing is disabled (don't recall exactly).

More importantly:

I fixed this on the caller side by checking if the Id I successfully parsed isn't an empty id: !trace.TraceId.Equals(SentryId.Empty).

Then, I read it at:

https://github.com/getsentry/symbol-collector/blob/e98beaecaee0d0ff11b4e88f0a5fba17aaef9d07/src/SymbolCollector.Android/MainActivity.cs#L34

That gets passed to the Host.Init

https://github.com/getsentry/symbol-collector/blob/4be060029975ae17ca1d881d0c0d5bbfbe33e38e/src/SymbolCollector.Android.Library/Host.cs#L37C67-L37C78

And used further down at:

https://github.com/getsentry/symbol-collector/blob/4be060029975ae17ca1d881d0c0d5bbfbe33e38e/src/SymbolCollector.Android.Library/Host.cs#L110-L114

Which I added trace.TraceId != SentryId.Empty to make sure I don't crash by providing 0000 accidentally.

We can close this issue, it's not critical because most ppl only use HTTP and rely on our own trace propagation mechanism.
But it's definitely a pitfall since it works when you test having Sentry enabled, but once you disable and get empty SentryId.

I don't know how you weren't able to reproduce it though.