[sdk+hosting] Move AddOpenTelemetry & OpenTelemetryBuilder into Hosting package #4174
Conversation
| @@ -21,5 +21,6 @@ | |||
| [assembly: InternalsVisibleTo("OpenTelemetry.Exporter.Prometheus.AspNetCore" + AssemblyInfo.PublicKey)] | |||
| [assembly: InternalsVisibleTo("OpenTelemetry.Exporter.Prometheus.AspNetCore.Tests" + AssemblyInfo.PublicKey)] | |||
| [assembly: InternalsVisibleTo("OpenTelemetry.Exporter.Prometheus.HttpListener.Tests" + AssemblyInfo.PublicKey)] | |||
| [assembly: InternalsVisibleTo("OpenTelemetry.Extensions.Hosting" + AssemblyInfo.PublicKey)] | |||
There was a problem hiding this comment.
@alanwest and I discussed keeping OpenTelemetryBuilder inside the SDK. I decided to move it to hosting because it wasn't used inside SDK at all after moving AddOpenTelemetry. We think there might be a better API we can ship for the Sdk.Create* style which also uses OpenTelemetryBuilder. What I'm thinking is in vNext (or whatever) we can explore that. We can always move OpenTelemetryBuilder back into SDK via TypeForwardedToAttribute. If we do that, then we can also remove this InternalsVisibleTo cheat, it is only needed because OpenTelemetryBuilder has to register some SDK internals.
There was a problem hiding this comment.
The other thing we discussed was whether the AddOpenTelemetry(this IServiceCollection) extension had any utility outside of a hosting use case. If we ever thought this might be useful, forwarding this extension might be trickier, no?
Though, it seems some folk find the non-hosting use case unlikely, so I suppose in that case should we just not concern ourselves with it?
There was a problem hiding this comment.
I can't think of any strong use case for needing to call AddOpenTelemetry beyond setting up your host. The main case for acting on the IServiceCollection beyond host setup is the library author case, but we have services.ConfigureTracerProviderBuilder & services.ConfigureMeterProviderBuilder for that.
If we found a use case and wanted to move AddOpenTelemetry back into SDK, you can move a type. So in this case OpenTelemetryServicesExtensions. It currently has some other obsolete methods, but the plan was to drop them before stable so if we do that, I think the potential to move it is there. But if we did move it we would be right back in the hole of IHostedService dependency not being part of SDK.
There was a problem hiding this comment.
But if we did move it we would be right back in the hole of IHostedService dependency not being part of SDK.
Right, this is the tricky part. Unless M.E.DI.A could introduce that hook for when the provider has been built. That said, I'm not really concerned since you're thinking AddOpenTelemetry goes hand-in-hand with a hosted use case.
Also, 👍 to:
but we have services.ConfigureTracerProviderBuilder & services.ConfigureMeterProviderBuilder for that.
There was a problem hiding this comment.
The reason I reverted #4151 and went this direction was that whole design was built on the hope that we could do something in .NET 8 to M.E.DI.A to supplant the need for IHostedService. I took a stab at making that so, and couldn't see a viable path to do it which wouldn't be breaking to custom IServiceProviders 😢 Also @noahfalk's feedback was the separation between "dropping services into a collection" and "starting up/requesting services" was intentional in the design.
|
|
||
| <!-- this is temporary. will remove in future PR. --> | ||
| <Nullable>disable</Nullable> | ||
| <Nullable>enable</Nullable> |
There was a problem hiding this comment.
Would it make sense to move this into a separate PR?
Just thinking it might simplify life. :)
There was a problem hiding this comment.
Fair point. I had to move some code in from SDK which was nullable enabled so I had to either clobber that and throw away usefulness or update the remaining things to be enabled. Nothing has been simple on this! 🤣
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4174 +/- ##
==========================================
- Coverage 85.59% 85.43% -0.16%
==========================================
Files 293 291 -2
Lines 11371 11380 +9
==========================================
- Hits 9733 9723 -10
- Misses 1638 1657 +19
|
Alternative to #4151
Includes #4164
Changes
AddOpenTelemetryextension andOpenTelemetryBuilderclass into the hosting package.StartWithHostextension.Details
This PR removes
StartWithHostby moving theIServiceCollectionconfiguration pattern into the hosting package. Users who want to use theIServiceCollectionstartup pattern will need the hosting dependency.TODOs
CHANGELOG.mdupdated for non-trivial changes(Will do documentation updates as a follow-up PR.)