Refactor exporter - step 6 #1094
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1094 +/- ##
==========================================
- Coverage 75.04% 74.46% -0.58%
==========================================
Files 223 223
Lines 6295 6344 +49
==========================================
Hits 4724 4724
- Misses 1571 1620 +49
|
reyang
force-pushed
the
reyang/exporter6
branch
from
d2d504b to
8011fc9
Compare
reyang
force-pushed
the
reyang/exporter6
branch
from
8011fc9 to
f9122c0
Compare
| @@ -29,9 +29,13 @@ public class BatchExportActivityProcessor : ActivityProcessor | |||
| { | |||
| private readonly ActivityExporterSync exporter; | |||
| private readonly CircularBuffer<Activity> queue; | |||
There was a problem hiding this comment.
nit: not from your change but why not name the field circularBuffer? It is not a queue...
There was a problem hiding this comment.
After the change it seems we're still quite messy with names, as the spec is asking for maxQueueSize, so we have something weird like new CircularBuffer<Activity>(maxQueueSize).
| private readonly int maxExportBatchSize; | ||
| private readonly Thread exporterThread; |
There was a problem hiding this comment.
I think having an explicit thread gives a lot of benefit:
- The exporter thread runaway time would give idea on how much CPU cycles were used by exporting job.
- While debugging a live process or a dump, it is easy to see whether the exporting job got stuck or not.
- It avoids the accidental context leakage of
SuppressInstrumentation.
| /// timed out. Using a 32bit signed integer to specify the time | ||
| /// interval in milliseconds. | ||
| /// </summary> | ||
| /// <param name="timeoutMillis"> |
There was a problem hiding this comment.
Q.: why the preference for an int instead of a TimeSpan? I see TimeSpan as more user friendly but opinions differ :)
There was a problem hiding this comment.
- It aligns better with the spec.
- It allows default parameter to be specified in the signature directly, which results in simpler code.
- Debugging a crash dump would be simpler.
None of the above gives any significant benefit though.
| private readonly int maxExportBatchSize; | ||
| private readonly Thread exporterThread; | ||
| private readonly ManualResetEvent exportTrigger = new ManualResetEvent(false); |
There was a problem hiding this comment.
It seems that ManualResetEventSlim is a better option here. Any specific reason to not use it?
There was a problem hiding this comment.
I don't see the slim version giving benefit other than burning CPU cycles.
Most of these operations will wait for I/O operation which takes longer time so the spin will upgrade to a real event.
| this.dataExportedNotification.Set(); | ||
| this.dataExportedNotification.Reset(); |
There was a problem hiding this comment.
Is this guaranteed to wake up the one waiting?
| this.dataExportedNotification.Set(); | ||
| this.dataExportedNotification.Reset(); |
There was a problem hiding this comment.
Is this guaranteed to wake-up one waiting for the signal?
| processor.ForceFlush(); | ||
| processor.ForceFlush(); | ||
| processor.ForceFlush(); | ||
| processor.ForceFlush(); |
There was a problem hiding this comment.
There is a 🚽 joke to be made here, but I don't want to be a drain.
This is a follow up PR of #1078.
Changes
BatchExportActivityProcessor.For significant contributions please make sure you have completed the following items: