-
Notifications
You must be signed in to change notification settings - Fork 887
Add otel.sdk.processor.log.processed metric #7486
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
Open
cijothomas
wants to merge
8
commits into
open-telemetry:main
Choose a base branch
from
cijothomas:cijothomas/sdk-self-obs-processor-log-processed
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
6a8f28c
Add otel.sdk.processor.log.processed metric
cijothomas 89ff78e
Fix CI: replace em-dashes with ASCII, fix markdown table style
cijothomas fa79e5a
Fix flaky QueueFull test: use blocking exporter for determinism
cijothomas 48f2b86
Add TODO for potential ObservableCounter optimization
cijothomas 6c5df34
Address review: adopt MeterFactory, share tags, single Add call
cijothomas c5c90ff
README: friendlier opt-in wording
cijothomas 2a53a4a
Merge branch 'main' into cijothomas/sdk-self-obs-processor-log-processed
cijothomas d79a6af
Fix log processor metric timing and shutdown handling
cijothomas File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,8 @@ public class BatchLogRecordExportProcessor : BatchExportProcessor<LogRecord> | |
| private readonly KeyValuePair<string, object?>[] successTags; | ||
| private readonly KeyValuePair<string, object?>[] queueFullTags; | ||
| private readonly KeyValuePair<string, object?>[] alreadyShutdownTags; | ||
| private volatile bool isShutdown; | ||
| private int activeOnEndCount; | ||
| private int isShutdown; | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="BatchLogRecordExportProcessor"/> class. | ||
|
|
@@ -49,69 +50,84 @@ public BatchLogRecordExportProcessor( | |
| this.successTags = baseTags; | ||
| this.queueFullTags = [.. baseTags, new("error.type", "queue_full")]; | ||
| this.alreadyShutdownTags = [.. baseTags, new("error.type", "already_shutdown")]; | ||
| this.ExportStarted = this.RecordSuccessfulProcessing; | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override void OnEnd(LogRecord data) | ||
| { | ||
| bool enqueued; | ||
| if (Volatile.Read(ref this.isShutdown) != 0) | ||
| { | ||
| SdkSelfObservability.LogProcessedCounter.Add(1, this.alreadyShutdownTags); | ||
| return; | ||
| } | ||
|
|
||
| Interlocked.Increment(ref this.activeOnEndCount); | ||
| try | ||
| { | ||
| if (Volatile.Read(ref this.isShutdown) != 0) | ||
| { | ||
| SdkSelfObservability.LogProcessedCounter.Add(1, this.alreadyShutdownTags); | ||
| return; | ||
| } | ||
|
|
||
| bool enqueued; | ||
|
|
||
| // Note: Intentionally not using Guard.ThrowIfNull to save prod cycles | ||
| // Note: Intentionally not using Guard.ThrowIfNull to save prod cycles | ||
| #pragma warning disable CA1062 // Validate arguments of public methods | ||
| switch (data.Source) | ||
| switch (data.Source) | ||
| #pragma warning restore CA1062 // Validate arguments of public methods | ||
| { | ||
| case LogRecord.LogRecordSource.FromSharedPool: | ||
| data.Buffer(); | ||
| data.AddReference(); | ||
| enqueued = this.TryExport(data); | ||
| if (!enqueued) | ||
| { | ||
| LogRecordSharedPool.Current.Return(data); | ||
| } | ||
|
|
||
| break; | ||
|
|
||
| case LogRecord.LogRecordSource.CreatedManually: | ||
| data.Buffer(); | ||
| enqueued = this.TryExport(data); | ||
| break; | ||
|
|
||
| case LogRecord.LogRecordSource.FromThreadStaticPool: | ||
| default: | ||
| Debug.Assert(data.Source == LogRecord.LogRecordSource.FromThreadStaticPool, "LogRecord source was something unexpected"); | ||
|
|
||
| // Note: If we are using ThreadStatic pool we make a copy of the record. | ||
| enqueued = this.TryExport(data.Copy()); | ||
| break; | ||
| } | ||
| { | ||
| case LogRecord.LogRecordSource.FromSharedPool: | ||
| data.Buffer(); | ||
| data.AddReference(); | ||
| enqueued = this.TryExport(data); | ||
| if (!enqueued) | ||
| { | ||
| LogRecordSharedPool.Current.Return(data); | ||
| } | ||
|
|
||
| // TODO: Consider switching to an ObservableCounter that piggybacks on | ||
| // CircularBuffer.AddedCount and DroppedCount to eliminate per-item | ||
| // Counter.Add() overhead. This would require a registry pattern for | ||
| // multiple instances but avoids any hot-path cost when a listener is active. | ||
| KeyValuePair<string, object?>[] tags; | ||
| break; | ||
|
|
||
| if (this.isShutdown) | ||
| { | ||
| tags = this.alreadyShutdownTags; | ||
| } | ||
| else if (!enqueued) | ||
| { | ||
| tags = this.queueFullTags; | ||
| case LogRecord.LogRecordSource.CreatedManually: | ||
| data.Buffer(); | ||
| enqueued = this.TryExport(data); | ||
| break; | ||
|
|
||
| case LogRecord.LogRecordSource.FromThreadStaticPool: | ||
| default: | ||
| Debug.Assert(data.Source == LogRecord.LogRecordSource.FromThreadStaticPool, "LogRecord source was something unexpected"); | ||
|
|
||
| // Note: If we are using ThreadStatic pool we make a copy of the record. | ||
| enqueued = this.TryExport(data.Copy()); | ||
| break; | ||
| } | ||
|
|
||
| if (!enqueued) | ||
| { | ||
| SdkSelfObservability.LogProcessedCounter.Add(1, this.queueFullTags); | ||
| } | ||
| } | ||
| else | ||
| finally | ||
| { | ||
| tags = this.successTags; | ||
| Interlocked.Decrement(ref this.activeOnEndCount); | ||
| } | ||
|
|
||
| SdkSelfObservability.LogProcessedCounter.Add(1, tags); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| protected override bool OnShutdown(int timeoutMilliseconds) | ||
| { | ||
| this.isShutdown = true; | ||
| Interlocked.Exchange(ref this.isShutdown, 1); | ||
|
|
||
| SpinWait spinner = default; | ||
| while (Volatile.Read(ref this.activeOnEndCount) != 0) | ||
| { | ||
| spinner.SpinOnce(); | ||
| } | ||
|
Comment on lines
+122
to
+126
Member
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. Should this honour |
||
|
|
||
| return base.OnShutdown(timeoutMilliseconds); | ||
| } | ||
|
|
||
| private void RecordSuccessfulProcessing(long count) | ||
| => SdkSelfObservability.LogProcessedCounter.Add(count, this.successTags); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,8 @@ public class SimpleLogRecordExportProcessor : SimpleExportProcessor<LogRecord> | |
|
|
||
| private readonly KeyValuePair<string, object?>[] successTags; | ||
| private readonly KeyValuePair<string, object?>[] alreadyShutdownTags; | ||
| private volatile bool isShutdown; | ||
| private int activeOnEndCount; | ||
| private int isShutdown; | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="SimpleLogRecordExportProcessor"/> class. | ||
|
|
@@ -37,15 +38,41 @@ public SimpleLogRecordExportProcessor(BaseExporter<LogRecord> exporter) | |
| /// <inheritdoc/> | ||
| public override void OnEnd(LogRecord data) | ||
| { | ||
| base.OnEnd(data); | ||
| SdkSelfObservability.LogProcessedCounter.Add( | ||
| 1, this.isShutdown ? this.alreadyShutdownTags : this.successTags); | ||
| if (Volatile.Read(ref this.isShutdown) != 0) | ||
| { | ||
| SdkSelfObservability.LogProcessedCounter.Add(1, this.alreadyShutdownTags); | ||
| return; | ||
| } | ||
|
|
||
| Interlocked.Increment(ref this.activeOnEndCount); | ||
| try | ||
| { | ||
| if (Volatile.Read(ref this.isShutdown) != 0) | ||
| { | ||
| SdkSelfObservability.LogProcessedCounter.Add(1, this.alreadyShutdownTags); | ||
| return; | ||
| } | ||
|
|
||
| SdkSelfObservability.LogProcessedCounter.Add(1, this.successTags); | ||
| base.OnEnd(data); | ||
| } | ||
| finally | ||
| { | ||
| Interlocked.Decrement(ref this.activeOnEndCount); | ||
| } | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| protected override bool OnShutdown(int timeoutMilliseconds) | ||
| { | ||
| this.isShutdown = true; | ||
| Interlocked.Exchange(ref this.isShutdown, 1); | ||
|
|
||
| SpinWait spinner = default; | ||
| while (Volatile.Read(ref this.activeOnEndCount) != 0) | ||
|
Comment on lines
+70
to
+71
Member
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. Same here. |
||
| { | ||
| spinner.SpinOnce(); | ||
| } | ||
|
|
||
| return base.OnShutdown(timeoutMilliseconds); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Should we defend against this callback throwing so that
Export()is still called?