Skip to content

fix: include Data set via ITransactionTracer in SentryTransaction #4148

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

Merged
jamescrosswell merged 12 commits into from
Mar 19, 2026
Merged

fix: include Data set via ITransactionTracer in SentryTransaction #4148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e6e6c9b
test(trace): add expected data to trace context
Flash0ver Apr 28, 2025
f57868e
test(transaction): expect SetData via TransactionTracer
Flash0ver Apr 28, 2025
31e9125
test(transaction): expect "data" via TransactionTracer
Flash0ver Apr 28, 2025
13c2a16
fix(transaction): store Data from Tracer in Context
Flash0ver Apr 28, 2025
cb9e0c3
test(trace): add missing name
Flash0ver Apr 28, 2025
3db4876
test(AspNetCore): update verified
Flash0ver Apr 28, 2025
e91052d
docs(CHANGELOG): add entry to unreleased Fixes
Flash0ver Apr 28, 2025
3745efe
docs(CHANGELOG): set pull request
Flash0ver Apr 28, 2025
9216b0f
Merge branch 'main' into fix/transaction/tracer-setdata-not-sent
jamescrosswell Jul 13, 2025
0e9d7be
Merge branch 'main' into fix/transaction/tracer-setdata-not-sent
Flash0ver Mar 18, 2026
f401ea0
fix: change Trace's data from non-Concurrent to Concurrent-Dictionary
Flash0ver Mar 18, 2026
454ceaf
docs: fix spacing
Flash0ver Mar 18, 2026
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
### Fixes

- Common tags such as `Environment` and `Release` and custom event processors are all now correctly applied to CaptureFeedback events ([#4942](https://github.com/getsentry/sentry-dotnet/pull/4942))
- Include `Data` set via `ITransactionTracer` in `SentryTransaction` ([#4148](https://github.com/getsentry/sentry-dotnet/pull/4148))

## 6.2.0

Expand Down
17 changes: 17 additions & 0 deletions src/Sentry/Internal/Extensions/JsonExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,23 @@ public static void Deconstruct(this JsonProperty jsonProperty, out string name,
return result;
}

public static ConcurrentDictionary<string, object?>? GetConcurrentDictionaryOrNull(this JsonElement json)
{
if (json.ValueKind != JsonValueKind.Object)
{
return null;
}

var result = new ConcurrentDictionary<string, object?>();

foreach (var (name, value) in json.EnumerateObject())
{
result[name] = value.GetDynamicOrNull();
}

return result;
}

public static Dictionary<string, string?>? GetStringDictionaryOrNull(this JsonElement json)
{
if (json.ValueKind != JsonValueKind.Object)
Expand Down
8 changes: 4 additions & 4 deletions src/Sentry/Protocol/Trace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ internal set
/// <inheritdoc />
public bool? IsSampled { get; internal set; }

private Dictionary<string, object?> _data = new();
private ConcurrentDictionary<string, object?> _data = new();

/// <summary>
/// Get the metadata
Expand Down Expand Up @@ -79,7 +79,7 @@ public void SetData(string key, object? value)
Origin = Origin,
Status = Status,
IsSampled = IsSampled,
_data = _data.ToDict()
_data = new ConcurrentDictionary<string, object?>(_data),
};

/// <summary>
Expand Down Expand Up @@ -137,7 +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();
var data = json.GetPropertyOrNull("data")?.GetConcurrentDictionaryOrNull() ?? new();

return new Trace
{
Expand All @@ -149,7 +149,7 @@ public static Trace FromJson(JsonElement json)
Description = description,
Status = status,
IsSampled = isSampled,
_data = data
_data = data,
};
}
}
11 changes: 4 additions & 7 deletions src/Sentry/TransactionTracer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,15 +153,12 @@ public IReadOnlyList<string> Fingerprint
/// <inheritdoc />
public IReadOnlyCollection<Breadcrumb> Breadcrumbs => _breadcrumbs;

private readonly ConcurrentDictionary<string, object?> _data = new();

/// <inheritdoc />
[Obsolete("Use Data")]
public IReadOnlyDictionary<string, object?> Extra => _data;
public IReadOnlyDictionary<string, object?> Extra => _contexts.Trace.Data;

/// <inheritdoc />
public IReadOnlyDictionary<string, object?> Data => _data;

public IReadOnlyDictionary<string, object?> Data => _contexts.Trace.Data;
Copy link
Copy Markdown
Member Author

@Flash0ver Flash0ver Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

question: reference Contexts in SentryTransaction vs copy Contexts to SentryTransaction

When creating a SentryTransaction from ITransactionTracer, we reference the SentryContexts:

sentry-dotnet/src/Sentry/SentryTransaction.cs

Line 246 in 3745efe

Contexts = tracer.Contexts;

Should we instead copy the Contexts (via e.g. SentryContexts.Clone)?

Or is that an indicator that this fix here is not quite right, technically?
Should we maybe, rather than have SetData writing into SentryContexts.Trace.Data directly, instead when creating the SentryTransaction from the ITransactionTracer copy the ITransactionTracer.Data (backed by it's own Dictionary`2) over to the Context of the new SentryTransaction.
E.g. something like

class SentryTransaction
{
  public SentryTransaction(ITransactionTracer tracer) : this(tracer.Name, tracer.NameSource)
  {
    Contexts = tracer.Contexts;
    foreach (KeyValuePair<string, object?> data in tracer.Data)
    {
      Contexts.Trace.SetData(data.Key, data.Value);
    }
  }
}

or similar.
But that would have a side-effect on the Contexts of TransactionTracer, which I don't think is expected when creating a new SentryTransaction from TransactionTracer.
Should we perhaps indeed make a copy of the Trace and/or SentryContext?

Relates to #3936 (comment)

Copy link
Copy Markdown
Collaborator

@jamescrosswell jamescrosswell Mar 18, 2026

Choose a reason for hiding this comment

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

I think at this point the tracer is immutable - we're capturing the transaction trace so the trace should be finished and won't change.

Copy link
Copy Markdown
Member

@bruno-garcia bruno-garcia Apr 28, 2025

Choose a reason for hiding this comment

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

Note that if you're copying all objects from the collection while they are mutable and still on the scope, mutations done on the scope will be reflected on the transaction object resulting in possible inaccurate data being sent to Sentry, or potentially crashes when we read the data for serialization while the data is being mutated


private readonly ConcurrentDictionary<string, string> _tags = new();

Expand Down Expand Up @@ -271,10 +268,10 @@ internal TransactionTracer(IHub hub, ITransactionContext context, TimeSpan? idle

/// <inheritdoc />
[Obsolete("Use SetData")]
public void SetExtra(string key, object? value) => _data[key] = value;
public void SetExtra(string key, object? value) => _contexts.Trace.SetData(key, value);

/// <inheritdoc />
public void SetData(string key, object? value) => _data[key] = value;
public void SetData(string key, object? value) => _contexts.Trace.SetData(key, value);
Copy link
Copy Markdown
Member Author

@Flash0ver Flash0ver Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

question: concurrency

The now used

sentry-dotnet/src/Sentry/Protocol/Trace.cs

Line 53 in 3745efe

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

currently is a regular Dictionary`2.
Should this now become a ConcurrentDictionary`2?
Because it's also the private readonly ConcurrentDictionary<string, object?> _data = new(); that this PR removes from this type?

Or is that an indicator that this fix is technically not quite right?

Relates to #3936 (comment) and #3936 (comment)

Copy link
Copy Markdown
Member

@bruno-garcia bruno-garcia Apr 28, 2025

Choose a reason for hiding this comment

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

SentryTransaction shouldn't be accessed concurrently since it represents "a point in time" instance that gets serialized to Sentry.

The TransactionTracer is potentially accessed concurrently (we set it onto the Scope which has concurrent access in both global mode (every ConfigureScope call access the same scope instance) or non global mode (since even though there's a single Scope instance per thread, we do access/read things in order to make clones for new threads).

Throwing Concurrent around is not always the solution. In this case for sure not since we want SentryTrasaction to be a snapshot, so we need a deep clone here. We don't want just a clone of the collection if the items in the collection are mutable. I wrote a note above about this.

If things on the context are immutable, things are easier though we could have data loss if we have concurrent updates without synchronization.

Copy link
Copy Markdown
Collaborator

@jamescrosswell jamescrosswell Mar 18, 2026

Choose a reason for hiding this comment

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

Hm... I see what you're saying. In the original code the TrancactionTracer stored additional data in private readonly ConcurrentDictionary<string, object?> _data = new();

In the new code that data instead gets stored in _contexts.Trace._data which is private Dictionary<string, object?> _data = new();.

So yes, I think we should probably make the this a concurrent dictionary:

sentry-dotnet/src/Sentry/Protocol/Trace.cs

Line 53 in 4169cb7

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


/// <inheritdoc />
public void SetTag(string key, string value) => _tags[key] = value;
Expand Down
12 changes: 10 additions & 2 deletions test/Sentry.AspNetCore.Tests/WebIntegrationTests.Versioning.DotNet10_0.verified.txt
View file
Open in desktop
Copy link
Copy Markdown
Collaborator

@jamescrosswell jamescrosswell Mar 19, 2026

Choose a reason for hiding this comment

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

🎉

Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@
Origin: auto.http.aspnetcore,
Description: ,
Status: Ok,
IsSampled: true
IsSampled: true,
Data: {
http.request.method: GET,
http.response.status_code: 200
}
}
},
User: {
Expand Down Expand Up @@ -80,7 +84,11 @@
route.controller: Version,
route.version: 1.1
},
IsFinished: true
IsFinished: true,
Data: {
http.request.method: GET,
http.response.status_code: 200
}
}
}
]
Expand Down
12 changes: 10 additions & 2 deletions test/Sentry.AspNetCore.Tests/WebIntegrationTests.Versioning.DotNet8_0.verified.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@
Origin: auto.http.aspnetcore,
Description: ,
Status: Ok,
IsSampled: true
IsSampled: true,
Data: {
http.request.method: GET,
http.response.status_code: 200
}
Comment on lines +26 to +29
Copy link
Copy Markdown
Member Author

@Flash0ver Flash0ver Apr 28, 2025

Choose a reason for hiding this comment

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

note: from

}
},
User: {
Expand Down Expand Up @@ -80,7 +84,11 @@
route.controller: Version,
route.version: 1.1
},
IsFinished: true
IsFinished: true,
Data: {
http.request.method: GET,
http.response.status_code: 200
}
}
}
]
Expand Down
12 changes: 10 additions & 2 deletions test/Sentry.AspNetCore.Tests/WebIntegrationTests.Versioning.DotNet9_0.verified.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@
Origin: auto.http.aspnetcore,
Description: ,
Status: Ok,
IsSampled: true
IsSampled: true,
Data: {
http.request.method: GET,
http.response.status_code: 200
}
}
},
User: {
Expand Down Expand Up @@ -80,7 +84,11 @@
route.controller: Version,
route.version: 1.1
},
IsFinished: true
IsFinished: true,
Data: {
http.request.method: GET,
http.response.status_code: 200
}
}
}
]
Expand Down
9 changes: 8 additions & 1 deletion test/Sentry.Tests/Protocol/Context/TraceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public void SerializeObject_AllPropertiesSetToNonDefault_SerializesValidObject()
SpanId = SpanId.Parse("2000000000000000"),
TraceId = SentryId.Parse("75302ac48a024bde9a3b3734a82e36c8")
};
trace.SetData("route", "home");

// Act
var actual = trace.ToJsonString(_testOutputLogger, indented: true);
Expand All @@ -52,7 +53,10 @@ public void SerializeObject_AllPropertiesSetToNonDefault_SerializesValidObject()
"trace_id": "75302ac48a024bde9a3b3734a82e36c8",
"op": "op123",
"origin": "auto.abc.def.ghi",
"status": "aborted"
"status": "aborted",
"data": {
"route": "home"
}
}
""",
actual);
Expand All @@ -72,6 +76,7 @@ public void Clone_CopyValues()
SpanId = SpanId.Parse("2000000000000000"),
TraceId = SentryId.Parse("75302ac48a024bde9a3b3734a82e36c8")
};
trace.SetData("previous_route", "home");

// Act
var clone = trace.Clone();
Expand All @@ -84,6 +89,8 @@ public void Clone_CopyValues()
Assert.Equal(trace.ParentSpanId, clone.ParentSpanId);
Assert.Equal(trace.SpanId, clone.SpanId);
Assert.Equal(trace.TraceId, clone.TraceId);
Assert.Equal(trace.Data, clone.Data);
Assert.NotSame(trace.Data, clone.Data);
}

[Fact]
Expand Down
26 changes: 25 additions & 1 deletion test/Sentry.Tests/Protocol/SentryTransactionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ public void SerializeObject_AllPropertiesSetToNonDefault_SerializesValidObject()

// Act
var finalTransaction = new SentryTransaction(transaction);
var actualString = finalTransaction.ToJsonString(_testOutputLogger);
var actualString = finalTransaction.ToJsonString(_testOutputLogger, indented: true);
var actual = Json.Parse(actualString, SentryTransaction.FromJson);

// Assert
Expand All @@ -278,6 +278,30 @@ public void SerializeObject_AllPropertiesSetToNonDefault_SerializesValidObject()

return o;
});

Assert.Contains($$"""
Copy link
Copy Markdown
Member Author

@Flash0ver Flash0ver Apr 28, 2025

Choose a reason for hiding this comment

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

note: I kept this test similar to

sentry-dotnet/test/Sentry.Tests/Protocol/SentryEventTests.verify.cs

Line 6 in 3745efe

public async Task SerializeObject_AllPropertiesSetToNonDefault_SerializesValidObject()

which also does a serialize/deserialize-roundtrip test
plus an additional "Contains-JSON" check on the serialized string

"contexts": {
".NET Framework": {
".NET Framework": "\u0022v2.0.50727\u0022, \u0022v3.0\u0022, \u0022v3.5\u0022",
".NET Framework Client": "\u0022v4.8\u0022, \u0022v4.0.0.0\u0022",
".NET Framework Full": "\u0022v4.8\u0022"
},
"context_key": "context_value",
"trace": {
"type": "trace",
"span_id": "{{context.SpanId}}",
"parent_span_id": "{{context.ParentSpanId}}",
"trace_id": "{{context.TraceId}}",
"op": "op123",
"origin": "auto.serialize.transaction",
"description": "desc123",
"status": "aborted",
"data": {
"extra_key": "extra_value"
}
}
}
""", actualString);
}

[Fact]
Expand Down
Loading