[hosting] Register OpenTelemetry at the beginning of IServiceCollection #4883

danelson · 2023-09-25T14:56:10Z

Changes

Attempt to add OpenTelemetry services to the beginning of the IServiceCollection. This will reduce the likelihood of missing telemetry emitted in hosted services.

Prior to this change the ordering of registered services impacted whether telemetry was exported. Using the following MyHostedService as an example that emits a span named test.

public class MyHostedService : BackgroundService
{
    private ActivitySource activitySource = new ActivitySource(nameof(MyHostedService));
    public MyHostedService()
    {
    }

    protected override Task ExecuteAsync(CancellationToken stoppingToken)
    {
        using (var activity = this.activitySource.StartActivity("test"))
        {
        }
        return Task.CompletedTask;
    }
}

Prior to this change the following would not emit test because MyHostedService was registered before the OpenTelemetry services.

public static IHostBuilder CreateHostBuilder(string[] args) =>
    Host.CreateDefaultBuilder(args)
        .ConfigureServices((hostContext, services) =>
        {
            services.AddHostedService<MyHostedService>();
            services.AddOpenTelemetry()
                .WithTracing(builder => builder
                    .AddSource(nameof(MyHostedService))
                    .AddOtlpExporter(configure =>
                    {
                        configure.Endpoint = new Uri("http://custom:4317/v1/traces");
                    }));
        });

Note that after this change the previous example works but it is still possible to run into issues by doing something like

public static IHostBuilder CreateHostBuilder(string[] args) =>
    Host.CreateDefaultBuilder(args)
        .ConfigureServices((hostContext, services) =>
        {
            services.AddOpenTelemetry()
                .WithTracing(builder => builder
                    .AddSource(nameof(MyHostedService))
                    .AddOtlpExporter(configure =>
                    {
                        configure.Endpoint = new Uri("http://custom:4317/v1/traces");
                    }));
            services.Insert(0, ServiceDescriptor.Singleton<IHostedService, MyHostedService>());
        });

Merge requirement checklist

CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes
Changes in public API reviewed (if applicable)

danelson · 2023-09-25T14:57:19Z

src/OpenTelemetry.Extensions.Hosting/OpenTelemetryServicesExtensions.cs

@@ -47,8 +46,10 @@ public static OpenTelemetryBuilder AddOpenTelemetry(this IServiceCollection serv
    {
        Guard.ThrowIfNull(services);

-        services.TryAddEnumerable(
-            ServiceDescriptor.Singleton<IHostedService, TelemetryHostedService>());
+        if (!services.Any((ServiceDescriptor d) => d.ServiceType == typeof(IHostedService) && d.ImplementationType == typeof(TelemetryHostedService)))


Not able to comment on the line but should the summary/remarks this method calls this out?

Good idea to update the remarks! I would make it "Notes:" and then use a bulleted list. There should be other examples around doing that. Probably also a good idea to update the CHANGELOG. Something like * Changed the behavior of the OpenTelemetryBuilder.AddOpenTelemetry extension to INSERT OpenTelemetry services at index 0 instead of ADDING at the end to improve the experience of users capturing telemetry in hosted services registered before the OpenTelemetry registration. Kind of a mouthful feel free to improve it 😄

the entire TelemetryHostedService is an internal implementation detail, so probably okay to not mention about it in the summary.

changelog suggestions : lets clarify that this still does not guarantee as the TelemetryHostedService may not have done initializing while other hosted services start, so possible to still miss telemetry until its fully initialized.

Let me know if you think the wording needs any additional clarity.

lets clarify that this still does not guarantee as the TelemetryHostedService may not have done initializing while other hosted services start, so possible to still miss telemetry until its fully initialized

I didn't dig into older versions but the current code seems to await each IHostedService.StartAsync one-by-one so this new logic of inserting at 0 should actually work really well. The only case where it wouldn't work (that I can think of) is if user inserted their service at index 0 after we stuck our service there 😄

got it. The readme can callout this as well. (that the only time we'll miss telemetry from a hosted service is if user manually inserts its ahead of ours)

Note, that in .NET 8 hosted service can be configured to start concurrently with HostOptions.ServicesStartConcurrently = true

codecov · 2023-09-25T15:10:21Z

Codecov Report

Merging #4883 (58a7a11) into main (f63e6e5) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 58a7a11 differs from pull request most recent head d402991. Consider uploading reports for the commit d402991 to get more accurate results

@@            Coverage Diff             @@
##             main    #4883      +/-   ##
==========================================
- Coverage   83.26%   83.26%   -0.01%     
==========================================
  Files         295      295              
  Lines       12294    12294              
==========================================
- Hits        10237    10236       -1     
- Misses       2057     2058       +1

Flag	Coverage Δ
unittests	`83.26% <100.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...ensions.Hosting/OpenTelemetryServicesExtensions.cs	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

test/OpenTelemetry.Extensions.Hosting.Tests/OpenTelemetryServicesExtensionsTests.cs

cijothomas · 2023-09-25T22:44:45Z

src/OpenTelemetry.Extensions.Hosting/OpenTelemetryServicesExtensions.cs

-    /// cref="IServiceCollection"/>.
+    /// cref="IServiceCollection"/>.</item>
+    /// <item>OpenTelemetry SDK services are inserted at the beginning of the
+    /// <see cref="IServiceCollection"/> in an attempt to provide a better


Suggested change

/// <see cref="IServiceCollection"/> in an attempt to provide a better

/// <see cref="IServiceCollection"/> in an attempt to ensure telemetry generated from other services are also collected, but this is not guranteed.

Seems hard to capture all the nuance here. Maybe we should just link to a doc?

/// <item>OpenTelemetry SDK services are inserted at the beginning of the <see cref="IServiceCollection"/> and started with the host. For details about the ordering of events and capturing telemetry in <see cref="IHostedService" />s see: <see href="https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Extensions.Hosting/README.md#hosted-service-ordering-and-telemetry-capture" />.

If we are going to put something in the README then I think including a link here makes the most sense. One potential downside is that the link could break.

@CodeBlanch @cijothomas can we come to an agreement on the preferred documentation style? Just don't want this to sit stale.

My vote would be... do the link as I have it above and just stub out the anchor section in the README with "TBD" as the content. We can flush it out on a follow-up.

cijothomas

LGTM. Not a blocker - but this is worth mentioning about in the readme page of ext.hosting package?

cijothomas · 2023-09-25T22:47:04Z

src/OpenTelemetry.Extensions.Hosting/OpenTelemetryServicesExtensions.cs

-    /// Note: This is safe to be called multiple times and by library authors.
+    /// Notes:
+    /// <list type="bullet">
+    /// <item>This is safe to be called multiple times and by library authors.


not something added in this PR, but this is not to be called by library authors....so must be a typo here? @CodeBlanch do you think this was intended?

There's really 2 questions...

Q: Is it safe for library authors to call this?

A: Yes! Multiple calls will stack. Everything is building-up/modifying a single provider for any given IServiceCollection. This is what the comments are saying.

Q: Should library authors call this?

A: It depends! If you want to force OpenTelemetry to be started, perfectly fine to call this. If you just want to configure something in the event OTel happens to be used by the host, don't call this. That is what IServiceCollection.ConfigureOpenTelemetry[Tracer|Meter|Logger]Provider extensions are for. The current comments aren't really addressing that bit, I'll follow-up on that.

Register opentelemetry as the first hosted service

4c02e55

danelson requested a review from a team as a code owner September 25, 2023 14:56

danelson commented Sep 25, 2023

View reviewed changes

Fix line endings

c3421f9

cijothomas reviewed Sep 25, 2023

View reviewed changes

test/OpenTelemetry.Extensions.Hosting.Tests/OpenTelemetryServicesExtensionsTests.cs Show resolved Hide resolved

Update changelog; Add remarks

89e1785

danelson requested review from CodeBlanch and cijothomas September 25, 2023 18:37

cijothomas reviewed Sep 25, 2023

View reviewed changes

cijothomas approved these changes Sep 25, 2023

View reviewed changes

cijothomas reviewed Sep 25, 2023

View reviewed changes

CodeBlanch added the pkg:OpenTelemetry.Extensions.Hosting Issues related to OpenTelemetry.Extensions.Hosting NuGet package label Sep 27, 2023

Update docs for AddOpenTelemetry

58a7a11

CodeBlanch changed the title ~~Register OpenTelemetry at the beginning of IServiceCollection~~ [hosting] Register OpenTelemetry at the beginning of IServiceCollection Oct 5, 2023

Merge branch 'main' into hosted-service-order

d402991

CodeBlanch merged commit 3e885c7 into open-telemetry:main Oct 5, 2023
67 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[hosting] Register OpenTelemetry at the beginning of IServiceCollection #4883

[hosting] Register OpenTelemetry at the beginning of IServiceCollection #4883

danelson commented Sep 25, 2023 •

edited

danelson Sep 25, 2023

CodeBlanch Sep 25, 2023

cijothomas Sep 25, 2023

cijothomas Sep 25, 2023

danelson Sep 25, 2023

CodeBlanch Sep 25, 2023

cijothomas Sep 26, 2023

gao-artur Dec 17, 2023

codecov bot commented Sep 25, 2023 •

edited

cijothomas Sep 25, 2023

CodeBlanch Sep 26, 2023

danelson Sep 26, 2023

danelson Oct 2, 2023

CodeBlanch Oct 2, 2023

cijothomas left a comment

cijothomas Sep 25, 2023

CodeBlanch Sep 26, 2023

	/// <see cref="IServiceCollection"/> in an attempt to provide a better
	/// <see cref="IServiceCollection"/> in an attempt to ensure telemetry generated from other services are also collected, but this is not guranteed.

[hosting] Register OpenTelemetry at the beginning of IServiceCollection #4883

[hosting] Register OpenTelemetry at the beginning of IServiceCollection #4883

Conversation

danelson commented Sep 25, 2023 • edited

Changes

Merge requirement checklist

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

codecov bot commented Sep 25, 2023 • edited

Codecov Report

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danelson commented Sep 25, 2023 •

edited

codecov bot commented Sep 25, 2023 •

edited