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

Dependency injection support #1889

Merged
merged 19 commits into from
Mar 26, 2021
Merged
Show file tree
Hide file tree
Changes from 8 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
17 changes: 7 additions & 10 deletions examples/AspNetCore/Startup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.OpenApi.Models;
using OpenTelemetry;
using OpenTelemetry.Exporter;
using OpenTelemetry.Resources;
using OpenTelemetry.Trace;

Expand Down Expand Up @@ -63,20 +63,17 @@ public void ConfigureServices(IServiceCollection services)
.SetResourceBuilder(ResourceBuilder.CreateDefault().AddService(this.Configuration.GetValue<string>("Jaeger:ServiceName")))
.AddAspNetCoreInstrumentation()
.AddHttpClientInstrumentation()
.AddJaegerExporter(jaegerOptions =>
{
jaegerOptions.AgentHost = this.Configuration.GetValue<string>("Jaeger:Host");
jaegerOptions.AgentPort = this.Configuration.GetValue<int>("Jaeger:Port");
}));
.AddJaegerExporter());

services.Configure<JaegerExporterOptions>(this.Configuration.GetSection("Jaeger"));
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved
break;
case "zipkin":
services.AddOpenTelemetryTracing((builder) => builder
.AddAspNetCoreInstrumentation()
.AddHttpClientInstrumentation()
.AddZipkinExporter(zipkinOptions =>
{
zipkinOptions.Endpoint = new Uri(this.Configuration.GetValue<string>("Zipkin:Endpoint"));
}));
.AddZipkinExporter());

services.Configure<ZipkinExporterOptions>(this.Configuration.GetSection("Zipkin"));
break;
case "otlp":
// Adding the OtlpExporter creates a GrpcChannel.
Expand Down
4 changes: 2 additions & 2 deletions examples/AspNetCore/appsettings.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
"UseExporter": "console",
"Jaeger": {
"ServiceName": "jaeger-test",
"Host": "localhost",
"Port": 6831
"AgentHost": "localhost",
"AgentPort": 6831
},
"Zipkin": {
"ServiceName": "zipkin-test",
Expand Down
4 changes: 0 additions & 4 deletions src/OpenTelemetry.Api/Trace/TracerProviderBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,6 @@ namespace OpenTelemetry.Trace
/// </summary>
public abstract class TracerProviderBuilder
{
internal TracerProviderBuilder()
{
}

/// <summary>
/// Adds an instrumentation to the provider.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,22 +38,35 @@ public static TracerProviderBuilder AddJaegerExporter(this TracerProviderBuilder
throw new ArgumentNullException(nameof(builder));
}

var exporterOptions = new JaegerExporterOptions();
configure?.Invoke(exporterOptions);
var jaegerExporter = new JaegerExporter(exporterOptions);
if (builder is IDeferredTracerBuilder deferredTracerBuilder)
{
return deferredTracerBuilder.Configure((sp, builder) =>
{
AddJaegerExporter(builder, sp.GetOptions<JaegerExporterOptions>(), configure);
});
}

return AddJaegerExporter(builder, new JaegerExporterOptions(), configure);
}

private static TracerProviderBuilder AddJaegerExporter(TracerProviderBuilder builder, JaegerExporterOptions options, Action<JaegerExporterOptions> configure = null)
{
configure?.Invoke(options);

var jaegerExporter = new JaegerExporter(options);

if (exporterOptions.ExportProcessorType == ExportProcessorType.Simple)
if (options.ExportProcessorType == ExportProcessorType.Simple)
{
return builder.AddProcessor(new SimpleActivityExportProcessor(jaegerExporter));
}
else
{
return builder.AddProcessor(new BatchActivityExportProcessor(
jaegerExporter,
exporterOptions.BatchExportProcessorOptions.MaxQueueSize,
exporterOptions.BatchExportProcessorOptions.ScheduledDelayMilliseconds,
exporterOptions.BatchExportProcessorOptions.ExporterTimeoutMilliseconds,
exporterOptions.BatchExportProcessorOptions.MaxExportBatchSize));
options.BatchExportProcessorOptions.MaxQueueSize,
options.BatchExportProcessorOptions.ScheduledDelayMilliseconds,
options.BatchExportProcessorOptions.ExporterTimeoutMilliseconds,
options.BatchExportProcessorOptions.MaxExportBatchSize));
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,22 +38,35 @@ public static TracerProviderBuilder AddZipkinExporter(this TracerProviderBuilder
throw new ArgumentNullException(nameof(builder));
}

var exporterOptions = new ZipkinExporterOptions();
configure?.Invoke(exporterOptions);
var zipkinExporter = new ZipkinExporter(exporterOptions);
if (builder is IDeferredTracerBuilder deferredTracerBuilder)
{
return deferredTracerBuilder.Configure((sp, builder) =>
{
AddZipkinExporter(builder, sp.GetOptions<ZipkinExporterOptions>(), configure);
});
}

return AddZipkinExporter(builder, new ZipkinExporterOptions(), configure);
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally don't like how this approach is causing all exporters and instrumentations to have to type check the builder and then have different code paths. IMO, it shouldn't be optional for these exporters and instrumentations to support separating configuration and construction phases. It should be mandatory that they register things with a deferred approach so that we can keep configuration and construction phases cleanly separated.

Copy link
Contributor

@ejsmith ejsmith Mar 14, 2021

Choose a reason for hiding this comment

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

I would personally prefer if we could force all of them to be registered with lambdas and not even make it an option to immediately new them up when configuring. Using a factory method approach for configuration is compatible with all targeted platforms without taking a dependency on Microsoft.Extensions.DependencyInjection for the core library while still allowing a DI approach to be implemented in a platform that supports it like the hosting package.

Copy link
Member Author

Choose a reason for hiding this comment

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

Of everything on this PR, I'm the least happy with this bit. But I don't think it is possible to do it any other way 😭 It's not really a problem of separating configuration and construction, this is about the inversion of dependencies we've created for ourselves. To do it "properly" we would need this ctor: public ZipkinExporter(IOptions<ZipkinExporterOptions> options). But that of course requires a direct dependency.

}

private static TracerProviderBuilder AddZipkinExporter(TracerProviderBuilder builder, ZipkinExporterOptions options, Action<ZipkinExporterOptions> configure = null)
{
configure?.Invoke(options);

var zipkinExporter = new ZipkinExporter(options);

if (exporterOptions.ExportProcessorType == ExportProcessorType.Simple)
if (options.ExportProcessorType == ExportProcessorType.Simple)
{
return builder.AddProcessor(new SimpleActivityExportProcessor(zipkinExporter));
}
else
{
return builder.AddProcessor(new BatchActivityExportProcessor(
zipkinExporter,
exporterOptions.BatchExportProcessorOptions.MaxQueueSize,
exporterOptions.BatchExportProcessorOptions.ScheduledDelayMilliseconds,
exporterOptions.BatchExportProcessorOptions.ExporterTimeoutMilliseconds,
exporterOptions.BatchExportProcessorOptions.MaxExportBatchSize));
options.BatchExportProcessorOptions.MaxQueueSize,
options.BatchExportProcessorOptions.ScheduledDelayMilliseconds,
options.BatchExportProcessorOptions.ExporterTimeoutMilliseconds,
options.BatchExportProcessorOptions.MaxExportBatchSize));
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
// <copyright file="TracerProviderBuilderHosting.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using System.Collections.Generic;
using System.Diagnostics;
using Microsoft.Extensions.DependencyInjection;

