Add OpenTelemetry.Hosting #253

JamesNK · 2019-10-02T04:32:33Z

Fixes #241

This PR adds a OpenTelemetry.Hosting package for improving integration between OpenTelemetry and DI (could also be named OpenTelemetry.DependencyInjection)

I've kept the DI dependency isolated to this package. The user experience could be improved with strongly typed registration methods, e.g. AddZipkin(options), but that would require either additional projects depending on DI, or more packages for DI integration, e.g. OpenTelemetry.Exporter.Zipkin.DependencyInjection.

To start collectors I have an IHostedService registered for each. It will automatically start and stop collectors, avoiding the current hack of users manually resolving services somewhere to ensure they are created. Generic host experts: It feels like I'm abusing what an IHostedService is for to inject start and stop logic via DI. Let me know if there is a better way of doing this. @davidfowl @glennc @pakrym

After:

services.AddOpenTelemetry(telemetry =>
{
    telemetry.AddCollector<RequestsCollector>(new RequestsCollectorOptions());
    telemetry.AddCollector<DependenciesCollector>(new DependenciesCollectorOptions());
    telemetry.SetTracer<Tracer, TraceConfig>();
    telemetry.SetSpanExporter<ZipkinTraceExporter>(new ZipkinTraceExporterOptions { ServiceName = "my-service" });
});

Before:

services.AddSingleton<ISampler>(Samplers.AlwaysSample);
services.AddSingleton<ZipkinTraceExporterOptions>(_ => new ZipkinTraceExporterOptions { ServiceName = "my-service" });
services.AddSingleton<SpanExporter, ZipkinTraceExporter>();
services.AddSingleton<SpanProcessor, BatchingSpanProcessor>();
services.AddSingleton<TraceConfig>();
services.AddSingleton<ITracer, Tracer>();

// you may also configure request and dependencies collectors
services.AddSingleton<RequestsCollectorOptions>(new RequestsCollectorOptions());
services.AddSingleton<RequestsCollector>();

services.AddSingleton<DependenciesCollectorOptions>(new DependenciesCollectorOptions());
services.AddSingleton<DependenciesCollector>();

public void Configure(IApplicationBuilder app, RequestsCollector requestsCollector,  DependenciesCollector dependenciesCollector)
{
    // ...
}

JamesNK · 2019-10-02T05:01:59Z

src/OpenTelemetry.Shims.OpenTracing/TracerShim.cs

@@ -52,7 +52,7 @@ private TracerShim(Trace.ITracer tracer)
                throw new ArgumentNullException(nameof(format));
            }

-            if (carrier == default)


VS complained about this 🤷‍♂

It's a bug (only in internal builds) being fixed.

I ended up changing those lines to carrier == null (for now).

davidfowl · 2019-10-02T05:40:09Z

src/OpenTelemetry.Hosting/Implementation/CollectorHostedService.cs

