-
Notifications
You must be signed in to change notification settings - Fork 649
Enable OpenTelemetry by default and deprecate EnableOpenTelemetry #7577
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
Conversation
Co-authored-by: danielmarbach <174258+danielmarbach@users.noreply.github.com>
…rom tests Co-authored-by: danielmarbach <174258+danielmarbach@users.noreply.github.com>
Co-authored-by: danielmarbach <174258+danielmarbach@users.noreply.github.com>
Co-authored-by: danielmarbach <174258+danielmarbach@users.noreply.github.com>
Co-authored-by: danielmarbach <174258+danielmarbach@users.noreply.github.com>
…leOpenTelemetry test Co-authored-by: danielmarbach <174258+danielmarbach@users.noreply.github.com>
add8089 to
a8e29d1
Compare
a8e29d1 to
fa7f380
Compare
|
Wondering if we can do something like the following in the feature setup? if (!ActivitySources.Main.HasListeners())
return;In the feature setup. If is safe to assume that when Endpoint.Create/Start is invoked OTel is initialized? |
|
@ramonsmits I looked into this briefly and concluded there might be too many ordering dragons so I went with the most straightforward approach for now. Happy to revisit if you think it is feasible. Maybe the best way is to properly change the activity factory to follow those guidelines we have
|
|
@ramonsmits have a look. I think this is a viable approach that addresses your input too. |
|
I wonder if https://docs.particular.net/nservicebus/operations/opentelemetry it becomes clear that I can only see this disable mechanism as being useful for a case where you have AddSource in your standard template but for some weird reason you want to disable the traces creation. But given you could simply remove the sources I'm wondering why would this even be necessary? So the only reason I can come up with that we need this API is because A user COULD invoke Thoughts? |
I would drop that API completely. IMHO enabling OTel should be done only via its own API and we should follow/adapt. |
I tend to agree, especially because this is done in a major version the behavior change outlined in
can be added to the upgrade guide and the flag could be reintroduced later in a minor version if really needed. |
|
For reference @timbussmann original PR #6445 @mikeminutillo You mentioned
Maybe you'd like to review this also after reading #7577 (comment) |
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 enables OpenTelemetry instrumentation by default in NServiceBus, aligning with OpenTelemetry guidelines that telemetry should be emitted by default rather than requiring explicit opt-in.
Key Changes:
- OpenTelemetry feature now enabled by default in endpoint configuration
EnableOpenTelemetry()extension method deprecated as a compile-time error (v10) and removed in v11- ActivityFactory optimized with early exit when no diagnostic listeners are present
- Test infrastructure simplified by removing
OpenTelemetryEnabledEndpointand usingDefaultServerthroughout
Reviewed changes
Copilot reviewed 37 out of 38 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/NServiceBus.Core/EndpointConfiguration.cs | Added OpenTelemetryFeature to the list of default-enabled features |
| src/NServiceBus.Core/obsoletes-v10.cs | Added deprecation for EnableOpenTelemetry() extension method |
| src/NServiceBus.Core/OpenTelemetry/OpenTelemetryConfigurationExtensions.cs | Deleted file - functionality moved to obsoletes |
| src/NServiceBus.Core/Hosting/HostingComponent.Settings.cs | Removed EnableOpenTelemetry property (no longer needed) |
| src/NServiceBus.Core/Hosting/HostingComponent.Configuration.cs | Always uses ActivityFactory instead of conditionally switching to NoOpActivityFactory |
| src/NServiceBus.Core/OpenTelemetry/Tracing/ActivityFactory.cs | Added early exit optimization when no listeners present; refactored null checks to use early return pattern |
| src/NServiceBus.Core/OpenTelemetry/Tracing/NoOpActivityFactory.cs | Added sealed modifier for performance |
| src/NServiceBus.Core.Tests/OpenTelemetry/ActivityFactoryTests.cs | Added unit tests for no-listener scenario |
| src/NServiceBus.AcceptanceTests/Core/OpenTelemetry/When_no_listener_available.cs | New acceptance test verifying behavior when no diagnostic listeners are present |
| src/NServiceBus.AcceptanceTests/Core/OpenTelemetry/OpenTelemetryEnabledEndpoint.cs | Deleted - no longer needed since OpenTelemetry is now default |
| Multiple test files | Replaced OpenTelemetryEnabledEndpoint with DefaultServer; removed explicit EnableOpenTelemetry() calls |
| src/NServiceBus.Core.Tests/ApprovalFiles/APIApprovals.ApproveNServiceBus.approved.txt | Updated to show obsolete attribute on EnableOpenTelemetry |
| src/NServiceBus.AcceptanceTests/ApprovalFiles/When_pipelines_are_built.Should_preserve_order.approved.txt | Shows OpenTelemetry behaviors now in default pipeline |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/NServiceBus.AcceptanceTests/Core/OpenTelemetry/When_no_listener_available.cs
Outdated
Show resolved
Hide resolved
src/NServiceBus.AcceptanceTests/Core/OpenTelemetry/Metrics/When_retrying_messages.cs
Show resolved
Hide resolved
|
@mikeminutillo I'm merging. Should you have comments I'm happy to address those later |
OpenTelemetry guidelines specify telemetry should be emitted by default. Currently requires explicit opt-in via
endpointConfiguration.EnableOpenTelemetry().Changes
Core
OpenTelemetryFeatureto default-enabled features listEnableOpenTelemetryproperty removedEnableOpenTelemetry()as compile error (v10), removed in v11Tests
OpenTelemetryEnabledEndpoint- redundant since now defaultDefaultServerUsage
Before:
After:
Existing calls to
EnableOpenTelemetry()will produce compile errors with migration guidance.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
f.feedz.io/usr/bin/dotnet dotnet build src/NServiceBus.Core/NServiceBus.Core.csproj(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.