Refactor exporter - step 3#1083
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1083 +/- ##
==========================================
- Coverage 76.66% 76.59% -0.08%
==========================================
Files 223 224 +1
Lines 6158 6165 +7
==========================================
+ Hits 4721 4722 +1
- Misses 1437 1443 +6
|
| } | ||
|
|
||
| public override Task<ExportResult> ExportAsync(IEnumerable<Activity> activityBatch, CancellationToken cancellationToken) | ||
| [MethodImpl(MethodImplOptions.Synchronized)] |
There was a problem hiding this comment.
I didn't even notice this until I read the description. My initial reaction... feels a little hackish. We shouldn't rely on users to add this, we should wrap the call in a lock if we want to make sure it isn't called concurrently? The spec says something about Export shouldn't be called concurrently, it doesn't say Exporters should implement Export so it isn't callable concurrently, which is what this feels like. 🤷
There was a problem hiding this comment.
Or how about having the SimpleExportActivityProcessor taking a parameter which allows people to specify whether it is reentrant or not (and by default use lock), I was thinking about ETW/LTTng scenario where folks want ultimate concurrency.
Something like this.
There was a problem hiding this comment.
I like the idea of a flag to opt into different behavior than what the spec says. We could also have a SimpleProcessor and a BatchingProcessor that are fully spec-compliant but also ship something like a ConcurrentProcessor or ReentrantProcessor that can be explicit in its changing of the rules?
There was a problem hiding this comment.
I moved the current logic to ReentrantExportActivityProcessor, and changed SimpleExportActivityProcessor to simply call the reentrant one with a lock guard.
There was a problem hiding this comment.
Now the PR got merged and OOP fans are staring at us - "Why are you guys making a non-reentrant class a child of reentrant one" 😂
There was a problem hiding this comment.
It is a bit odd, isn't it? I know you were shooting for code re-use, maybe the flag/option idea would be better? You could make a base class that has everything but the OnEnd part?
| { | ||
| private readonly ActivityExporterSync exporter; | ||
| private bool stopped; | ||
| private readonly object lck = new object(); |
There was a problem hiding this comment.
nit: Looks like "ick" 😄 Probably syncObject or lockObject is more common?
There was a problem hiding this comment.
I'm not a C# developer, borrowed/learned this from @cijothomas 😺
There was a problem hiding this comment.
:D Thanks for fixing the names throughout.
| /// </summary> | ||
| /// <param name="disposing"><see langword="true"/> to release both managed and unmanaged resources; <see langword="false"/> to release only unmanaged resources.</param> | ||
| protected override void Dispose(bool disposing) | ||
| { |
There was a problem hiding this comment.
@reyang Just noticed you're not calling Shutdown from Dispose. You did that on purpose, right?
You know, TracerProviderSdk doesn't seem to call Shutdown on anything, only Dispose. Why don't we just remove Shutdown methods across the board? I know the spec says we need it, but we have Dispose pattern in .NET for shutdown. Breaking with the spec here (in name only) might actually lead to a simpler, more easy-to-use & implement library?
There was a problem hiding this comment.
Tracked by #1024.
Yep, I think Dispose is essentially the language idiomatic version of Shutdown. Dispose doesn't take any parameter, Shutdown might potentially take a timeout value, and that can be engineered as a property of the object instead of an argument to the function.
3 participants
This is a follow up PR of #1078.
Changes
ConsoleExporterto the new path.[MethodImpl(MethodImplOptions.Synchronized)].For significant contributions please make sure you have completed the following items: