-
Notifications
You must be signed in to change notification settings - Fork 784
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.
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
Refactor exporter - step 6 #1094
Changes from 11 commits
1eae566
978504f
ed29fe4
785c26e
b2a3adf
c959eb2
f9122c0
f8b6ef3
87ffe58
bcd609e
d257e49
1bbb14c
41d34e6
02f34e2
dc824ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,10 +28,14 @@ namespace OpenTelemetry.Trace | |
public class BatchExportActivityProcessor : ActivityProcessor | ||
{ | ||
private readonly ActivityExporterSync exporter; | ||
private readonly CircularBuffer<Activity> queue; | ||
private readonly TimeSpan scheduledDelay; | ||
private readonly TimeSpan exporterTimeout; | ||
private readonly CircularBuffer<Activity> circularBuffer; | ||
private readonly int scheduledDelayMillis; | ||
private readonly int exporterTimeoutMillis; | ||
private readonly int maxExportBatchSize; | ||
private readonly Thread exporterThread; | ||
private readonly AutoResetEvent exportTrigger = new AutoResetEvent(false); | ||
private readonly ManualResetEvent dataExportedNotification = new ManualResetEvent(false); | ||
private readonly ManualResetEvent shutdownTrigger = new ManualResetEvent(false); | ||
private bool disposed; | ||
private long droppedCount = 0; | ||
|
||
|
@@ -71,10 +75,16 @@ public BatchExportActivityProcessor( | |
} | ||
|
||
this.exporter = exporter ?? throw new ArgumentNullException(nameof(exporter)); | ||
this.queue = new CircularBuffer<Activity>(maxQueueSize); | ||
this.scheduledDelay = TimeSpan.FromMilliseconds(scheduledDelayMillis); | ||
this.exporterTimeout = TimeSpan.FromMilliseconds(exporterTimeoutMillis); | ||
this.circularBuffer = new CircularBuffer<Activity>(maxQueueSize); | ||
this.scheduledDelayMillis = scheduledDelayMillis; | ||
this.exporterTimeoutMillis = exporterTimeoutMillis; | ||
this.maxExportBatchSize = maxExportBatchSize; | ||
this.exporterThread = new Thread(new ThreadStart(this.ExporterProc)) | ||
{ | ||
IsBackground = true, | ||
Name = $"OpenTelemetry-{nameof(BatchExportActivityProcessor)}-{exporter.GetType().Name}", | ||
}; | ||
this.exporterThread.Start(); | ||
} | ||
|
||
/// <summary> | ||
|
@@ -95,7 +105,7 @@ internal long ReceivedCount | |
{ | ||
get | ||
{ | ||
return this.queue.AddedCount + this.DroppedCount; | ||
return this.circularBuffer.AddedCount + this.DroppedCount; | ||
} | ||
} | ||
|
||
|
@@ -106,18 +116,18 @@ internal long ProcessedCount | |
{ | ||
get | ||
{ | ||
return this.queue.RemovedCount; | ||
return this.circularBuffer.RemovedCount; | ||
} | ||
} | ||
|
||
/// <inheritdoc/> | ||
public override void OnEnd(Activity activity) | ||
{ | ||
if (this.queue.TryAdd(activity, maxSpinCount: 50000)) | ||
if (this.circularBuffer.TryAdd(activity, maxSpinCount: 50000)) | ||
{ | ||
if (this.queue.Count >= this.maxExportBatchSize) | ||
if (this.circularBuffer.Count >= this.maxExportBatchSize) | ||
{ | ||
// TODO: signal the exporter | ||
this.exportTrigger.Set(); | ||
} | ||
|
||
return; // enqueue succeeded | ||
|
@@ -127,6 +137,74 @@ public override void OnEnd(Activity activity) | |
Interlocked.Increment(ref this.droppedCount); | ||
} | ||
|
||
/// <summary> | ||
/// Flushes the <see cref="Activity"/> currently in the queue, blocks | ||
/// the current thread until flush completed, shutdown signaled or | ||
/// timed out. | ||
/// </summary> | ||
/// <param name="timeoutMillis"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q.: why the preference for an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
None of the above gives any significant benefit though. |
||
/// The number of milliseconds to wait, or <c>Timeout.Infinite</c> to | ||
/// wait indefinitely. | ||
/// </param> | ||
/// <returns> | ||
/// Returns <c>true</c> when flush completed; otherwise, <c>false</c>. | ||
/// </returns> | ||
public bool ForceFlush(int timeoutMillis = Timeout.Infinite) | ||
{ | ||
if (timeoutMillis < 0 && timeoutMillis != Timeout.Infinite) | ||
{ | ||
throw new ArgumentOutOfRangeException(nameof(timeoutMillis)); | ||
} | ||
|
||
var tail = this.circularBuffer.RemovedCount; | ||
var head = this.circularBuffer.AddedCount; | ||
|
||
if (head == tail) | ||
{ | ||
return true; // nothing to flush | ||
} | ||
|
||
this.exportTrigger.Set(); | ||
|
||
if (timeoutMillis == 0) | ||
{ | ||
return false; | ||
} | ||
|
||
var triggers = new WaitHandle[] { this.dataExportedNotification, this.shutdownTrigger }; | ||
|
||
var sw = Stopwatch.StartNew(); | ||
|
||
while (true) | ||
{ | ||
if (timeoutMillis == Timeout.Infinite) | ||
{ | ||
WaitHandle.WaitAny(triggers); | ||
} | ||
else | ||
{ | ||
var timeout = (long)timeoutMillis - sw.ElapsedMilliseconds; | ||
|
||
if (timeout <= 0) | ||
{ | ||
return this.circularBuffer.RemovedCount >= head; | ||
} | ||
|
||
WaitHandle.WaitAny(triggers, (int)timeout); | ||
} | ||
|
||
if (this.circularBuffer.RemovedCount >= head) | ||
{ | ||
return true; | ||
} | ||
|
||
if (this.shutdownTrigger.WaitOne(0)) | ||
{ | ||
return false; | ||
} | ||
} | ||
} | ||
|
||
/// <inheritdoc/> | ||
/// <exception cref="OperationCanceledException">If the <paramref name="cancellationToken"/> is canceled.</exception> | ||
public override Task ForceFlushAsync(CancellationToken cancellationToken) | ||
|
@@ -135,6 +213,49 @@ public override Task ForceFlushAsync(CancellationToken cancellationToken) | |
throw new NotImplementedException(); | ||
} | ||
|
||
/// <summary> | ||
/// Attempt to drain the queue and shutdown the exporter, blocks the | ||
reyang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// current thread until shutdown completed or timed out. | ||
/// </summary> | ||
/// <param name="timeoutMillis"> | ||
/// The number of milliseconds to wait, or <c>Timeout.Infinite</c> to | ||
/// wait indefinitely. | ||
/// </param> | ||
public void Shutdown(int timeoutMillis = Timeout.Infinite) | ||
{ | ||
if (timeoutMillis < 0 && timeoutMillis != Timeout.Infinite) | ||
{ | ||
throw new ArgumentOutOfRangeException(nameof(timeoutMillis)); | ||
} | ||
|
||
if (timeoutMillis == Timeout.Infinite) | ||
{ | ||
this.ForceFlush(); | ||
this.shutdownTrigger.Set(); | ||
this.exporterThread.Join(); | ||
return; | ||
} | ||
|
||
if (timeoutMillis == 0) | ||
{ | ||
this.shutdownTrigger.Set(); | ||
return; | ||
} | ||
|
||
var sw = Stopwatch.StartNew(); | ||
|
||
this.ForceFlush(timeoutMillis); | ||
|
||
this.shutdownTrigger.Set(); | ||
|
||
var timeout = (long)timeoutMillis - sw.ElapsedMilliseconds; | ||
|
||
if (timeout > 0) | ||
{ | ||
this.exporterThread.Join((int)timeout); | ||
} | ||
} | ||
|
||
/// <inheritdoc/> | ||
/// <exception cref="OperationCanceledException">If the <paramref name="cancellationToken"/> is canceled.</exception> | ||
public override Task ShutdownAsync(CancellationToken cancellationToken) | ||
|
@@ -153,6 +274,9 @@ protected override void Dispose(bool disposing) | |
|
||
if (disposing && !this.disposed) | ||
{ | ||
// TODO: Dispose/Shutdown flow needs to be redesigned, currently it is convoluted. | ||
this.Shutdown(this.exporterTimeoutMillis); | ||
|
||
try | ||
{ | ||
this.exporter.Dispose(); | ||
|
@@ -165,5 +289,32 @@ protected override void Dispose(bool disposing) | |
this.disposed = true; | ||
} | ||
} | ||
|
||
private void ExporterProc() | ||
{ | ||
var triggers = new WaitHandle[] { this.exportTrigger, this.shutdownTrigger }; | ||
|
||
while (true) | ||
{ | ||
// only wait when the queue doesn't have enough items, otherwise keep busy and send data continuously | ||
if (this.circularBuffer.Count < this.maxExportBatchSize) | ||
{ | ||
WaitHandle.WaitAny(triggers, this.scheduledDelayMillis); | ||
} | ||
|
||
if (this.circularBuffer.Count > 0) | ||
{ | ||
this.exporter.Export(new Batch<Activity>(this.circularBuffer, this.maxExportBatchSize)); | ||
|
||
this.dataExportedNotification.Set(); | ||
this.dataExportedNotification.Reset(); | ||
Comment on lines
+295
to
+296
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this guaranteed to wake-up one waiting for the signal?
reyang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
if (this.shutdownTrigger.WaitOne(0)) | ||
{ | ||
break; | ||
} | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not a
Task
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having an explicit thread gives a lot of benefit:
SuppressInstrumentation
.