-
Notifications
You must be signed in to change notification settings - Fork 872
Fix NullReferenceException in WithMetrics when OTEL_SDK_DISABLED=true #6869
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: cijothomas <5232798+cijothomas@users.noreply.github.com>
Co-authored-by: cijothomas <5232798+cijothomas@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6869 +/- ##
==========================================
- Coverage 87.29% 87.25% -0.04%
==========================================
Files 263 263
Lines 12385 12387 +2
==========================================
- Hits 10811 10808 -3
- Misses 1574 1579 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
- Refactor to avoid suppressing analyzer warnings. - Update CHANGELOG. - Remove use of "crashes" in test. - Sort items. - Apply Visual Studio refactoring suggestions.
Add coverage for counters and histograms of different types.
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 occurred when OTEL_SDK_DISABLED=true is set. The issue was that OpenTelemetryMetricsListener assumed it would always receive a MeterProviderSdk instance but received a NoopMeterProvider when the SDK was disabled, causing null dereference on event subscriptions.
Changes:
- Made
OpenTelemetryMetricsListenerhandle nullableMeterProviderSdkwith appropriate null guards - Added comprehensive test for SDK disabled scenario
- Enhanced test coverage for byte, short, int, and float numeric types across metrics tests
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/OpenTelemetry/Metrics/IMetricsListener/OpenTelemetryMetricsListener.cs | Made meterProviderSdk nullable and added null guards in constructor, Dispose(), and InstrumentPublished() to handle NoopMeterProvider |
| test/OpenTelemetry.Extensions.Hosting.Tests/OpenTelemetryMetricsBuilderExtensionsTests.cs | Added test for SDK disabled scenario and expanded test coverage to include byte, short, int, float, and double counter types |
| test/OpenTelemetry.Extensions.Hosting.Tests/OpenTelemetry.Extensions.Hosting.Tests.csproj | Added reference to EnvironmentVariableScope test helper |
| test/OpenTelemetry.Tests/Metrics/MetricApiTests.cs | Added multithreaded counter and histogram tests for byte, short, int, and float types |
| test/OpenTelemetry.Exporter.OpenTelemetryProtocol.FuzzTests/Generators.cs | Updated fuzzing tests to include counters for all numeric types |
| src/OpenTelemetry.Extensions.Hosting/CHANGELOG.md | Documented the NullReferenceException fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.FuzzTests/Generators.cs
Show resolved
Hide resolved
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.FuzzTests/Generators.cs
Show resolved
Hide resolved
test/OpenTelemetry.Extensions.Hosting.Tests/OpenTelemetryMetricsBuilderExtensionsTests.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Extensions.Hosting.Tests/OpenTelemetryMetricsBuilderExtensionsTests.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Extensions.Hosting.Tests/OpenTelemetryMetricsBuilderExtensionsTests.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Extensions.Hosting.Tests/OpenTelemetryMetricsBuilderExtensionsTests.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Extensions.Hosting.Tests/OpenTelemetryMetricsBuilderExtensionsTests.cs
Outdated
Show resolved
Hide resolved
Remove redundant local variables.
When
OTEL_SDK_DISABLED=true,WithMetrics()crashes withNullReferenceException. TheOpenTelemetryMetricsListenerconstructor receives aNoopMeterProviderfrom DI but assumesMeterProviderSdk, leading to null dereference when subscribing to events.Changes
OpenTelemetryMetricsListener.cs
meterProviderSdkfield nullable (MeterProviderSdk?)Dispose(), andInstrumentPublished()Test coverage
WithMetricsWhenSdkDisabledTestverifying no crash when SDK disabledNoopMeterProviderreturned and meters remain usableExample
Original prompt
OTEL_SDK_DISABLEDis set to true #6868💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.