Emit transaction.data inside contexts.trace.data

[aritchie]

Resolves #3873

Removed serialization of "Extras" on SentryTransaction

Unit tests included

[jamescrosswell]

@aritchie awesome work getting a PR done on day 1! I wasn't necessarily even expecting you to be able to build the repo without some help/questions.

See comments about the name of the field though - I think the purpose of this PR is to change that from Extra to Data for protocol compatibility with the other SDKs.

[jamescrosswell]

Thanks @aritchie, couple of comments but Trace.Data is looking good now.

We also need to add Span.Data

Note that there is a bit of an equivalence between SpanTracer and SentrySpan... the SpanTracer class is the concrete implementation of ISpan that is used when traces are active. The SentrySpan class is the (mostly immutable) DTO that is used to serialise span data when sending events to Sentry.

Similarly, there are both TransactionTracer and SentryTransaction classes.

... just in case that wasn't obvious.

[bruno-garcia]

@AbhiPrasad I believe you know why we're doing this and the expected outcome, right? Could you help with a review?

Also tagged @adinauer as this is quite similar to the Java implementation, maybe you have thoughts?

[bruno-garcia]

This could stay here but my understanding is that we're "replacing" SetExtra on Transaction (Not on Event) with SetData. But it doesn't serialize on the top level document like extra does, it goes under contexts.trace.data

[aritchie]

My original PR redirected SetExtra to Contexts.Trace.Data so this is easy to do. Just not sure what you guys are looking for here.

[jamescrosswell]

@aritchie I think Michi's comment clarifies things. So it looks like this PR is on the right track.

I can't see a way to add SetData methods to Transaction and Span without it being a breaking change though. We can't add members to interfaces, but the performance APIs all use/expose interfaces publicly - e.g. here:

sentry-dotnet/src/Sentry/SentrySdk.cs

Lines 593 to 594 in a421af5

	public static ITransactionTracer StartTransaction(string name, string operation)
	=> CurrentHub.StartTransaction(name, operation);

What I'd suggest then:

• We keep the existing SetExtra methods and simply map these under the hood (as is already done in this PR) to the data properties on the protocol.
• We create another issue for the v6.0.0 milestone to add SetData methods to Transactions/Spans (Transactions might actually be gone by then anyway) and mark the Extra/SetExtra members as obsolete at that point
• For now, we don't add the [Obsolete] to any of the Extra/SetExtra members

[adinauer]

Could we simply deprecate setExtra and add setData in the current major version?

Regarding changing where this goes customers may have filtering logic in place, e.g. in beforeSend/beforeSendTransaction. Since for Java the change coincided with us working on a major version (v8), we opted for changing this in a major so customers are more careful when upgrading and don't end up sending PII by accident. Here's the Java PR: getsentry/sentry-java#3735 in case you want to compare something.

[jamescrosswell]

Could we simply deprecate setExtra and add setData in the current major version?

This would be a breaking change... adding a new member to an interface would break any existing classes implementing that interface - and this would involve modifying multiple interfaces (for historical reasons).

I think if it was a real showstopper, we could consider doing it. But in this case, changing the names of those methods from SetExtra to SetData doesn't really change anything except how SDK users use our SDK to add arbitrary data to traces and spans. It doesn't enable delivering any features that we want/need for Sentry. So it feels like a lot of work for very little gain.

I'd rather we do this in the next major release (v6.0) unless there is any compelling reason to do it now.

[aritchie]

@bruno-garcia @jamescrosswell Bruno & I had a discussion about this. Bruno agreed to the proposed workflow.
Technically, it doesn't break the signatures for existing calls. There is a new marker-ish interface to deal with the new methods.

I'm fine with whatever. I just want to put this "pr to bed"

[jamescrosswell]

@aritchie something we could do, which would not be a breaking change, would be to add a SetData extension method to IHasExtra, which simply called SetExtra.

Eventually in version 6.0 we could make those methods on the class rather than extension methods.

[aritchie]

I did try that. The issue was how much that interface touched. It meant adding all of those methods to a lot of other classes. Changing IHasExtra truly is a breaking change though because users that do implement it, would have to make updates to their models. In this case, they only have to update their Spans (3 classes at the moment)

[jamescrosswell]

Not an interface method/member... An extension method. It would basically just be an alternate syntax for calling Set Extra (without changes to any interfaces or classes).

[aritchie]

They want the collection called Data as well. I can't use an extension for the property.