-
-
Notifications
You must be signed in to change notification settings - Fork 225
Emit transaction.data inside contexts.trace.data #3936
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
Changes from 9 commits
4292c46
253239f
9d51787
58e4a1e
590a182
d50a2dd
19bbb62
b396668
eda625a
c480bec
b8f34d1
5c54924
330223d
d5400ad
c7cdedf
6491301
69b74f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,50 @@ | ||||||
| using System.Text.Json.Nodes; | ||||||
|
|
||||||
| namespace Sentry.Tests; | ||||||
|
|
||||||
| public partial class SerializationTests | ||||||
| { | ||||||
| private readonly IDiagnosticLogger _testOutputLogger; | ||||||
|
|
||||||
| public SerializationTests(ITestOutputHelper output) | ||||||
| { | ||||||
| _testOutputLogger = new TestOutputDiagnosticLogger(output); | ||||||
| } | ||||||
|
|
||||||
| [Fact] | ||||||
| public void Serialization_TransactionAndSpanData() | ||||||
| { | ||||||
| var hub = Substitute.For<IHub>(); | ||||||
| var context = new TransactionContext("name", "operation", new SentryTraceHeader(SentryId.Empty, SpanId.Empty, false)); | ||||||
| var transactionTracer = new TransactionTracer(hub, context); | ||||||
| var span = transactionTracer.StartChild("childop"); | ||||||
| span.SetExtra("span1", "value1"); | ||||||
|
|
||||||
| var transaction = new SentryTransaction(transactionTracer) | ||||||
| { | ||||||
| IsSampled = false | ||||||
| }; | ||||||
| transaction.Contexts.Trace.SetData("transaction1", "transaction_value"); | ||||||
|
||||||
| public static ITransactionTracer StartTransaction(string name, string operation) | |
| => CurrentHub.StartTransaction(name, operation); |
What I'd suggest then:
- We keep the existing
SetExtramethods 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we simply deprecate
setExtraand addsetDatain 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They want the collection called Data as well. I can't use an extension for the property.
Uh oh!
There was an error while loading. Please reload this page.