Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Oct 5, 2023
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4c02e55
Register opentelemetry as the first hosted service
danelson Sep 24, 2023
c3421f9
Fix line endings
danelson Sep 25, 2023
89e1785
Update changelog; Add remarks
danelson Sep 25, 2023
58a7a11
Update docs for AddOpenTelemetry
danelson Oct 4, 2023
d402991
Merge branch 'main' into hosted-service-order
CodeBlanch Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/OpenTelemetry.Extensions.Hosting/OpenTelemetryServicesExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
// limitations under the License.
// </copyright>

using Microsoft.Extensions.DependencyInjection.Extensions;
using Microsoft.Extensions.Hosting;
using OpenTelemetry;
using OpenTelemetry.Extensions.Hosting.Implementation;
Expand Down Expand Up @@ -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)))
Copy link
Contributor Author

@danelson danelson Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@CodeBlanch CodeBlanch Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Member

@cijothomas cijothomas Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@cijothomas cijothomas Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@danelson danelson Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@CodeBlanch CodeBlanch Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Member

@cijothomas cijothomas Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link

@gao-artur gao-artur Dec 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
services.Insert(0, ServiceDescriptor.Singleton<IHostedService, TelemetryHostedService>());
}

return new(services);
}
Expand Down
40 changes: 40 additions & 0 deletions test/OpenTelemetry.Extensions.Hosting.Tests/OpenTelemetryServicesExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// limitations under the License.
// </copyright>

using System.Diagnostics;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
Expand Down Expand Up @@ -450,9 +451,48 @@ public void AddOpenTelemetry_WithLogging_NestedResolutionUsingConfigureTest()
Assert.True(innerTestExecuted);
}

[Fact]
public async Task AddOpenTelemetry_HostedServiceOrder_DoesNotMatter()
{
var exportedItems = new List<Activity>();

var builder = new HostBuilder().ConfigureServices(services =>
{
services.AddHostedService<TestHostedService>();
services.AddOpenTelemetry()
.WithTracing(builder =>
{
builder.SetSampler(new AlwaysOnSampler());
builder.AddSource(nameof(TestHostedService));
builder.AddInMemoryExporter(exportedItems);
});
});

var host = builder.Build();
await host.StartAsync().ConfigureAwait(false);
await host.StopAsync().ConfigureAwait(false);
host.Dispose();

Assert.Single(exportedItems);
}

private sealed class MySampler : Sampler
{
public override SamplingResult ShouldSample(in SamplingParameters samplingParameters)
=> new(SamplingDecision.RecordAndSample);
}

private sealed class TestHostedService : BackgroundService
{
private readonly ActivitySource activitySource = new ActivitySource(nameof(TestHostedService));

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

return Task.CompletedTask;
}
}
}