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
CodeBlanch 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 1 commit
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
Prev Previous commit
Next Next commit
Update changelog; Add remarks
  • Loading branch information
@danelson
danelson committed Sep 25, 2023
commit 89e1785b9dd821d93aa3e6cad8c57149946bc845
8 changes: 8 additions & 0 deletions src/OpenTelemetry.Extensions.Hosting/CHANGELOG.md
View file
Open in desktop
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
10 changes: 8 additions & 2 deletions src/OpenTelemetry.Extensions.Hosting/OpenTelemetryServicesExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,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

@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.

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

@CodeBlanch CodeBlanch Sep 26, 2023

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

@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.

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

@CodeBlanch CodeBlanch Sep 26, 2023

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

@danelson danelson Sep 26, 2023

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

@danelson danelson Oct 2, 2023

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

@CodeBlanch CodeBlanch Oct 2, 2023

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 Down