fix: include Data set via ITransactionTracer in SentryTransaction
#4148
Conversation
|
|
||
| /// <inheritdoc /> | ||
| public void SetData(string key, object? value) => _data[key] = value; | ||
| public void SetData(string key, object? value) => _contexts.Trace.SetData(key, value); |
There was a problem hiding this comment.
question: concurrency
The now used
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)
There was a problem hiding this comment.
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 IReadOnlyDictionary<string, object?> Data => _data; | ||
|
|
||
| public IReadOnlyDictionary<string, object?> Data => _contexts.Trace.Data; |
There was a problem hiding this comment.
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
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)
| Data: { | ||
| http.request.method: GET, | ||
| http.response.status_code: 200 | ||
| } |
There was a problem hiding this comment.
note: from
| @@ -244,6 +244,30 @@ public void SerializeObject_AllPropertiesSetToNonDefault_SerializesValidObject() | |||
|
|
|||
| return o; | |||
| }); | |||
|
|
|||
| Assert.Contains($$""" | |||
There was a problem hiding this comment.
note: I kept this test similar to
which also does a serialize/deserialize-roundtrip test
plus an additional "Contains-JSON" check on the serialized string
|
|
||
| /// <inheritdoc /> | ||
| public IReadOnlyDictionary<string, object?> Data => _data; | ||
|
|
||
| public IReadOnlyDictionary<string, object?> Data => _contexts.Trace.Data; |
There was a problem hiding this comment.
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
|
|
||
| /// <inheritdoc /> | ||
| public void SetData(string key, object? value) => _data[key] = value; | ||
| public void SetData(string key, object? value) => _contexts.Trace.SetData(key, value); |
There was a problem hiding this comment.
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.
fixes #4140
relates to #3936
Changes
SentryTransaction.DataDataofSentryContexts.TraceinsteadAdded links to discussions from the original PR (if I understood all the context correctly).
Added questions where I am uncertain whether it's the correct way of fixing the issue.