[Geneva] Log error when buffer too small#4027
[Geneva] Log error when buffer too small#4027martincostello wants to merge 2 commits intoopen-telemetry:mainfrom
Conversation
Log an error if a metric is too large for the buffer. Fixes open-telemetry#1152.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4027 +/- ##
==========================================
+ Coverage 71.94% 71.97% +0.02%
==========================================
Files 458 448 -10
Lines 17872 17828 -44
==========================================
- Hits 12858 12831 -27
+ Misses 5014 4997 -17
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Add PR number.
There was a problem hiding this comment.
Pull request overview
Adds a dedicated error log path for Geneva metric TLV serialization buffer overflows, addressing #1152 by producing a clearer diagnostic event when a metric cannot fit into the fixed-size buffer.
Changes:
- Catch buffer-exhaustion exceptions during TLV metric export and emit a dedicated EventSource error event.
- Add a new ExporterEventSource event (ID 11) for “metric serialization buffer exceeded”.
- Add a unit test validating the new buffer-overflow log behavior and update Geneva exporter changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/OpenTelemetry.Exporter.Geneva.Tests/GenevaMetricExporterTests.cs | Adds a new test that forces a serialization buffer overflow and asserts the dedicated error event is emitted. |
| src/OpenTelemetry.Exporter.Geneva/Metrics/TlvMetricExporter.cs | Adds targeted exception handling to detect buffer overflow and log the new event. |
| src/OpenTelemetry.Exporter.Geneva/Internal/ExporterEventSource.cs | Introduces a new EventSource event (ID 11) with a clearer buffer-overflow message. |
| src/OpenTelemetry.Exporter.Geneva/CHANGELOG.md | Documents the new error logging behavior in Unreleased notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| catch (Exception ex) when (ex is IndexOutOfRangeException || ex is ArgumentException) | ||
| { | ||
| // The fixed-size serialization buffer's bounds were exceeded. | ||
| // Note: SerializeByte/SerializeUInt16/etc. throw IndexOutOfRangeException when | ||
| // the buffer index exceeds the array length; Encoding.UTF8.GetBytes throws | ||
| // ArgumentException on .NET 5+ when the destination span is too small. | ||
| if (ExporterEventSource.Log.IsEnabled(EventLevel.Error, EventKeywords.All)) | ||
| { | ||
| ExporterEventSource.Log.MetricSerializationBufferFull(metric.Name, GenevaMetricExporter.BufferSize); | ||
| } | ||
|
|
||
| result = ExportResult.Failure; | ||
| } |
There was a problem hiding this comment.
The exception filter catches all ArgumentException-derived exceptions, which includes ArgumentOutOfRangeException thrown by Socket.Send (and potentially other argument validation failures). That can misclassify transport/logic bugs as a "serialization buffer exceeded" condition and drops the original exception details. Consider narrowing this catch to only the specific encoding destination-too-small case (e.g., exclude ArgumentOutOfRangeException) or refactor serialization to throw a dedicated/consistent exception for buffer exhaustion and catch only that.
There was a problem hiding this comment.
I'm open to changing this, but it needs a bunch of refactoring to make the call sites that get detected for this more specific.
| var capturedEvents = new List<EventWrittenEventArgs>(); | ||
| var path = string.Empty; | ||
| Socket server = null; | ||
|
|
||
| using var listener = new BufferOverflowEventListener(capturedEvents); | ||
| try |
There was a problem hiding this comment.
EventListener callbacks can be raised on background threads while the test thread is asserting. Using a shared List here (and adding to it in OnEventWritten) is not thread-safe and can lead to intermittent failures (e.g., collection modified during enumeration). Use a thread-safe collection (ConcurrentQueue/ConcurrentBag) or protect accesses with a lock, and consider waiting until the expected event arrives before asserting.
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Fixes #1152
Changes
Log an error if a metric is too large for the buffer.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)