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

Moving IDeferredTracerProviderBuilder to API #2100

Merged

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Jun 23, 2021

Related to #1889, #2058

Changes

Moved IDeferredTracerProviderBuilder from SDK to API. The goal is to allow instrumentation libraries to be written without a dependency on SDK.

Public API

API doesn't have a dependency on Microsoft.Extensions.DependencyInjection so IDeferredTracerProviderBuilder can no longer expose IServiceCollection.

// Moved from OpenTelemetry to OpenTelemetry.Api
namespace OpenTelemetry.Trace
{
    public interface IDeferredTracerProviderBuilder
    {
        // removed
        // IServiceCollection Services { get; }

        // existing
        TracerProviderBuilder Configure(Action<IServiceProvider, TracerProviderBuilder> configure);
    }
}

// Added to OpenTelemetry.Extensions.Hosting
namespace OpenTelemetry.Trace
{
    public static class TracerProviderBuilderExtensions
    {
       public static IServiceCollection GetServices(this TracerProviderBuilder tracerProviderBuilder) {}
    }
}

TODOs

  • CHANGELOG.md updated for non-trivial changes
  • Changes in public API reviewed

@CodeBlanch CodeBlanch requested a review from a team June 23, 2021 21:36
@codecov
Copy link

codecov bot commented Jun 23, 2021

Codecov Report

Merging #2100 (8900722) into main (b6d4e88) will increase coverage by 0.02%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2100      +/-   ##
==========================================
+ Coverage   85.10%   85.12%   +0.02%     
==========================================
  Files         187      187              
  Lines        6075     6078       +3     
==========================================
+ Hits         5170     5174       +4     
+ Misses        905      904       -1     
Impacted Files Coverage Δ
...s.Hosting/Trace/TracerProviderBuilderExtensions.cs 73.91% <66.66%> (-1.09%) ⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 96.12% <0.00%> (+0.77%) ⬆️
...ing/Implementation/TracerProviderBuilderHosting.cs 92.30% <0.00%> (+7.69%) ⬆️

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM!

@cijothomas
Copy link
Member

Merging quickly as this was discussed in the SIG meetings, and this is required for 1.0.0-beta5 release, (and to unblock 1.1.0 as well).
If there are any additional feedback on this, please leave a comment! Thanks.

@cijothomas cijothomas merged commit e481492 into open-telemetry:main Jun 24, 2021
@CodeBlanch CodeBlanch deleted the api-deferredrracerproviderbuilder2 branch June 24, 2021 16:33
@danielcweber
Copy link

danielcweber commented Jan 8, 2022

Would you be open to having an interface, let's say IFancyDeferredTracerProviderBuilder that would live in the SDK and again expose IServiceCollection? Code with a reference to the SDK could type check TracerProviderBuilder on the interface (like it's often done today) to get a hold of an IServiceCollection. Background: I'm instrumenting some stuff, and mostly it could be done by decorating existing DI-registrations....but no way to do it in one place (by a single extension method of TracerProviderBuilder since, well, no IServiceCollection). I'm puzzled that all the instrumentations can easily live without decorators.

EDIT: I'm referring to TracerProviderBuilderHosting that would eventually implement the interface.

@CodeBlanch
Copy link
Member Author

@danielcweber I don't think anything is off the table 😄 I'm not totally following you though. Could you post some pseudo-code (or whatever) to kind of demo how this would be used?

One potential solution you can do right now...

This extension allows you to register a callback action which will be fired when the TraceProvider build is invoked:

public static TracerProviderBuilder Configure(this TracerProviderBuilder tracerProviderBuilder, Action<IServiceProvider, TracerProviderBuilder> configure)

Now of course you need the TracerProviderBuilder instance to do that, so it isn't immediately helpful for your case, but it can still be helpful! Let's say you define some helper service you can register like...

public delegate void ConfigureTracerProviderAction(IServiceProvider serviceProvider, TracerProviderBuilder builder);

Now we can use that extension above to (at "build" time) find extra external configurations and run them:

            services.AddOpenTelemetryTracing((builder) => builder
                .Configure((sp, builder) =>
                {
                    IEnumerable<ConfigureTracerProviderAction> registeredConfigurations = sp.GetServices<ConfigureTracerProviderAction>();
                    foreach (ConfigureTracerProviderAction registeredConfiguration in registeredConfigurations)
                    {
                        registeredConfiguration(sp, builder);
                    }
                }));

That allows you to then register anything you want to run with the IServiceCollection independently of TracerProviderBuilder (so long as you can access IServiceCollection):

            services.AddTransient<MyInstrumentation>();
            services.AddSingleton<ConfigureTracerProviderAction>((sp, builder) => builder
                .AddInstrumentation(() => sp.GetRequiredService<MyInstrumentation>()));

@danielcweber
Copy link

danielcweber commented Jan 8, 2022

That allows you to then register anything you want to run with the IServiceCollection independently of TracerProviderBuilder (so long as you can access IServiceCollection):

That's exactly the point. I don't want to have to configure something on TracerProviderBuilder and at the same time register services on an IServiceCollection in two different places in the code. I want my instrumentation to be configured by a single call to an extension to TracerProviderBuilder and do it all there. But since there's a need to register decorators, I need to get an IServiceCollection from the TracerProviderBuilder. It's usually in there, since TracerProviderBuilderHosting wraps it...but there's no interface that would expose it.

Basically, I want to repeat the pattern seen here, but with an interface that would let me get the IServiceCollection.

@CodeBlanch
Copy link
Member Author

@danielcweber

If you just want to make an extension method that configures + does service registrations, does this work?

https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Extensions.Hosting#building-extension-methods

You have to take a dependency on OpenTelemetry.Extensions.Hosting to get access to all the fun extensions.

GetServices gives you back the IServiceCollection:

public static IServiceCollection GetServices(this TracerProviderBuilder tracerProviderBuilder)
{
if (tracerProviderBuilder is TracerProviderBuilderHosting tracerProviderBuilderHosting)
{
return tracerProviderBuilderHosting.Services;
}
return null;
}

@danielcweber
Copy link

Awesome. I had no clue this extension existed. I should have searched for it more thoroughly.

Thanks a lot. That should do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants