Replace BaseScope with IScope #590
Conversation
Codecov Report
@@ Coverage Diff @@
## main #590 +/- ##
==========================================
- Coverage 81.58% 81.58% -0.01%
==========================================
Files 151 149 -2
Lines 3986 4023 +37
Branches 879 885 +6
==========================================
+ Hits 3252 3282 +30
- Misses 490 494 +4
- Partials 244 247 +3
Continue to review full report at Codecov.
|
src/Sentry/Scope.cs
Outdated
| /// <inheritdoc /> | ||
| [DataMember(Name = "breadcrumbs", EmitDefaultValue = false)] | ||
| [DontSerializeEmpty] | ||
| public ICollection<Breadcrumb> Breadcrumbs { get; } = new SynchronizedList<Breadcrumb>(); |
There was a problem hiding this comment.
I'm not a fan of this custom "synchronized list". Doesnt' ConcurrentBag and List both implement IEnumerable?
The extension method is internal and can type cast to the type we know we support in both implementation.
There was a problem hiding this comment.
IEnumerable doesn't have .Add().
ConcurrentBag implements ICollection but doesn't guarantee order.
There was a problem hiding this comment.
We don't want Add exposed to the user. We need Add ourselves on the AddBreadcrumb method (before we just typecast to the right type)
There was a problem hiding this comment.
Right about the order. I don't see the reason to change the way this was done before tbqh
There was a problem hiding this comment.
I wanted to avoid leaky abstraction from IScope, because then AddBreadcrumb would be coupled to both SentryEvent and Scope at runtime without a way to validate its assumptions statically.
There was a problem hiding this comment.
Another approach is to add AddBreadcrumb directly to IScope, which would eliminate the abstraction issues and would probably be the cleanest solution.
There was a problem hiding this comment.
Anyway, I don't really care about this too much and I guess it's safer to just keep things how they were. Reverted it back to ConcurrentQueue on Scope.
There was a problem hiding this comment.
OK adding to the interface sounds OK
src/Sentry/Scope.cs
Outdated
| /// <inheritdoc /> | ||
| [DataMember(Name = "extra", EmitDefaultValue = false)] | ||
| [DontSerializeEmpty] | ||
| public IDictionary<string, object?> Extra { get; } = new ConcurrentDictionary<string, object?>(); |
There was a problem hiding this comment.
In the old implementation these lists were not allocated at all. Now we get these two lists instatiated for each new Scope pushed/poped even if nothing is ever added
Rearranging scope data and cleaning up the code. Required for implementing transactions.
Some side effects of the changes:
BreadcrumbsfromConcurrentQueue<T>to a lock-synchronizedList<T>. This is becauseConcurrentQueue<T>doesn't implementICollection<T>or any other common abstraction that could be used inIScope. Synchronized list is less efficient, but for small collections that don't get updated on a hot path it's actually really good.nulldefaults for collections. As I understand it, this was only used to avoid serializing additional properties if they are unset. Replaced this behavior with a special attribute and a custom contract resolver instead. Previous workaround was also inefficient (serializer still dynamically read those properties, forcing them to initialize) and lead to unnecessarynulls appearing in the code base. After this change, the collections are now null-safe as an additional benefit. I didn't extend this on objects likeContextsandRequestbecause it's more difficult and probably not worth it. Ultimately, manual serialization fixes all those problems in a much more elegant way.