Skip to content
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

Default host behavior for OpenTelemetryBuilder With* extensions #4985

Closed
CodeBlanch opened this issue Oct 24, 2023 · 2 comments · Fixed by #5072
Closed

Default host behavior for OpenTelemetryBuilder With* extensions #4985

CodeBlanch opened this issue Oct 24, 2023 · 2 comments · Fixed by #5072
Labels
pkg:OpenTelemetry.Extensions.Hosting Issues related to OpenTelemetry.Extensions.Hosting NuGet package
Milestone

Comments

@CodeBlanch
Copy link
Member

CodeBlanch commented Oct 24, 2023

Related to #4896

See: #4958 (comment)

Currently calling WithMetrics or WithLogging does NOT enable integration with the hosting pipeline (ILoggerFactory or IMetricsListener).

Based on discussions with @noahfalk & @samsp-msft it sounds like we want to enable those by default perhaps giving users a way to opt-out. Opening this issue to track that effort.

Updates...

Metrics: #4958 changes WithMetrics to register the IMetricsListener
Logging: #5072 changes WithLogging to perform services.AddLogging(builder => builder.UseOpenTelemetry())

@CodeBlanch CodeBlanch added the pkg:OpenTelemetry.Extensions.Hosting Issues related to OpenTelemetry.Extensions.Hosting NuGet package label Oct 24, 2023
@CodeBlanch CodeBlanch added this to the 1.7.0 milestone Oct 24, 2023
@martinjt
Copy link
Member

martinjt commented Nov 8, 2023

I think this is an interesting approach, however, a core part of the OpenTelemetry ecosystem is adding instrumentation libraries and opting in. What we're doing here is predicating this on the fact that AspNetCore exists, and therefore adding Meter's for that, or some other things.

Right now, you need to add OpenTelemetry.Instrumentation.AspNetCore as a package AND add it to the pipeline to enable it. This seems like we're suggesting to remove those hops in favour of "if you add .WithMetrics() you'll get the defaults".

I'm not sure that's the approach we should take here, especially not in a minor release. As soon as someone updates their app, they're going to get a tonne of new metric datapoints, and if they'd not added AspNetCore, process etc. before due to, say, cost at the backend side, they're suddenly going to blow that out.

Logging is more interesting, and I'm not sure about that one, but metrics I don't think we should be adding listeners on anything until someone has opted in.

@CodeBlanch
Copy link
Member Author

@martinjt

Slight clarification. What we're talking about here is enabling the integration, not turning on or listening to anything. If a user calls services.AddOpenTelemetry().WithMetrics(builder => builder.AddOtlpExporter()) they won't see any metrics unless they take further action. That action could be...

  • Calling AddMeter to listen via the SDK.
  • Add some instrumentation (ex builder.AddAspNetCoreInstrumentation()) to the builder (generally it calls AddMeter for you) to listen via the SDK.
  • Adding configuration under "Metrics" to listen via IMetricsListener (new .NET 8 stuff).
  • Calling services.AddMetrics(builder => builder.EnableMetrics(...)) to listen via IMetricsListener (new .NET 8 stuff).

Essentially the user still has to opt-in to everything they want to listen to. Does that help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry.Extensions.Hosting Issues related to OpenTelemetry.Extensions.Hosting NuGet package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants