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

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

fix: include Data set via ITransactionTracer in SentryTransaction #4148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 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
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

- Work around iOS SHA1 bug ([#4143](https://github.com/getsentry/sentry-dotnet/pull/4143))
- Include `Data` set via `ITransactionTracer` in `SentryTransaction`([#4148](https://github.com/getsentry/sentry-dotnet/pull/4148))

## 5.6.0

Expand Down
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 @@ -159,15 +159,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
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
Member

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 @@ -278,10 +275,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
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
Member

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.


/// <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.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
Member Author

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
8 changes: 7 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,7 @@ 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);
}

[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 @@ -231,7 +231,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 @@ -244,6 +244,30 @@ public void SerializeObject_AllPropertiesSetToNonDefault_SerializesValidObject()

return o;
});

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

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