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

Question: Change namespace of extension methods? #1541

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Nov 14, 2020

I'm putting this out here to pose a question regarding the namespaces we've chosen for some of our extension methods. We may very well not want to adopt any of the changes I've made here.

Background

We have many extension methods off of TracerBuilderProvider. Extension methods are great, but often can be a little tricky for a newcomer to a library since in some contexts they'll need to make sure they add the appropriate using statements.

For example, in the context of an ASP.NET Core app one might write the following:

using Microsoft.Extensions.Hosting;

...

services.AddOpenTelemetryTracing((builder) => // no additional using here since this is an extension on a non-otel type and is in the Microsoft.Extensions.Hosting namespace
    builder
        .AddSource("my-source") // no additional using needed here since AddSource is not an extension on TracerProviderBuilder
        .AddAspNetCoreInstrumentation() // this requires using OpenTelemetry.Trace
        .AddZipkinExporter() // also requires OpenTelemetry.Trace
);


// In the future we will probably have the following

services.AddOpenTelemetryMetrics((builder) =>
    builder
        .AddAspNetCoreInstrumentation() // this requires using OpenTelemetry.Metrics
        .AddPrometheus() // also requires OpenTelemetry.Metrics
);

...

This example code requires the user to know to add both using OpenTelemetry.Trace and using OpenTelemetry.Metrics to expose the necessary extension methods.

Question

Does it make sense to keep extension methods for OpenTelemetry types (i.e., TracerProviderBuilder, MeterProviderBuilder, etc) in a single namespace like OpenTelemetry or OpenTelemetry.Extensions? Adding a using statement would still be required, but the benefit would be that a single using statement would make all extension methods available.

The change here is in the context of the Zipkin and Jaeger exporters, but as shown, this question applies to our extension methods defined in instrumentation projects as well. Also, note that this would be a small breaking change.

@alanwest alanwest added question Further information is requested pr:do-not-merge labels Nov 14, 2020
@alanwest alanwest changed the title Change namespace of extension methods to OpenTelemetry Question: Change namespace of extension methods? Nov 14, 2020
@cijothomas
Copy link
Member

Cant think of the "perfect" solution here.
Making exporters and all other extensions methods in the namespace OpenTelemtry, makes it easy to write code with just one import.
Separating OpenTelemetry.Trace, OpenTelemetry.Metric allows clean separation between 2 distinct concepts.

Want to hear more opinions on this.

@alanwest
Copy link
Member Author

Cant think of the "perfect" solution here.

Yes, I'm finding this to be true, as well. I do not have strong opinions here, but I do lean towards making extension methods as easy to discover as possible. To me, it's a bit of a usability concern when I'm learning a new library and copy and paste some sample code only to find that the code uses an extension method and I'm left wondering either what namespace I need to include or what package I'm missing. Though, I suppose we've done a pretty good job of including the appropriate usings in our sample code.

Most important to me is that we have clear guidance so that we can remain consistent with whatever we decide. I've noticed we have a few different flavors for handling extension methods:

Flavor #1: Extension on an OTel SDK type

At the moment, we're mostly putting extension methods for OTel SDK types in the same namespace as the type they are extending. This make sense organizationally, though as demonstrated in this PR, it may be common that code is implicitly using an OTel type (i.e., TracerProviderBuilder) without an explicit using of the OpenTelemetry.Trace namespace.

Flavor #2: Extension on a non-OTel SDK type

I think it makes sense to follow whatever precedence already exists for extension methods on these types, and from what I can tell we are following this. For example, the extension methods on IServiceCollection and ILoggerBuilder are in the
Microsoft.Extensions.DependencyInjection and Microsoft.Extensions.Logging, respectively, because this is the common pattern for extending these types.

Flavor #3: Extension on an OTel SDK type, but treating it as though it was non-OTel

I've found one example of this - it may have been unintentional...

namespace Microsoft.Extensions.Logging
{
public static class InMemoryExporterLoggingExtensions
{
public static OpenTelemetryLoggerOptions AddInMemoryExporter(this OpenTelemetryLoggerOptions loggerOptions, Action<InMemoryExporterOptions> configure = null)

@alanwest
Copy link
Member Author

Closing in favor of #1576

@alanwest alanwest closed this Nov 18, 2020
@alanwest alanwest deleted the alanwest/extension-method-namespace branch March 9, 2021 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants