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.

Sign up for GitHub

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

Emit transaction.data inside contexts.trace.data #3936

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Emit transaction.data inside contexts.trace.data #3936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4292c46
Push SentryTransaction extras into the Contexts.Trace.Data element
aritchie Feb 3, 2025
253239f
Update SerializationTests.verify.cs
aritchie Feb 3, 2025
9d51787
Update serialization tests
aritchie Feb 3, 2025
58e4a1e
Update tests
aritchie Feb 3, 2025
590a182
Update CHANGELOG.md
aritchie Feb 3, 2025
d50a2dd
Update CHANGELOG.md
aritchie Feb 3, 2025
19bbb62
Switch serialization test to use json path tests instead of verify
aritchie Feb 4, 2025
b396668
Revert "Push SentryTransaction extras into the Contexts.Trace.Data el…
aritchie Feb 6, 2025
eda625a
Trace should not use interface, just set implementation methods. Sen…
aritchie Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## Unreleased

### Fixes

- Emit transaction.data inside contexts.trace.data ([#3936](https://github.com/getsentry/sentry-dotnet/pull/3936))

## 5.1.0

### Significant change in behavior
Expand Down
23 changes: 21 additions & 2 deletions src/Sentry/Protocol/Trace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,21 @@ internal set
/// <inheritdoc />
public bool? IsSampled { get; internal set; }

private Dictionary<string, object?> _data = new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could potentially back this by a Lazy<T> to avoid allocations when no Trace.Data is set.

We try to do this wherever possible - particularly for the trace classes as there can potentially be thousands of these allocated at any one time in larger applications.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So lazy is an allocation on its own and there is no computation here. I can add this. I could also set the initial size of the dictionary to 0 which is likely better. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could initialise the dictionary only if it's accessed... something like:

Dictionary<string, object?>? _data = null;
Dictionary<string, object?> Data => _data ??= new();


/// <summary>
/// Get the metadata
/// </summary>
public IReadOnlyDictionary<string, object?> Data => _data;

/// <summary>
/// Adds metadata to the trace
/// </summary>
/// <param name="key"></param>
/// <param name="value"></param>
public void SetData(string key, object? value)
=> _data[key] = value;

/// <summary>
/// Clones this instance.
/// </summary>
Expand All @@ -63,7 +78,8 @@ internal set
Operation = Operation,
Origin = Origin,
Status = Status,
IsSampled = IsSampled
IsSampled = IsSampled,
_data = _data.ToDict()
jamescrosswell marked this conversation as resolved.
Show resolved Hide resolved
};

/// <summary>
Expand Down Expand Up @@ -103,6 +119,7 @@ public void WriteTo(Utf8JsonWriter writer, IDiagnosticLogger? logger)
writer.WriteString("origin", Origin ?? Internal.OriginHelper.Manual);
writer.WriteStringIfNotWhiteSpace("description", Description);
writer.WriteStringIfNotWhiteSpace("status", Status?.ToString().ToSnakeCase());
writer.WriteDictionaryIfNotEmpty("data", _data, logger);

writer.WriteEndObject();
}
Expand All @@ -120,6 +137,7 @@ public static Trace FromJson(JsonElement json)
var description = json.GetPropertyOrNull("description")?.GetString();
var status = json.GetPropertyOrNull("status")?.GetString()?.Replace("_", "").ParseEnum<SpanStatus>();
var isSampled = json.GetPropertyOrNull("sampled")?.GetBoolean();
var data = json.GetPropertyOrNull("data")?.GetDictionaryOrNull() ?? new();

return new Trace
{
Expand All @@ -130,7 +148,8 @@ public static Trace FromJson(JsonElement json)
Origin = origin,
Description = description,
Status = status,
IsSampled = isSampled
IsSampled = isSampled,
_data = data
};
}
}
1 change: 1 addition & 0 deletions src/Sentry/SentryTransaction.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ public void AddBreadcrumb(Breadcrumb breadcrumb) =>
_breadcrumbs.Add(breadcrumb);

/// <inheritdoc />
[Obsolete("Add metadata to Contexts.Trace.SetData")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be doing this? I thought Extra was supposed to stay the way it is... or is this something you've seen it the docs or other SDKs?

Copy link
Member

Choose a reason for hiding this comment

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

I thought we were adding SetData to Transaction, but the field serializes under contexts.trace.data under the hood. @cleptric ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure what the answer is here? @bruno-garcia

Copy link
Member

@cleptric cleptric Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Deprecate extra and add SetData to Span and Transaction please.
Data from Transaction ends up on contexts.trace.data, from Span on its Data property directly.

public void SetExtra(string key, object? value) =>
_extra[key] = value;

Expand Down
50 changes: 50 additions & 0 deletions test/Sentry.Tests/SerializationTests.cs
View file
Open in desktop
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");
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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

Copy link
Member

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.

Copy link
Collaborator

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?

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.

var json = transaction.ToJsonString(_testOutputLogger);
_testOutputLogger.LogDebug(json);

var node = JsonNode.Parse(json);
var dataNode = node?["contexts"]?["trace"]?["data"]?["transaction1"]?.GetValue<string>();
dataNode.Should().NotBeNull("contexts.trace.data.transaction1 not found");
dataNode.Should().Be("transaction_value");

var spansNode = node?["spans"]?.AsArray();
spansNode.Should().NotBeNull("spans not found");
var spanDataNode = spansNode!.FirstOrDefault()?["data"]?["span1"]?.GetValue<string>();
spanDataNode.Should().NotBeNull("spans.data not found");
spanDataNode.Should().Be("value1");

// verify deserialization
var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json));
var el = JsonElement.ParseValue(ref reader);
var backTransaction = SentryTransaction.FromJson(el);

backTransaction.Spans.First().Extra["span1"].Should().Be("value1", "Span value missing");
backTransaction.Contexts.Trace.Data["transaction1"].Should().Be("transaction_value", "Transaction value missing");
}
}
9 changes: 1 addition & 8 deletions test/Sentry.Tests/SerializationTests.verify.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,8 @@

namespace Sentry.Tests;

public class SerializationTests
public partial class SerializationTests
{
private readonly IDiagnosticLogger _testOutputLogger;

public SerializationTests(ITestOutputHelper output)
{
_testOutputLogger = new TestOutputDiagnosticLogger(output);
}

[Theory]
[MemberData(nameof(GetData))]
public async Task Serialization(string name, object target)
Expand Down