[sdk] Use reflection and dynamic types to start providers #4151

CodeBlanch · 2023-02-06T23:41:10Z

I've received a lot of feedback that StartWithHost isn't great 🤣 Chatted offline with some folks who seemed to think a reflection-based solution would be better so here goes...

Changes

Removes StartWithHost and uses magic to let the AddOpenTelemetry() extension (part of SDK) own that responsibility without a dependency on Microsoft.Extensions.Hosting.Abstractions.

Details

With this change the IHostedService currently registered into IServiceCollection by OpenTelemetry.Extensions.Hosting becomes part of the SDK. If the SDK finds the Microsoft.Extensions.Hosting.IHostedService type (reflectively) and RuntimeFeature.IsDynamicCodeSupported is true the SDK will automatically build an IHostedService and drop it into the IServiceCollection. We won't need the OpenTelemetry.Extensions.Hosting package at all if we do this... we can just mark it deprecated and remove the code from the solution. Users who do not have dynamic code support can either access the TracerProvider and/or MeterProvider manually from the IServiceProvider or use the "Sdk.Create*().Build()" configuration style (aka "Console" style).

TODOs

Appropriate CHANGELOG.md updated for non-trivial changes
Fix up tests
* [ ] Fix up documentation Will do this as a follow-up.
Changes in public API reviewed

… SDK.

samsp-msft · 2023-02-07T21:31:44Z

Relying on ref emit is going to cause problems with Native AOT and potentially trimming. While there will need to be application code differences for those apps - we want to keep that to a minimum, and I would suggest that this is such a fundamental scenario that having differences is going to be bad.

CodeBlanch · 2023-02-07T22:44:15Z

@samsp-msft What's funny is I spent much effort in .NET 7 and OTel 1.4 to remove our reflection emit only to put it back for this 🤣 We could add a dependency on Microsoft.Extensions.Hosting.Abstractions in the SDK to not need it, but I don't think many people would go for that. Plus it only works when an actual host is being used. .NET Framework, Console apps, it is just noise. I think we need to view this as an interim step. My hope was to work with the .NET team on an API for .NET 8 we could use to warm up the SDK and then we could remove this. Thoughts on that path?

codecov · 2023-02-07T23:18:27Z

Codecov Report

Merging #4151 (dd2f8c3) into main (1a9a492) will increase coverage by 0.01%.
The diff coverage is 84.92%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4151      +/-   ##
==========================================
+ Coverage   85.56%   85.58%   +0.01%     
==========================================
  Files         293      292       -1     
  Lines       11371    11467      +96     
==========================================
+ Hits         9730     9814      +84     
- Misses       1641     1653      +12

Impacted Files	Coverage Δ
...ensions.Hosting/OpenTelemetryServicesExtensions.cs	`0.00% <0.00%> (ø)`
...y/Internal/Shims/IgnoresAccessChecksToAttribute.cs	`0.00% <0.00%> (ø)`
src/OpenTelemetry/OpenTelemetryBuilder.cs	`63.63% <ø> (ø)`
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs	`80.14% <76.92%> (-0.33%)`	⬇️
src/OpenTelemetry/Internal/HostingHelper.cs	`89.42% <89.42%> (ø)`
...ronmentVariables/EnvironmentVariablesExtensions.cs	`100.00% <100.00%> (+60.00%)`	⬆️
...emetry/OpenTelemetryServiceCollectionExtensions.cs	`100.00% <100.00%> (ø)`
...ter.ZPages/Implementation/ZPagesActivityTracker.cs	`97.14% <0.00%> (-2.86%)`	⬇️
...tpListener/Internal/PrometheusCollectionManager.cs	`75.82% <0.00%> (+2.19%)`	⬆️
...Propagators/OpenTelemetryPropagatorsEventSource.cs	`100.00% <0.00%> (+12.50%)`	⬆️

reyang

LGTM. This is a nice user experience improvement / simplification. We need to follow up with .NET runtime folks and see how to eventually get rid of reflection (not blocking).

src/OpenTelemetry.Extensions.Hosting/CHANGELOG.md

cijothomas · 2023-02-08T16:28:33Z

src/OpenTelemetry/CHANGELOG.md

