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 #2058

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented May 23, 2021

Related to #1889

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 simply expose IServiceCollection.

namespace OpenTelemetry.Trace
{
    public interface IDeferredTracerProviderBuilder
    {
        // removed
        // IServiceCollection Services { get; }

        // added
        IDeferredServiceCollection Services { get; }

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

// added to OpenTelemetry namespace because I'm expecting
// OpenTelemetry.Metrics.IDeferredMeterProviderBuilder to exist
// at some point and it will also use these
namespace OpenTelemetry
{
    // new
    public enum DeferredServiceLifetime
    {
        Singleton = 0,
        Scoped = 1,
        Transient = 2,
    }

    // new
    public interface IDeferredServiceCollection : IList<DeferredServiceDescriptor>
    {
    }

    // new
    public sealed class DeferredServiceDescriptor
    {
        public DeferredServiceDescriptor(Type serviceType, object instance) {}

        public DeferredServiceDescriptor(Type serviceType, Type implementationType, DeferredServiceLifetime lifetime) {}

        public DeferredServiceDescriptor(Type serviceType, Func<IServiceProvider, object> factory, DeferredServiceLifetime lifetime) {}

        public Type ServiceType { get; }

        public DeferredServiceLifetime Lifetime { get; }

        public Type ImplementationType { get; }

        public object ImplementationInstance { get; }

        public Func<IServiceProvider, object> ImplementationFactory { get; }
   }
}

TODOs

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

@CodeBlanch CodeBlanch requested a review from a team May 23, 2021 18:56
@codecov
Copy link

codecov bot commented May 23, 2021

Codecov Report

Merging #2058 (efd6448) into main (a19d9e0) will decrease coverage by 1.19%.
The diff coverage is 4.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2058      +/-   ##
==========================================
- Coverage   84.52%   83.32%   -1.20%     
==========================================
  Files         187      189       +2     
  Lines        6092     6184      +92     
==========================================
+ Hits         5149     5153       +4     
- Misses        943     1031      +88     
Impacted Files Coverage Δ
src/OpenTelemetry.Api/DeferredServiceDescriptor.cs 0.00% <0.00%> (ø)
...osting/Implementation/DeferredServiceCollection.cs 4.05% <4.05%> (ø)
...ing/Implementation/TracerProviderBuilderHosting.cs 38.46% <50.00%> (ø)
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 82.35% <0.00%> (+2.94%) ⬆️

@ejsmith
Copy link
Contributor

ejsmith commented Jun 2, 2021

Isn't this going to mean that all existing extensions on IServiceCollection aren't going to work?

@CodeBlanch
Copy link
Member Author

@ejsmith

Isn't this going to mean that all existing extensions on IServiceCollection aren't going to work?

Depends. DeferredServiceCollection is just a wrapper over IServiceCollection. So...

  • Code in the process that has access to the IServiceCollection instance can use all of the extensions and things will work fine.

  • Instrumentation/libraries using IDeferredServiceCollection can't use the extensions. These have to use the IDeferredServiceCollection interface or add new extensions that do. I'm guessing though that these libraries will only register a handful of services so it shouldn't be too painful to use the underlying Add/ServiceDescriptor API?

Should we just take a dependency on Microsoft.Extensions.DependencyInjection.Abstractions in the API project? Maybe! Looking to @cijothomas for his thoughts. No net452 target on there so we will likely introduce issues where things compile strangely on .NET Framework because it picks net452 lib instead of netstandard2.0.


IEnumerator IEnumerable.GetEnumerator() => this.GetEnumerator();

private static DeferredServiceDescriptor ConvertToOtel(ServiceDescriptor serviceDescriptor)
Copy link
Contributor

@utpilla utpilla Jun 2, 2021

Choose a reason for hiding this comment

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

Rename the methods to ConvertToDeferredServiceDescriptor and ConvertToDIServiceDescriptor?

@ejsmith
Copy link
Contributor

ejsmith commented Jun 2, 2021

I really wish you guys would just take a dep on ServiceProvider and Logging. I know there is some issue for supporting some really far back .NET version, but you are building the future of telemetry for .NET apps and a very large price is being paid for supporting a framework that is extremely unlikely to be used in combination with this lib. Nobody builds new apps on old frameworks and nobody would retrofit something like Otel on an app that can't even be upgraded to a slightly newer framework version.

return new DeferredServiceDescriptor(serviceDescriptor.ServiceType, serviceDescriptor.ImplementationType, lifetime);
}

private static DeferredServiceLifetime ConvertToOtel(ServiceLifetime serviceLifetime)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename the methods to ConvertToDeferredServiceLifetime and ConvertToDIServiceLifetime?

@cijothomas cijothomas added this to the 1.1.0-beta4 milestone Jun 14, 2021
@cijothomas
Copy link
Member

As discussed offline and in today SIG meeting:

  1. We can remove IServiceCollection Services; property from IDeferredTracerProviderBuilder. This will remove support for some scenarios, but it still supports quite a lot of scenarios that 1.0.1 did not support. Specifically, the following scenario is not supported with this:
public static class MyLibraryExtensions
{
    public static TracerProviderBuilder AddMyFeature(this TracerProviderBuilder tracerProviderBuilder)
    {
        if (!(tracerProviderBuilder is IDeferredTracerProviderBuilder deferredTracerProviderBuilder))
        {
            throw new NotSupportedException(
                "MyFeature requires an IDeferredTracerProviderBuilder instance.");
        }

        deferredTracerProviderBuilder.Services
            .AddHostedService<MyHostedService>()
            .AddSingleton<MyService>()
            .AddSingleton<MyProcessor>()
            .AddSingleton<MySampler>();

        return tracerProviderBuilder
            .AddProcessor<MyProcessor>()
            .SetSampler<MySampler>();
    }
}
  1. We can add IServiceCollection Services back in 1.3.0 release, when frameworks older than net462 are end-of-life, and we wouldn't have the conditional framework issue.
  2. This should unblock the 1.1.0-beta5 (and hence 1.1.0) release.

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.

4 participants