Emit transaction.data inside contexts.trace.data

[bitsandfoxes]

Relates to getsentry/team-sdks#95
See getsentry/team-sdks#95 (comment) for more details.

Current Behavior

• We currently map extra to data in our SDK
• When serializing SentryTransaction, the extra data gets written to extra. The ITransactionTracer inherits from IHasExtra so this makes sense.
  

  sentry-dotnet/src/Sentry/SentryTransaction.cs

  Line 404 in 7024f37

  writer.WriteDictionaryIfNotEmpty("extra", _extra, logger);

Problem

The current inheritance structure prevents us from correctly mapping:

• Transaction data to contexts.trace.data
• Span data to span.data

Proposed Solution

Instead of modifying the inheritance structure for transactions and spans (which would be hacky), we should:

1. Add SetData methods to both TransactionTracer and SpanTracer
2. Keep existing SetExtra functionality
   • This allows users to quickly add contexts without creating a full context
   • Note: We do not support arbitrary key-value pairs on the context

Technical Details

• Transaction data should map to: contexts.trace.data
  • Implementation via ITraceContext so this gets written in the correct place automatically with
    

    sentry-dotnet/src/Sentry/SentryTransaction.cs

    Line 398 in 7024f37

    writer.WriteSerializableIfNotNull("contexts", _contexts.NullIfEmpty(), logger);

• Span data should map to: span.data

[bruno-garcia]

Span.Data should end up on on context.trace.data too:

sentry-dotnet/src/Sentry/SpanTracer.cs

Line 88 in 3f63cb6

public IReadOnlyDictionary<string, object?> Extra => _data;

So we might want to add an Obsolete on SetExtra if that was used to jsut make transaction and span "look alike"

[bruno-garcia]

Another thought, since we have:

sentry-dotnet/src/Sentry/SentryContexts.cs

Line 59 in 3f63cb6

public Trace Trace => _innerDictionary.GetOrCreate<Trace>(Trace.Type);

We could have Span.Data map to that property

[bruno-garcia]

@aritchie maybe a good first thing if you're willing to take a stab at this?

[jamescrosswell]

@bruno-garcia @bitsandfoxes in the issue description we've got:

1. Add SetData methods to both TransactionTracer and SpanTracer
2. Keep existing SetExtra functionality
   This allows users to quickly add contexts without creating a full context
   Note: We do not support arbitrary key-value pairs on the context

That seems a bit redundant. You'd end up with both SetExtra and SetData methods on the Span/Transaction Tracers... both of which would be used to add arbitrary additional information as string/object pairs and both of which would, under the hood, be transmitted to Sentry in the contexts.transaction.data.

From @cleptric 's comment here:

We keep setExtra() as an API in the SDK and seralize transaction.data onto trace.context.data in the envelope

It seems like what we should be doing is keeping the SetExtra methods on our Span/Transaction tracers (SDK users have been using these forever and are used to them). However when we transmit that data to Sentry it should be sent in the context.transaction.data rather than as an extra property on the transaction/span.

Just double checking this as it's how @aritchie implemented it in his PR (which makes sense to me).

[bitsandfoxes]

However when we transmit that data to Sentry it should be sent in the context.transaction.data rather than as an extra property on the transaction/span.

I don't agree. The extra is a like historic relic that we did not get around removing because it proved too useful. I'd rather not pin future product plans for extracting metrics and whatnot from spans on it. I'm not super hung up on unified API, but especially the tracing product, potentially connecting multiple services in different technologies, should be as close as possible.

[jamescrosswell]

So I had a chat to @bruno-garcia. He said we should basically mimic what they've done in the Java SDK. I think in that PR they don't actually touch the Transaction - it's mainly a modification to spans (which is weird, given the title of the PR... but anyway).

@aritchie apologies for the fuzzy initial requirements... I think we have to circle back on this one though as, presently, what we've done in this PR is a bit different to what they've done in the JavaSDK.

[aritchie]

@jamescrosswell This seemed to be the cleanest way to do this without introducing breaking changes. If I don't make changes to the SentryTransaction, it will serialize to the extras at the moment.

[jamescrosswell]

If I'm following what they're doing in the Java code correctly... although they're storing this in SpanContext.data (during a trace) it gets copied across to the transaction.contexts.data when the trace gets turned into a Transaction that gets captured and sent to Sentry. So far so good.

However they've also retained some separate Extra data:
https://github.com/getsentry/sentry-java/blob/44e3c4c8fdb5aadc16ff12ab0ea447ac18625882/sentry/src/test/java/io/sentry/JsonSerializerTest.kt#L936

This is a bit hard to see, since it looks like they removed it from the test here:

But further down in the file there are still references to it:

So, if I'm getting this right, we're supposed to leave the Extra stuff where it is on the Transaction. This makes sense because Extra is a property that exists on all Sentry events (like Messages and Exceptions, not just Transactions).

We simply need to add a Data field to the Transaction.Context.Trace.

For spans, I'm less sure. Would need to dig into the Java SDK further. It's possible they were already using that SpanContext on Spans and so, with that PR, they're implicitly adding a Span.Data field as well (but they didn't have to fiddle with the Span class itself to achieve this - came for free from the SpanContext in that case).

[aritchie]

@jamescrosswell So they don't want data MOVED from SentryTransaction, just another data collection added to the trace?

Spans already had data in place. No change was required there.

[aritchie]

@jamescrosswell Spans does have the Extras but serializes to data. Do you want to refactor all of the calls to SetData/Data?