@@ -5,6 +5,10 @@
 * Removed the dependency on System.Reflection.Emit.Lightweight
  ([#4140](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4140))

+* The `AddOpenTelemetry` extension will now register an `IHostedService` if


lets do a more detailed write up here for folks updating to this version. Most end users do not really have the context about this, so we should write the steps one should follow when updating to this version.

Remove the nuget reference to otel.ext.hosting.

Remove the startwithhost().

(change ns based on the other PR from alan.)

Its okay to do this in a separate PR.

also need to write the steps for those users who have to manually retrieve MP/TP .

I will tackle this in a follow-up. Need to update documentation and then I'll drop a link here for more info.

src/OpenTelemetry/Internal/HostingHelper.cs

cijothomas · 2023-02-08T16:37:07Z

src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs

+            this.WriteEvent(48);
+        }
+
+        [Event(49, Message = "OpenTelemetry IHostedService application services registration skipped. Reason: '{0}'", Level = EventLevel.Informational)]


Warning? This is user-actionable right? We could point users to the steps they should take to resolve this.

There are 3 reasons we could log skipped:

RuntimeFeature.IsDynamicCodeSupported == false

Microsoft.Extensions.Hosting.IHostedService, Microsoft.Extensions.Hosting.Abstractions type could not be found

Reflection/dynamic code blew up. This will also cause an Error to be written for HostedServiceRegistrationFailure event.

1 + 2 could be normal/expected, just depends on who is setting up the host. I just updated it to be a warning but if any of that changed your mind LMK.

src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs

dpk83 · 2023-02-08T16:38:48Z

I don't think using reflection here is a clean approach. Why can't we have the existing AddOpenTelemetryMetrics and AddOpenTelemetryTracing APIs in Extensions.Hosting.

The Extensions.Hosting package is a different package that provides a better DI support model so this package exposing different APIs is better than using the hacky reflection here in my opinion.
Not only it will avoid reflection it will also help avoid the breaking changes for these APIs.

dpk83 · 2023-02-08T16:41:25Z

As a user I would prefer services.AddOpenTelmetryMetrics() over services.AddOpenTelemetry().WithMetrics().StartWithHost() anyday

docs/trace/getting-started-aspnetcore/Program.cs

dpk83 · 2023-02-08T16:48:56Z

src/OpenTelemetry/Internal/HostingHelper.cs

+        var iHostedServiceType = Type.GetType(
+            "Microsoft.Extensions.Hosting.IHostedService, Microsoft.Extensions.Hosting.Abstractions", throwOnError: false);
+
+        if (iHostedServiceType == null)


How will it behave for AzureFunctions. I believe IHostedService will be found on AzureFunctions but the hostedService infra is not supported.

I think Functions team added special support for OTel Hosted Service. I'll try to find that PR, but yes this is something to be tried out. (Independent of this, Otel 1.4 wont work in Functions unless dedicated plan, due to Functions not supporting DS 7.0)

https://github.com/Azure/azure-functions-host/blob/d4655cc4fbb34fc14e6861731991118a9acd02ed/src/WebJobs.Script.WebHost/DependencyInjection/DependencyValidator/DependencyValidator.cs#L57 This one. The ns etc needs to be changed to match this PR.

So, Otel is going to break for Azure functions with 1.4 upgrade? Shouldn't there be a major version bump if it's breaking the functionality for a group of customers?

I do not think so. There is no breaking change in 1.3 -> 1.4. Its just Azure Functions has a strict DS version dependency. As mentioned in the issue in Azure Functions host, the issue is not with OTel. Any app with DS 7.0 will not work.
Azure/azure-functions-host#8938

(IIRC, this issue was there from 1.0 of OTel onwards (Azure/azure-functions-host#7135) and it got fixed, but again broken (with every new .NET release). Hopefully by the time 1.4 is stable, Functions will fix it :)

Either way, lets use #4047 for this conversation, as this is unrelated to this PR.

If the issue is only with azure functions than it's okay. If it hits any regular .NET flavors than it's a major breaking change.

This is a great callout. Shamefully, I was not aware of this in Azure Functions! Playing with the code right now, I think we can make it backwards compatible with what functions is doing.

OK I just pushed an update that makes Azure Functions happy locally. Need to verify on an actual build of the packages but I think we're good.

@RohitRanjanMS Would you be able to help confirm if this will continue to work in Functions?

Thanks @cijothomas, yes, this will work for Functions.

cijothomas · 2023-02-08T16:56:22Z

Summarizing my understanding/thoughts:

With 1.4.0-rc3, OTel SDK has native DI capabilities.
We still needed Otel.Ext.Hosting package, purely to add a IHostedService to DI. The HostedService exists only to trigger retrieving Tracer/Meter Provider, which in turn, trigger building the whole SDK pipeline.
If there was something from DI that could be used instead to trigger building of pipeline, we wouldn't have the need for this IHostedService trick. Such capability do not exist today, and we hope to work with .NET team to get it added in future. Until that occurs, this reflection hack seems a reasonable option.

Also, we need to write a clear step-by-step guide for users upgrading to this version as an issue, and link to that issue from the package deprecations, Obsolete messages, and possibly from internal logs. I can imagine a lot of users (who are not actively following this), would be confused when the package goes missing, and all the breaking changes.

dpk83 · 2023-02-08T17:15:15Z

Such capability do not exist today, and we hope to work with .NET team to get it added in future. Until that occurs, this reflection hack seems a reasonable option.

When such ability gets into .NET, is it going to be in all .NET versions? If not than this reflection is here to live for a very long time, right?

Also, we need to write a clear step-by-step guide for users upgrading to this version as an issue, and link to that issue from the package deprecations, Obsolete messages, and possibly from internal logs. I can imagine a lot of users (who are not actively following this), would be confused when the package goes missing, and all the breaking changes.

It's a breaking change so wouldn't it demand a major version bump?

cijothomas · 2023-02-08T17:22:44Z

Also, we need to write a clear step-by-step guide for users upgrading to this version as an issue, and link to that issue from the package deprecations, Obsolete messages, and possibly from internal logs. I can imagine a lot of users (who are not actively following this), would be confused when the package goes missing, and all the breaking changes.

It's a breaking change so wouldn't it demand a major version bump?

There is no breaking change in any stable releases. Only in pre-release versions, so don't warrant major version bump.

samsp-msft · 2023-02-08T17:36:21Z

@samsp-msft What's funny is I spent much effort in .NET 7 and OTel 1.4 to remove our reflection emit only to put it back for this 🤣 We could add a dependency on Microsoft.Extensions.Hosting.Abstractions in the SDK to not need it, but I don't think many people would go for that. Plus it only works when an actual host is being used. .NET Framework, Console apps, it is just noise. I think we need to view this as an interim step. My hope was to work with the .NET team on an API for .NET 8 we could use to warm up the SDK and then we could remove this. Thoughts on that path?

I would suggest that taking a dependency on Microsoft.Extensions.Hosting.Abstractions from OpenTelemetry.Extensions.DependencyInjection would probably be cleaner and have less impact on customers than this change. There is going to be a big push for Native AOT for ASP.NET Core, and so this will be the wrong direction at that time,

…logic.

…etry-dotnet into sdk-hosting

cijothomas

some follow ups required in docs to help users migrate smoothly, but this should not block this PR.

…en-telemetry#4151)" This reverts commit b549e12.

)" (#4173) This reverts commit b549e12.

hejingkan2005 · 2023-02-13T11:56:21Z

@CodeBlanch , @cijothomas , I meet the same issue after migrating OTel to 1.4.0-rc.3, may I confirm with you whether this PR is the fix for the issue - "Could not load file or assembly 'System.Diagnostics.DiagnosticSource, version=7.0.0 ...'" with Azure function? What's the version of OTel is going to release?

Kielek · 2023-02-13T11:58:49Z

@hejingkan2005, please check 1.4.0-rc.4.

hejingkan2005 · 2023-02-13T13:38:42Z

@Kielek , thank you for your comment, but 1.4.0-rc.4 is not applicable as we get two other dependent packages which ask for version must equal to 1.4.0-rc.3:
+++++++++++++++
\SearchCrawler.WorkerFunction.csproj : error NU1608: Detected package version outside of dependency constraint: OpenTelemetry.Exporter.Geneva 1.4.0-rc.3 requires OpenTelemetry (= 1.4.0-rc.3) but version OpenTelemetry 1.4.0-rc.4 was resolved.
\SearchCrawler.WorkerFunction\SearchCrawler.WorkerFunction.csproj : error NU1608: Detected package version outside of dependency constraint: Azure.Monitor.OpenTelemetry.Exporter 1.0.0-beta.7 requires OpenTelemetry (= 1.4.0-rc.3) but version OpenTelemetry 1.4.0-rc.4 was resolved.
+++++++++++++++

cijothomas · 2023-02-13T15:58:39Z

@CodeBlanch , @cijothomas , I meet the same issue after migrating OTel to 1.4.0-rc.3, may I confirm with you whether this PR is the fix for the issue - "Could not load file or assembly 'System.Diagnostics.DiagnosticSource, version=7.0.0 ...'" with Azure function? What's the version of OTel is going to release?

This PR (which was reverted), is not for addressing any Functions specific issue.

CodeBlanch added 6 commits February 6, 2023 15:27

Remove StartWithHost extension and using reflection to do the work in…

d879a29

… SDK.

Warning cleanup.

7716560

Fix up.

acb39c2

Improved logging.

1d5df25

Remove OpenTelemetry.Extensions.Hosting dependency from examples & docs.

54eef54

Test fixes.

c66f283

Fixes and test updates.

c088a9d

Bug fix.

c29470e

CodeBlanch added 3 commits February 7, 2023 15:28

Patch CHANGELOGs.

0cd2c59

Clean up.

7a5036f

Merge from main.

a7d289e

CodeBlanch marked this pull request as ready for review February 7, 2023 23:35

CodeBlanch requested a review from a team February 7, 2023 23:35

reyang approved these changes Feb 8, 2023

View reviewed changes

Merge branch 'main' into sdk-hosting

1591396

cijothomas reviewed Feb 8, 2023

View reviewed changes

src/OpenTelemetry.Extensions.Hosting/CHANGELOG.md Show resolved Hide resolved

cijothomas reviewed Feb 8, 2023

View reviewed changes

src/OpenTelemetry/Internal/HostingHelper.cs Show resolved Hide resolved

cijothomas reviewed Feb 8, 2023

View reviewed changes

src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs Outdated Show resolved Hide resolved

cijothomas reviewed Feb 8, 2023

View reviewed changes

docs/trace/getting-started-aspnetcore/Program.cs Show resolved Hide resolved

dpk83 reviewed Feb 8, 2023

View reviewed changes

CodeBlanch added 4 commits February 8, 2023 13:18

Support Azure Functions TelemetryHostedService dependency inspection …

7be35d7

…logic.

Merge branch 'sdk-hosting' of https://github.com/CodeBlanch/opentelem…

a7e9608

…etry-dotnet into sdk-hosting

Code review.

f368c21

Code review.

b340749

cijothomas approved these changes Feb 8, 2023

View reviewed changes

Code review.

dd2f8c3

alanwest approved these changes Feb 8, 2023

View reviewed changes

alanwest merged commit b549e12 into open-telemetry:main Feb 8, 2023

CodeBlanch deleted the sdk-hosting branch February 9, 2023 00:02

CodeBlanch added a commit to CodeBlanch/opentelemetry-dotnet that referenced this pull request Feb 9, 2023

Revert "[sdk] Use reflection and dynamic types to start providers (op…

9c6a232

…en-telemetry#4151)" This reverts commit b549e12.

CodeBlanch mentioned this pull request Feb 9, 2023

Revert [sdk] Use reflection and dynamic types to start providers #4173

Merged

CodeBlanch added a commit that referenced this pull request Feb 9, 2023

Revert "[sdk] Use reflection and dynamic types to start providers (#4151

94f67d8

)" (#4173) This reverts commit b549e12.

CodeBlanch mentioned this pull request Feb 9, 2023

[sdk+hosting] Move AddOpenTelemetry & OpenTelemetryBuilder into Hosting package #4174

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sdk] Use reflection and dynamic types to start providers #4151