+
+        public Task StopAsync(CancellationToken cancellationToken)
+        {
+            (this.collector as IDisposable)?.Dispose();


Do this in dispose (implement IDisposable)

davidfowl · 2019-10-02T08:15:38Z

This still looks noisy:

services.AddOpenTelemetry(telemetry =>
{
    telemetry.AddCollector<RequestsCollector>(new RequestsCollectorOptions());
    telemetry.AddCollector<DependenciesCollector>(new DependenciesCollectorOptions());
    telemetry.SetTracer<Tracer, TraceConfig>();
    telemetry.SetSpanExporter<ZipkinTraceExporter>(new ZipkinTraceExporterOptions { ServiceName = "my-service" });
});

Is there anyway to get it down to something like:

services.AddOpenTelemetry(telemetry =>
{
    telemetry.AddCollector<RequestsCollector>();
    telemetry.AddCollector<DependenciesCollector>();
    telemetry.SetTracer<Tracer, TraceConfig>();
    telemetry.SetSpanExporter<ZipkinTraceExporter>(o => o.ServiceName  = "my-service");
});

Going further, using extension methods for hide some of the wire up:

services.AddOpenTelemetry(telemetry =>
{
    telemetry.UseRequestsCollector();
    telemetry.UseDependenciesCollector();
    telemetry.SetTracer<Tracer, TraceConfig>(); // What is this?
    telemetry.UseZipkin(o => o.ServiceName  = "my-service");
});

JamesNK · 2019-10-02T08:35:42Z

telemetry.SetTracer<Tracer, TraceConfig>(); // What is this?

It is different because an argument to TraceConfig's constructor is from DI so TraceConfig can't be newed up. Yeah it sucks.

services.AddOpenTelemetry(telemetry =>
{
    telemetry.AddCollector<RequestsCollector>();
    telemetry.AddCollector<DependenciesCollector>();
    telemetry.SetTracer<Tracer, TraceConfig>();
    telemetry.SetSpanExporter<ZipkinTraceExporter>(o => o.ServiceName  = "my-service");
});

Types on all those constructors take the option objects e.g. RequestCollector.ctor(). Is there a way to make them optional so DI doesn't mind if there is no options object?

services.AddOpenTelemetry(telemetry =>
{
    telemetry.UseRequestsCollector();
    telemetry.UseDependenciesCollector();
    telemetry.SetTracer<Tracer, TraceConfig>(); // What is this?
    telemetry.UseZipkin(o => o.ServiceName  = "my-service");
});

Currently there is zero DI integration throughout the rest of the projects. It is POCO objects with values set via constructors. Extension methods are possible, just means making a choice about:

I've kept the DI dependency isolated to OpenTelemetry.Hosting package. The user experience could be improved with strongly typed registration methods, e.g. AddZipkin(options), but that would require either additional projects depending on DI, or more packages for DI integration, e.g. OpenTelemetry.Exporter.Zipkin.DependencyInjection.

I'm guessing there is a design decision at the moment to avoid a dependency on Microsoft.Extensions.DependencyInjection.

pakrym · 2019-10-02T16:12:44Z

First OT needs to decide if they want first-class non-DI tracer setup. Making DI the ultimate composition root would require classes to be designed in a very particular way making non-DI setup prohibitively hard (hello logging).

You can imagine if they have a nice fist class API .Hosting library only has to provide hooks to start/stop collectors and exporters and making tracer resolvable from DI.

OpenTelemetry NO DI

var tracerBuilder = new TracerBuilder();
tracerBuilder.AddRequestCollector(o=> ...);
tracerBuilder.AddDependencyCollector(o=> ...);

tracerBuilder.AddZipkinExporter();

var tracer = tracerBuilder.Build();
await tracer.StartAsync(); // Starts exporters and collectors and whatever else needs explicit starting

Console.WriteLine("Yay");

await tracer.StopAsync();

OpenTelemetry + DI

services.AddOpenTelemetry(tracerBuilder=>
{
	tracerBuilder.AddRequestCollector(o=> ...);
	tracerBuilder.AddDependencyCollector(o=> ...);
	
	tracerBuilder.AddZipkinExporter();
});

// tracerBuilder.Build(), StartAsync/StopAsync is handled for you

lmolkova · 2019-10-02T17:28:42Z

src/OpenTelemetry.Hosting/OpenTelemetryServicesExtensions.cs

+            /// <inheritdoc />
+            public override Task<ExportResult> ExportAsync(IEnumerable<Span> batch, CancellationToken cancellationToken)
+            {
+                return Task.FromResult(ExportResult.Success);


BTW, I know we already have this problem in another place, would you mind caching task.fromresult - this is new Task every time

It should be ValueTask 😄

lmolkova · 2019-10-02T17:36:40Z

It is different because an argument to TraceConfig's constructor is from DI so TraceConfig can't be newed up. Yeah it sucks.

if you can think of how to change TraceConfig to better suit DI scenarios - go for it.

lmolkova · 2019-10-02T17:40:24Z

First OT needs to decide if they want first-class non-DI tracer setup. Making DI the ultimate composition root would require classes to be designed in a very particular way making non-DI setup prohibitively hard (hello logging).

Great idea. OTel configuration is too complicated already and it's going to be even worse with #239. We need something to help configure it without DI too. It is nice to make DI work on top of the same configuration sugar.

[Update] and it doesn't have to happen right now

JamesNK · 2019-10-02T19:36:29Z

I like #253 (comment)

I think the current approach of using DI to build types, but not integrating with DI, will always result in a bad user experience. The AddOpenTelemetry in this PR is probably the best you can get, and it isn't that great.

What I think you should end up with:

Builder types in OpenTelemetry.Abstractions.
Extension methods in the various projects to register with the builder, e.g. Zipkin would have AddZipkinExporter(this ITracingBuilder builder, Func<ZipkinOptions> configure) that would set the exporter on the builder.
Continue to have OpenTelemetry.Hosting for DI integration and automatic start/stop.

lmolkova · 2019-10-03T18:28:02Z

I think the current approach of using DI to build types, but not integrating with DI, will always result in a bad user experience. The AddOpenTelemetry in this PR is probably the best you can get, and it isn't that great.

What I think you should end up with:

@JamesNK Thanks! I'll work on this in the next few days.
In the meantime, Things are changing pretty rapidly and a new thing ( TracerFactory) was introduced in #238 that complicates configuration even further and affects this PR as well. I think we can merge this PR (if you can add support for TracerFactory) or wait for non-DI configuration changes first. Let me know how you want to proceed.

* Rename AzureMonitor Extensions * Add CHANGELOG

Add OpenTelemetry.Hosting

3a0c040

JamesNK requested review from austinlparker, discostu105, lmolkova and SergeyKanzhelev as code owners October 2, 2019 04:32

JamesNK added 2 commits October 2, 2019 17:59

Use in ASP.NET Core logging sample

e286e65

Clean up

71b78b0

JamesNK commented Oct 2, 2019

View reviewed changes

davidfowl reviewed Oct 2, 2019

View reviewed changes

PR feedback

95552f4

lmolkova reviewed Oct 2, 2019

View reviewed changes

lmolkova mentioned this pull request Oct 7, 2019

Configuration for tracing PoC #261

Closed

JamesNK mentioned this pull request Oct 12, 2019

Add OpenTelemetry.Hosting #275

Merged

JamesNK closed this Oct 12, 2019

lmolkova mentioned this pull request Oct 20, 2019

Improve dependency injection registration #241

Closed

Yun-Ting pushed a commit to Yun-Ting/opentelemetry-dotnet that referenced this pull request Oct 13, 2022

Remove "Contrib" from AzureMonitor Extensions (open-telemetry#253)

fa839e7

* Rename AzureMonitor Extensions * Add CHANGELOG

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OpenTelemetry.Hosting #253

Add OpenTelemetry.Hosting #253

JamesNK commented Oct 2, 2019 •

edited

Loading

JamesNK Oct 2, 2019

davidfowl Oct 2, 2019

pcwiese Oct 3, 2019

davidfowl Oct 2, 2019

davidfowl commented Oct 2, 2019

JamesNK commented Oct 2, 2019

pakrym commented Oct 2, 2019 •

edited

Loading

lmolkova Oct 2, 2019

davidfowl Oct 2, 2019

lmolkova commented Oct 2, 2019 •

edited

Loading

lmolkova commented Oct 2, 2019 •

edited

Loading

JamesNK commented Oct 2, 2019 •

edited

Loading

lmolkova commented Oct 3, 2019

Add OpenTelemetry.Hosting #253

Add OpenTelemetry.Hosting #253

Conversation

JamesNK commented Oct 2, 2019 • edited Loading

JamesNK Oct 2, 2019

Choose a reason for hiding this comment

davidfowl Oct 2, 2019

Choose a reason for hiding this comment

pcwiese Oct 3, 2019

Choose a reason for hiding this comment

davidfowl Oct 2, 2019

Choose a reason for hiding this comment

davidfowl commented Oct 2, 2019

JamesNK commented Oct 2, 2019

pakrym commented Oct 2, 2019 • edited Loading

OpenTelemetry NO DI

OpenTelemetry + DI

lmolkova Oct 2, 2019

Choose a reason for hiding this comment

davidfowl Oct 2, 2019

Choose a reason for hiding this comment

lmolkova commented Oct 2, 2019 • edited Loading

lmolkova commented Oct 2, 2019 • edited Loading

JamesNK commented Oct 2, 2019 • edited Loading

lmolkova commented Oct 3, 2019

JamesNK commented Oct 2, 2019 •

edited

Loading

pakrym commented Oct 2, 2019 •

edited

Loading

lmolkova commented Oct 2, 2019 •

edited

Loading

lmolkova commented Oct 2, 2019 •

edited

Loading

JamesNK commented Oct 2, 2019 •

edited

Loading