-
Notifications
You must be signed in to change notification settings - Fork 863
[OTLP] Fix NullReferenceException when no bucket boundaries configured for a view #6773
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
[OTLP] Fix NullReferenceException when no bucket boundaries configured for a view #6773
Conversation
- Handle `ExplicitBucketHistogramConfiguration.Boundaries` being empty. - Apply some IDE refactoring suggestions. Fixes open-telemetry#6771.
Add entry for bug.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6773 +/- ##
==========================================
- Coverage 86.69% 86.66% -0.04%
==========================================
Files 262 262
Lines 12319 12320 +1
==========================================
- Hits 10680 10677 -3
- Misses 1639 1643 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Add missing fullstop.
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.
Pull request overview
This PR fixes a NullReferenceException that occurs when serializing histogram metrics with empty bucket boundaries configured through a view. The fix adds a null check before attempting to serialize explicit bounds in the OTLP protobuf serializer.
- Added null safety check for
ExplicitBoundsbefore serialization - Applied IDE-suggested refactoring to use modern C# collection expressions and simplified lambda syntax
- Added comprehensive test coverage for histogram serialization with both explicit boundaries and empty boundaries
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/Serializer/ProtobufOtlpMetricSerializer.cs |
Added null check for buckets.ExplicitBounds to prevent NullReferenceException; simplified lambda parameter syntax |
src/OpenTelemetry/Metrics/View/ExplicitBucketHistogramConfiguration.cs |
Refactored to use collection expression [.. value] instead of value.ToArray() |
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/Implementation/Serializer/ProtobufOtlpMetricSerializerTests.cs |
Added tests for histogram serialization with explicit boundaries and no boundaries; refactored to extract common test logic |
test/OpenTelemetry.Tests/Shared/EventSourceTestHelper.cs |
Refactored to use collection expression [.. actualEvent.Payload!] instead of actualEvent.Payload!.ToArray() |
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/Implementation/Serializer/snapshots/ProtobufOtlpMetricSerializerTests.WriteMetricsData_Serializes_Metrics_With_No_Boundaries.verified.bin |
New snapshot file for testing histogram with no boundaries |
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/Implementation/Serializer/snapshots/ProtobufOtlpMetricSerializerTests.WriteMetricsData_Serializes_Metrics_With_Explicit_Boundaries.verified.bin |
New snapshot file for testing histogram with explicit boundaries |
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md |
Documented the bug fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@rajkumar-rangaraj, @alanwest, should we consider this as good enough reason to make 1.15.0 release (or even cherry-picking it to 1.14.1)? |
I suggest updating the patch version of |
...ricSerializerTests.WriteMetricsData_Serializes_Metrics_With_Explicit_Boundaries.verified.bin
Show resolved
Hide resolved
It is against our release policy. All packages should be versioned together |
Yes. Our policy requires all packages to be versioned together. The current process was introduced to reduce maintenance overhead during an aggressive development phase and to enable automation. While it helped from an engineering efficiency standpoint, in the current situation it is causing customer confusion and resulting in unnecessary, unrelated package version bumps. Based on customer feedback, this approach optimizes for our convenience but introduces real pain points for users. We should revisit the process and align it back with the release policy, even if that means slightly higher maintenance cost on our side. |
|
@Kielek @alanwest @martincostello Like to hear your thoughts on #6773 (comment) |
|
As a user, I think I would prefer to just get a single package update for the OTLP exporter with any fixes/changes, rather than have everything be updated. I think it would be different if it were a root node (say OpenTelemetry.Api) rather than a leaf, where people might not explicitly reference the OTLP package and then not get the fix, but as it's a leaf it should be pay-for-play, so there should be a package reference to update. Otherwise if we bump the version for everything, we'll be pushing out a new release and churn to users who don't use the OLTP exporter for no real reason (unless there's other unreleased changes they would benefit from). |
|
I agree with what you suggest, but it requires changes in the build pipeline. In current state I would consider merging pr related to separation certificate check from mtls and makindg 1.15.0. There is some good stuff waiting for the release |
|
We discussed this a bit in today's meeting. The consensus was that we wait to push out 1.15.0 for all packages. We're making the assumption that this is not an urgent bug that needs to be released immediately and can wait a few weeks. If anyone feels otherwise, please chime in. Generally though, we also discussed that if this were a more urgent bug then it would be ideal to have the option to release a patch version of only the affected package. We should consider making any necessary changes to the build pipeline to enable us to do this. |
Fixes #6771
Changes
ExplicitBucketHistogramConfiguration.Boundariesbeing empty.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)