[sdk] Use reflection and dynamic types to start providers #4151

CodeBlanch commented Feb 6, 2023 •

edited

Loading

samsp-msft commented Feb 7, 2023

CodeBlanch commented Feb 7, 2023

codecov bot commented Feb 7, 2023 •

edited

Loading

reyang left a comment

cijothomas Feb 8, 2023

cijothomas Feb 8, 2023

CodeBlanch Feb 8, 2023

cijothomas Feb 8, 2023

CodeBlanch Feb 8, 2023

dpk83 commented Feb 8, 2023 •

edited

Loading

dpk83 commented Feb 8, 2023

dpk83 Feb 8, 2023

cijothomas Feb 8, 2023

cijothomas Feb 8, 2023

dpk83 Feb 8, 2023

cijothomas Feb 8, 2023

dpk83 Feb 8, 2023

CodeBlanch Feb 8, 2023

CodeBlanch Feb 8, 2023

cijothomas Feb 8, 2023

RohitRanjanMS Feb 9, 2023

cijothomas commented Feb 8, 2023

dpk83 commented Feb 8, 2023

cijothomas commented Feb 8, 2023

samsp-msft commented Feb 8, 2023

cijothomas left a comment

hejingkan2005 commented Feb 13, 2023 •

edited

Loading

Kielek commented Feb 13, 2023

hejingkan2005 commented Feb 13, 2023

cijothomas commented Feb 13, 2023

[sdk] Use reflection and dynamic types to start providers #4151

[sdk] Use reflection and dynamic types to start providers #4151

Conversation

CodeBlanch commented Feb 6, 2023 • edited Loading

Changes

Details

TODOs

samsp-msft commented Feb 7, 2023

CodeBlanch commented Feb 7, 2023

codecov bot commented Feb 7, 2023 • edited Loading

Codecov Report

reyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpk83 commented Feb 8, 2023 • edited Loading

dpk83 commented Feb 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas commented Feb 8, 2023

dpk83 commented Feb 8, 2023

cijothomas commented Feb 8, 2023

samsp-msft commented Feb 8, 2023

cijothomas left a comment

Choose a reason for hiding this comment

hejingkan2005 commented Feb 13, 2023 • edited Loading

Kielek commented Feb 13, 2023

hejingkan2005 commented Feb 13, 2023

cijothomas commented Feb 13, 2023

CodeBlanch commented Feb 6, 2023 •

edited

Loading

codecov bot commented Feb 7, 2023 •

edited

Loading

dpk83 commented Feb 8, 2023 •

edited

Loading

hejingkan2005 commented Feb 13, 2023 •

edited

Loading