-
Notifications
You must be signed in to change notification settings - Fork 857
[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.
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
Changes from all commits
4c02e55
c3421f9
89e1785
58a7a11
d402991
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -35,10 +34,17 @@ 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. | ||
| /// 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"/> 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" />.</item> | ||
| /// </list> | ||
| /// </remarks> | ||
| /// <param name="services"><see cref="IServiceCollection"/>.</param> | ||
| /// <returns>The supplied <see cref="OpenTelemetryBuilder"/> for chaining | ||
|
|
@@ -47,8 +53,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))) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the entire
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me know if you think the wording needs any additional clarity.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 😄
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| { | ||
| services.Insert(0, ServiceDescriptor.Singleton<IHostedService, TelemetryHostedService>()); | ||
| } | ||
|
|
||
| return new(services); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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]Providerextensions are for. The current comments aren't really addressing that bit, I'll follow-up on that.