namespace OpenTelemetry.Trace
{
public class TracerProviderBuilderHosting : TracerProviderBuilderSdk, IDeferredTracerBuilder
{
private readonly List<InstrumentationFactory> instrumentationFactories = new List<InstrumentationFactory>();
private readonly List<Type> processorTypes = new List<Type>();
private readonly List<Action<IServiceProvider, TracerProviderBuilder>> configurationActions = new List<Action<IServiceProvider, TracerProviderBuilder>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I like having generic configuration actions like this.

private Type samplerType;

internal TracerProviderBuilderHosting(IServiceCollection services)
{
this.Services = services ?? throw new ArgumentNullException(nameof(services));
}

public IServiceCollection Services { get; }

TracerProviderBuilder IDeferredTracerBuilder.Configure(Action<IServiceProvider, TracerProviderBuilder> configure)
{
if (configure == null)
{
throw new ArgumentNullException(nameof(configure));
}

this.configurationActions.Add(configure);
return this;
}

internal TracerProviderBuilder AddInstrumentation<TInstrumentation>(
Func<IServiceProvider, TInstrumentation> instrumentationFactory)
where TInstrumentation : class
{
if (instrumentationFactory == null)
{
throw new ArgumentNullException(nameof(instrumentationFactory));
}

this.instrumentationFactories.Add(
new InstrumentationFactory(
typeof(TInstrumentation).Name,
"semver:" + typeof(TInstrumentation).Assembly.GetName().Version,
typeof(TInstrumentation)));

return this;
}

internal TracerProviderBuilder AddProcessor<T>()
where T : BaseProcessor<Activity>
{
this.processorTypes.Add(typeof(T));
return this;
}

internal TracerProviderBuilder SetSampler<T>()
where T : Sampler
{
this.samplerType = typeof(T);
return this;
}

internal TracerProvider Build(IServiceProvider serviceProvider)
{
foreach (InstrumentationFactory instrumentationFactory in this.instrumentationFactories)
{
this.AddInstrumentation(
instrumentationFactory.Name,
instrumentationFactory.Version,
() => serviceProvider.GetRequiredService(instrumentationFactory.Type));
}

foreach (Type processorType in this.processorTypes)
{
this.AddProcessor((BaseProcessor<Activity>)serviceProvider.GetRequiredService(processorType));
}

if (this.samplerType != null)
{
this.SetSampler((Sampler)serviceProvider.GetRequiredService(this.samplerType));
}

foreach (Action<IServiceProvider, TracerProviderBuilder> configureAction in this.configurationActions)
{
configureAction(serviceProvider, this);
}

return ((TracerProviderBuilderSdk)this).Build();
}

internal readonly struct InstrumentationFactory
{
public readonly string Name;
public readonly string Version;
public readonly Type Type;

internal InstrumentationFactory(string name, string version, Type type)
{
this.Name = name;
this.Version = version;
this.Type = type;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@
// </copyright>

using System;
using System.Collections.Generic;
using Microsoft.Extensions.DependencyInjection.Extensions;
using Microsoft.Extensions.Hosting;
using OpenTelemetry;
using OpenTelemetry.Extensions.Hosting.Implementation;
using OpenTelemetry.Trace;

Expand All @@ -35,8 +35,7 @@ public static class OpenTelemetryServicesExtensions
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
public static IServiceCollection AddOpenTelemetryTracing(this IServiceCollection services)
{
services.AddOpenTelemetryTracing(builder => { });
return services;
return services.AddOpenTelemetryTracing(builder => { });
}

/// <summary>
Expand All @@ -52,63 +51,9 @@ public static IServiceCollection AddOpenTelemetryTracing(this IServiceCollection
throw new ArgumentNullException(nameof(configure));
}

var builder = Sdk.CreateTracerProviderBuilder();
var builder = new TracerProviderBuilderHosting(services);
configure(builder);
services.AddOpenTelemetryTracing(() => builder.Build());
return services;
}

/// <summary>
/// Adds OpenTelemetry TracerProvider to the specified <see cref="IServiceCollection" />.
/// </summary>
/// <param name="services">The <see cref="IServiceCollection" /> to add services to.</param>
/// <param name="configure">The <see cref="TracerProviderBuilder"/> action to configure TracerProviderBuilder.</param>
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
public static IServiceCollection AddOpenTelemetryTracing(this IServiceCollection services, Action<IServiceProvider, TracerProviderBuilder> configure)
{
if (configure is null)
{
throw new ArgumentNullException(nameof(configure));
}

var builder = Sdk.CreateTracerProviderBuilder();
services.AddOpenTelemetryTracing((sp) =>
{
configure(sp, builder);
return builder.Build();
});
return services;
}

/// <summary>
/// Adds OpenTelemetry TracerProvider to the specified <see cref="IServiceCollection" />.
/// </summary>
/// <param name="services">The <see cref="IServiceCollection" /> to add services to.</param>
/// <param name="createTracerProvider">A delegate that provides the tracer provider to be registered.</param>
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
private static IServiceCollection AddOpenTelemetryTracing(this IServiceCollection services, Func<TracerProvider> createTracerProvider)
{
if (services is null)
{
throw new ArgumentNullException(nameof(services));
}

if (createTracerProvider is null)
{
throw new ArgumentNullException(nameof(createTracerProvider));
}

try
{
services.AddSingleton(s => createTracerProvider());
AddOpenTelemetryTracingInternal(services);
}
catch (Exception ex)
{
HostingExtensionsEventSource.Log.FailedInitialize(ex);
}

return services;
return services.AddOpenTelemetryTracing(sp => builder.Build(sp));
}

/// <summary>
Expand All @@ -131,8 +76,9 @@ private static IServiceCollection AddOpenTelemetryTracing(this IServiceCollectio

try
{
services.AddSingleton(s => createTracerProvider(s));
AddOpenTelemetryTracingInternal(services);
return services
.AddSingleton(s => createTracerProvider(s))
.AddOpenTelemetryTracingInternal();
}
catch (Exception ex)
{
Expand All @@ -142,9 +88,10 @@ private static IServiceCollection AddOpenTelemetryTracing(this IServiceCollectio
return services;
}

private static void AddOpenTelemetryTracingInternal(IServiceCollection services)
private static IServiceCollection AddOpenTelemetryTracingInternal(this IServiceCollection services)
{
services.TryAddEnumerable(ServiceDescriptor.Singleton<IHostedService, TelemetryHostedService>());
return services;
}
}
}
Loading