Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
8 changes: 8 additions & 0 deletions src/OpenTelemetry.Extensions.Hosting/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@

## Unreleased

* Changed the behavior of the `OpenTelemetryBuilder.AddOpenTelemetry` extension
to INSERT OpenTelemetry services at the beginning of the `IServiceCollection`
in an attempt to provide a better experience for end users capturing telemetry
in hosted services. Note that this does not guarantee that OpenTelemetry
services will be initialized while other hosted services start, so it is
possible to miss telemetry until OpenTelemetry services are fully initialized.
([#4883](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4883))

## 1.6.0

Released 2023-Sep-05
Expand Down
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 All @@ -35,10 +34,16 @@ public static class OpenTelemetryServicesExtensions
/// cref="IServiceCollection"/>.
/// </summary>
/// <remarks>
/// 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.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

/// Only a single <see cref="TracerProvider"/> and/or <see
/// cref="MeterProvider"/> will be created for a given <see
/// 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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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" />.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

/// experience for end users capturing telemetry in hosted services.</item>
/// </list>
/// </remarks>
/// <param name="services"><see cref="IServiceCollection"/>.</param>
/// <returns>The supplied <see cref="OpenTelemetryBuilder"/> for chaining
Expand All @@ -47,8 +52,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

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

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

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

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

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

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

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)

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
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;
}
}
}