[API Proposal]: Introduce DI friendly IMeter<T> for modern services

[dpk83]

This is edited by @tarekgh

Runtime Metrics APIs Proposal

At present, the metrics APIs that are exposed do not integrate smoothly with DI. This is because the current support relies on creating Meter objects as static, which cannot be utilized within DI containers. As a solution, this proposal recommends developing new APIs that will enable metrics to work seamlessly with DI.

Meter Factory

The IMeterFactory interface is being introduced, which can be registered with the DI container and utilized to create or obtain Meter objects.

IMeterFactory

namespace System.Diagnostics.Metrics
{
    public interface IMeterFactory : IDisposable
    {
        Meter Create(
                string name,
                string? version = null,
                IEnumerable<KeyValuePair<string, object?>>? tags = null);
    }
}

A default implementation will be offered, which should be adequate for most use cases. Nonetheless, users may choose to provide their own implementation for alternative purposes, such as testing.

Meter factories will be accountable for the following responsibilities:

• Creating a new meter.
• Attaching the factory instance to the Meter constructor for all created Meter objects. Additional details regarding this will be addressed later in the metrics APIs' additions.
• Storing created meters in a cache and returning a cached instance if a meter with the same parameters (name, version, and tags) is requested.
• Disposing of all cached Meter objects upon factory disposal.

DI IMeterFactory Registration

The default IMeterFactory registration can be done by the API:

namespace Microsoft.Extensions.Metrics
{
    public static class MetricsServiceExtensions
    {
        public static IServiceCollection AddMetrics(this IServiceCollection services);        
    }
}

Core Metrics APIs Addition

The proposed modifications aim to facilitate the following:

• Inclusion of default tags to the Meter
• Addition of default tags to the instruments
• Incorporation of scope into the Meter involves assigning an IMeterFactory object to the Meter, which serves as a marker for all Meters associated with a particular factory.
• The Meter will now cache non-Observable instruments internally to prevent re-creation when attempting to recreate the same instrument with identical parameters such as name, unit, tags, and generic type.

namespace System.Diagnostics.Metrics
{
    public abstract class Instrument
    {
        protected Instrument(
                    Meter meter,
                    string name,
                    string? unit,
                    string? description,
                    IEnumerable<KeyValuePair<string, object?>>? tags);

         public IEnumerable<KeyValuePair<string, object?>>? Tags { get; }
    }

    public abstract class Instrument<T> : Instrument where T : struct
    {
        protected Instrument(
                    Meter meter,
                    string name,
                    string? unit,
                    string? description, IEnumerable<KeyValuePair<string, object?>>? tags);
    }

    public abstract class ObservableInstrument<T> : Instrument where T : struct
    {
        protected ObservableInstrument(
                    Meter meter,
                    string name,
                    string? unit,
                    string? description,
                    IEnumerable<KeyValuePair<string, object?>> tags);
    }

    public class Meter : IDisposable
    {
        public Meter(
                string name,
                string? version,
                IEnumerable<KeyValuePair<string, object?>>? tags,
                IMeterFactory? factory = null);

        public IEnumerable<KeyValuePair<string, object?>>? Tags { get ; }}

        public IMeterFactory? Factory { get; }

        public Counter<T> CreateCounter<T>(
                            string name,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public UpDownCounter<T> CreateUpDownCounter<T>(
                            string name,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public Histogram<T> CreateHistogram<T>(
                            string name,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableCounter<T> CreateObservableCounter<T>(
                            string name,
                            Func<T> observeValue,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableCounter<T> CreateObservableCounter<T>(
                            string name,
                            Func<Measurement<T>> observeValue,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableCounter<T> CreateObservableCounter<T>(
                            string name,
                            Func<IEnumerable<Measurement<T>>> observeValues,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableUpDownCounter<T> CreateObservableUpDownCounter<T>(
                            string name,
                            Func<T> observeValue,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableUpDownCounter<T> CreateObservableUpDownCounter<T>(
                            string name,
                            Func<Measurement<T>> observeValue,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableUpDownCounter<T> CreateObservableUpDownCounter<T>(
                            string name,
                            Func<IEnumerable<Measurement<T>>> observeValues,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableGauge<T> CreateObservableGauge<T>(
                            string name,
                            Func<T> observeValue,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableGauge<T> CreateObservableGauge<T>(
                            string name,
                            Func<Measurement<T>> observeValue,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableGauge<T> CreateObservableGauge<T>(
                            string name,
                            Func<IEnumerable<Measurement<T>>> observeValues,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;
}

Testing Helper

The InstrumentRecorder class has been introduced to facilitate metric testing with DI. With this class, it is straightforward to monitor a particular instrument and its meter readings, and then generate reports on the values published by the instrument.

namespace Microsoft.Extensions.Metrics
{
    public sealed class InstrumentRecorder<T> : IDisposable where T : struct
    {
        public InstrumentRecorder(Instrument instrument);
        public InstrumentRecorder(
                        IMeterFactory? meterFactory,
                        string meterName,
                        string instrumentName);
        public InstrumentRecorder(
                        Meter meter,
                        string instrumentName);

        public Instrument Instrument { get; }

        public IEnumerable<Measurement<T>> GetMeasurements();
        public void Dispose();
    }
}

Code Example

    //
    // Register Metrics
    //

    services.AddMetrics();

    //
    // Access the MeterFactory
    //

    IMeterFactory meterFactory = serviceProvider.GetRequiredService<IMeterFactory>();

    //
    // Create Meter
    //

    Meter meter = meterFactory.Create(
                            "MeterName",
                            "version",
                            new TagList()
                            {
                                { "Key1", "Value1" },
                                { "Key2", "Value2" }
                            });

    //
    // Create Instrument
    //

    Counter<int> counter = meter.CreateCounter<int>(
                            "CounterName",
                            "cm",
                            "Test counter 1",
                            new TagList() { { "K1", "V1" } });

    //
    // Test instrument published values
    //

    var recorder = new InstrumentRecorder<int>(meterFactory, "MeterName", "CounterName");
    // can do the following too:
    // var recorder = new InstrumentRecorder<int>(counter);
    counter. Add(1);
    Assert.Equal(1, recorder.GetMeasurements().ElementAt(0));

end of @tarekgh edit and start of old description

Background and motivation

.NET exposes Meter API to instrument code for collecting metrics. Currently the Meter is a class which doesn't work well with DI (Dependency Injection) based model. In order to create a meter object a developer currently need to create an object of the Meter class and pass the meter name, something like this

var meter = new Meter("myMeter");

While this works find on a smaller scale, for large scale complex services this starts becoming difficult to manage and having a DI based API surface could make things much simpler. Apart from the fact that DI would improve the testability of the code, it will also allow for better management of creation of Meter object in a consistent fashion. With DI based approach a meter can be consistently created with the type name of the class where the meter is injected, which opens up a lot of possibilities for further extensions that could simplify the programming model.

We have a metrics solution that our teams uses. When we implemented the solution .NET metrics API was not in existence so we had created our own API surface for metrics. We introduced DI friendly IMeter and IMeter which works similar to ILogger and ILogger. Over the time we found the DI friendly surface extremely useful and extensible, it turned out to be super easy for services and has allowed us to have a consistent approach of creating meter object everywhere. Service owners and library owners don't need to worry about cooking up names for the meter object and the meter object automatically gets created with consistent naming. This allows us to add various capabilities by providing filtering based on the meter names via config. Few examples of the capabilities

• Services can configure the state of individual meters via config (i.e. enable/disable a meter by configuring the metering state in their appsettings.json using the type name of namespace, similar to how one can configure logging levels with .NET ILogger)
• Services can configure different destinations for metrics generated by different meter objects. This works even when using third party libraries seamlessly as long as the libraries are using DI to inject meter. etc.

Modern .NET services use DI heavily due to it's benefits and the trend will only increase going forward. Based on our experience with IMeter API and the feedback of simplicity from our customers, we think it would be highly useful for the rest of the community too.

API Proposal

namespace System.Diagnostics.Metrics;

public interface IMeter
{
}

public interface IMeter<out TCategoryName> : IMeter
{
}

OR

namespace System.Diagnostics.Metrics;

public class Meter<out TCategoryName>
{
}

API Usage

public class MyClass
{
    // Inject Meter<T> object using DI
    public MyClass(IMeter<MyClass> meter)
    {
        var counter = meter.CreateCounter<long>("counter1");
    }
}

public class AnotherClass
{
    // Inject a meter with the full name of the provided T gets injected using DI.
    public AnotherClass(IMeter<AnotherClass> meter)
    {
        var counter = meter.CreateCounter<long>("counter2");
    }
}

Alternative Designs

The alternative is to write a custom DI wrapper for Meter and introduce Meter in a wrapper which then defeats the purpose of exposing the Meter API exposed from .NET directly.

Risks

It's a new addition to existing APIs so shouldn't cause breaking changes.

[ghost]

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection See info in area-owners.md if you want to be subscribed.

Issue Details

---

### Background and motivation .NET exposes Meter API to instrument code for collecting metrics. Currently the Meter is a class which doesn't work well with DI (Dependency Injection) based model. In order to create a meter object a developer currently need to create an object of the Meter class and pass the meter name, something like this ```csharp var meter = new Meter("myMeter"); ``` While this works find on a smaller scale, for large scale complex services this starts becoming difficult to manage and having a DI based API surface could make things much simpler. Apart from the fact that DI would improve the testability of the code, it will also allow for better management of creation of Meter object in a consistent fashion. With DI based approach a meter can be consistently created with the type name of the class where the meter is injected, which opens up a lot of possibilities for further extensions that could simplify the programming model. In R9, we have a metrics solution. When we implemented the solution .NET metrics API was not in existence so we had created our own API surface for metrics. We introduced DI friendly IMeter and IMeter which works similar to ILogger and ILogger. Over the time we found the DI friendly surface extremely useful and extensible, it turned out to be super easy for services and has allowed us to have a consistent approach of creating meter object everywhere. Service owners and library owners don't need to worry about cooking up names for the meter object and the meter object automatically gets created with consistent naming. This allows us to add various capabilities by providing filtering based on the meter names via config. Few examples of the capabilities - Services can configure the state of individual meters via config (i.e. enable/disable a meter by configuring the metering state in their appsettings.json using the type name of namespace, similar to how one can [configure logging](https://learn.microsoft.com/en-us/dotnet/core/extensions/logging?tabs=command-line#configure-logging) levels with .NET ILogger) - Services can configure different destinations for metrics generated by different meter objects. This works even when using third party libraries seamlessly as long as the libraries are using DI to inject meter. etc. Modern .NET services use DI heavily due to it's benefits and the trend will only increase going forward. Based on our experience with IMeter API and the feedback of simplicity from our customers, we think it would be highly useful for the rest of the community too. Here are examples to implementations and usage in R9 - [IMeterT](https://domoreexp.visualstudio.com/R9/_git/SDK?path=/src/Extensions/Metering.Abstractions/IMeterT.cs&version=GBmain) - Example of [registering IMeter](https://domoreexp.visualstudio.com/R9/_git/SDK?path=/src/Extensions/Metering.Geneva/GenevaMeteringExtensions.cs&version=GBmain&line=129&lineEnd=129&lineStartColumn=9&lineEndColumn=17&lineStyle=plain&_a=contents) - Example of [injection of IMeter and usage ](https://domoreexp.visualstudio.com/R9/_git/SDK?path=/src/Extensions/Secrets.CertificateTelemetry/CertificateMeteringService.cs&version=GBmain&line=80&lineEnd=80&lineStartColumn=1&lineEndColumn=50&lineStyle=plain&_a=contents) - Example of the extensibility it open up to control various things via config [here](https://eng.ms/docs/experiences-devices/r9-sdk/docs/telemetry/metering/geneva-metric-export#section-4) . This is similar to [.NET's logging config](https://learn.microsoft.com/en-us/dotnet/core/extensions/logging?tabs=command-line#configure-logging) ### API Proposal ```csharp namespace System.Diagnostics.Metrics; public interface IMeter { } public interface IMeter : IMeter { } ``` OR ```csharp namespace System.Diagnostics.Metrics; public class Meter { } ``` ### API Usage ```csharp public class MyClass { // Inject Meter object using DI public MyClass(IMeter meter) { var counter = meter.CreateCounter("counter1"); } } public class AnotherClass { // Inject a meter with the full name of the provided T gets injected using DI. public AnotherClass(IMeter meter) { var counter = meter.CreateCounter("counter2"); } } ``` ### Alternative Designs The alternative is to write a custom DI wrapper for Meter and introduce Meter in a wrapper which then defeats the purpose of exposing the Meter API exposed from .NET directly. ### Risks It's a new addition to existing APIs so shouldn't cause breaking changes.

Author:	dpk83
Assignees:	-
Labels:	`api-suggestion`, `area-Extensions-DependencyInjection`
Milestone:	-

[julealgon]

With DI based approach a meter can be consistently created with the type name of the class where the meter is injected, which opens up a lot of possibilities for further extensions that could simplify the programming model.

This part turned me off.

The same pattern is used with ILogger and I see it more as a workaround to lack of injection target inspection in the container itself. If you look at the ILogger<T> type, it "does nothing", it's basically a marker interface to differentiate registrations in the container: again, a workaround.

Instead of expanding on that (IMHO terrible) practice, I'd rather see something similar to how HttpClient is handled today. It suffers from similar issues, but they were solved without "hurting" the type itself: HttpClient remains non-generic, but there are injection helpers that facilitate associating a given HttpClient with the service that is going to consume it.

Now... don't get me wrong: I still think even the AddHttpClient methods are also hacky workarounds to the underlying root problem here which is an inability of inspecting the target injection type during dependency injection. It would be nice to have that as it would allow you to have all these patterns without dedicated container extensions, but that's probably a separate discussion altogether.

Lastly, about this:

public class MyClass
{
    // Inject Meter<T> object using DI
    public MyClass(IMeter<MyClass> meter)
    {
        var counter = meter.CreateCounter<long>("counter1");
    }
}

I'm not very fond of having instrument creation happening inside my classes either. In all scenarios where I've used custom metrics, I've always made sure to configure and set them all up in the container, and inject the counters. Consumers shouldn't have to know how to create the counters (and gauges, and other instruments); they should only use them.

On that front, what I've also done is extract a ICounter<T> interface for my own purposes, and test around that.

If this proposal is seriously taken into consideration, I'd also recommend potentially extracting common interfaces for the instruments themselves as they are, IMHO, the core of the untestability in regards to metrics classes.

[tarekgh]

Thanks @julealgon for the feedback, it is helpful. We'll look at this at some point next year. I am wondering if you have any specific proposal that you think addresses the request in the proper way.

[julealgon]

We'll look at this at some point next year.

That's good to hear. This API is indeed extremely unfriendly to modern patterns using Extensions.Hosting.

I am wondering if you have any specific proposal that you think addresses the request in the proper way.

Aside from providing an interface to each instrument type, I think this entire namespace needs a companion "Extensions.Hosting" extension with methods to add metrics and instruments to the container. In a sense, this approach wouldn't be too far off from what Azure and OTEL provide with their Microsoft.Extensions.Azure and OpenTelemetry.Extensions.Hosting respectively.

As for the API, I could see very similar needs to what the various AddHttpClient extensions provide:

• Possibility of "tying" a specific instrument to a service
• Possibility of tying a named instrument to a service
• Possibility of resolving a named instrument from inside a service (some sort of I{Instrument}Factory analogous to IHttpClientFactory)

As well as some stuff that would have to be thought from scratch I think:

• Possibility of tying multiple instruments to the same service (I actually have this exact requirement in one of our APIs)
• Fluent API to create and add Meter instances to the container along with their instruments

As part of this work, I'd also like to see better integration with OTEL itself, so that one doesn't need to pass strings around to indicate the meters to track etc. Maybe add some method in the OTEL extensions that just "honors all meters defined in the container automatically" (that's what would make the most sense in my use cases).

Lastly, I just want to emphasize again how hard and convoluted it is to create some of these container-related extensions, and that is mostly due to restrictions on what the MS container keeps track of. It would be really nice if MS reconsidered the feature-set it provides out of the native container, so that things such as "named/keyed registrations" and "target-based injection decisions" were possible natively (especially the latter).

For reference, I'll drop one of our more advanced usages here (should help people understand where I'm coming from). Things have been renamed/simplified from the actual code:

    public static MeterProviderBuilder AddCustomStuffInstrumentation(
        this MeterProviderBuilder meterProviderBuilder)
    {
        ArgumentNullException.ThrowIfNull(meterProviderBuilder);

        const string meterName = "Company.Product";

        return meterProviderBuilder
            .ConfigureServices(
                services => services
                    .AddSingleton<CustomStuffAvailabilityReportingContext>()
                    .AddSingleton<ICustomStuffAvailabilityReportingContext>(p => p.GetRequiredService<CustomStuffAvailabilityReportingContext>())
                    .AddHostedService<CustomStuffAvailabilityReportingService>()
                    .AddHostedService<InstanceInitializerService<ObservableGauge<int>>>()
                    .AddSingleton(_ => new Meter(meterName))
                    .AddSingleton(
                        p =>
                        {
                            var reportingContext = p.GetRequiredService<ICustomStuffAvailabilityReportingContext>();

                            return p
                                .GetRequiredService<Meter>()
                                .CreateObservableGauge(
                                    "available-stuff",
                                    observeValue: () => reportingContext.LastStuffCountReading,
                                    unit: "Stuff",
                                    description: "Tracks how much stuff is available.");
                        })

                    .AddNamedSingleton(
                        p => p
                            .GetRequiredService<Meter>()
                            .CreateUpDownCounterOfInt(
                                "pending-stuff",
                                unit: "Stuff",
                                description: "Tracks how much stuff is pending."),
                        name: CounterRegistrationNames.Pending)

                    .AddNamedSingleton(
                        p => p
                            .GetRequiredService<Meter>()
                            .CreateUpDownCounterOfInt(
                                "available-things",
                                unit: "Thing",
                                description: "Tracks how many things are available."),
                        name: CounterRegistrationNames.Available)

                    .AddNamedSingleton(
                        p => p
                            .GetRequiredService<Meter>()
                            .CreateCounter<int>(
                                "confirmed-things",
                                unit: "CodeRanges",
                                description: "Tracks how many things have been confirmed.")
                            .AdaptToICounter(),
                        name: CounterRegistrationNames.Confirmed)

                    .AddTransient<IStuffTracker>(p => new StuffTracker(
                        pendingStuffCounter: p.GetRequiredNamedService<ICounter<int>>(CounterRegistrationNames.Pending),
                        availableThingsCounter: p.GetRequiredNamedService<ICounter<int>>(CounterRegistrationNames.Available),
                        confirmedThingsCounter: p.GetRequiredNamedService<ICounter<int>>(CounterRegistrationNames.Confirmed)))

                    .Decorate<IRequestHandler<IngestStuffRequest, IngestStuffResponse>, IngestStuffMetricsHandler>()
                    .Decorate<IRequestHandler<CancelThingRequest, CancelThingResponse>, ThingCancellationMetricsHandler>()
                    .Decorate<IRequestHandler<ConfirmThingRequest, ConfirmThingResponse>, ThingConfirmationMetricsHandler>()
                    .Decorate<IRequestHandler<ReserveStuffRequest, ReserveStuffResponse>, StuffReservationMetricsHandler>())
            .AddMeter(meterName);
    }

Notes about this implementation:

• CustomStuffAvailabilityReportingContext is used as an "ambient" storage to communicate a hosted service with an ObservableGauge.
• The InstanceInitializerService is there to "force" initialization of the gauge (I wish I didn't have to do this, but the gauge only starts when it is first resolved from the container of course...)
• The AddNamedSingleton is our custom extension to register named instances in the container (using Lazy<T, TMetadata> internally). Obviously wish this had a native counterpart but there is none
• The CreateUpDownCounterOfInt is just a .NET6 workaround for UpDownCounter support (creates a custom ICounter implementation that uses 2 counters internally: one to represent increments, and another to represent decrements). This is going away now that we've migrated to .NET7 and can use the actual UpDownCounter instrument
• AdaptToICounter is a custom extension that returns an adapter that implements our custom ICounter interface (abstraction to enable proper testing)
• StuffTracker relies on 3 counters and updates them depending on some methods that are called on it. This is where being able to inject multiple instruments in the same target service would be useful
• Decorate is just using Scrutor to add a few decorators to some of our existing command handlers (MediatR), so that we can update the 3 counters depending on what actions are performed

Our initial implementation only contained the 3 counters. The ObservableGauge + Hosted Service were added later as another workaround: they will be removed soon.

Of note as well, is how unfriendly to DI the ObservableGauge API is... the whole thing about passing a container-controlled state object from the gauge to the service was really hard to get right, especially with having to force initialization of the gauge separately.

[tekian]

This is very problematic.

Whenever a new meter is created, a static (i.e., application-wide) meter list under lock is modified.

1. It significantly deteriorates testability. In fact, virtually forces serialization of test case run if multiple testcases would like to assert a single meter/metric value.
2. Is not dependency-injection friendly and goes against .NET's own design principles.

It should be noted that metering is typically being used to create business counters and custom metrics across services. Being able to test the counters - values and dimensions - is paramount.

I don't think counters should be declared upfront as per @julealgon's example because why should a consumer of a library need to know how counters are named, aggregated or what's the unit. And if I own the feature, why would I want them to unfold into DI? Quite contrary. Whoever writes the feature has all the knowledge to make the right call on how should the metrics be named, structured, aggregated and used.

I believe it would be enough to provide an abstract MeterFactory with CreateMeter() method. Inside my class, I would then create all the counters that I need, details of which are cohesively part of the class that uses them. But the major difference is that now I can test this. I can provide MockMeterFactory and assert any values I need.

The real implementation of MeterFactory would new-up Meter, registering it with the static collection (still yuck!).

public class MyClass
{
    public MyClass(MeterFactory factory)
    {
        var meter = factory.CreateMeter("throttling");
        var requestsTotal = meter.CreateCounter<long>("requests-total");
    }
}

[julealgon]

I don't think counters should be declared upfront as per @julealgon's example because why should a consumer of a library need to know how counters are named, aggregated or what's the unit.

I have no idea what scenario you are alluding to. Certainly not mine, as that is not library code. Of course there would be zero expectation for a library consumer to known which counters a library needs, that's the concern of the library itself. It just does not apply at all to my example however as all the code above is self-contained and not consumed anywhere.

And if I own the feature, why would I want them to unfold into DI? Quite contrary. Whoever writes the feature has all the knowledge to make the right call on how should the metrics be named, structured, aggregated and used.

That setup code above is part of "whoever owns the feature". Configuring a meter, and using a meter, are 2 completely distinct concerns. Much like configuring a logger, or an httpclient, are. The similarity is very fitting, particularly when comparing to HttpClient.

What the consumer of the counter does is increment/decrement it, those are the behaviors they are interested in. Setting them up is a configuration/aggregation root concern.

Mocking a factory that returns concrete classes is practically useless for testing purposes, since I'd still not be able to test the main interaction with the counters: the increment/decrement triggers.

Thus, I strongly disagree with your take on this point.

The rest of your post, regarding the poor practices on how static meters are maintained inside the class, I 100% agree with however.

[noahfalk]

@julealgon - I' m not confident I captured all your constraints in the example above, but I'm curious if something like this feels like it is a step in the right direction to simplify your scenario:

    class CustomStuffInstrumentation : IDisposable
    {
        static string MeterName = "Company.Product";
        Meter _meter = new Meter(MeterName);
        public ObservableGauge<int> _availableThings = new ObservableGauge<int>("available-things", () => LastStuffCountReading);
        public UpDownCounter<int> PendingStuff { get; } = _meter.CreateUpDownCounter<int>(...);
        public UpDownCounter<int> ConfirmedThings { get; } = _meter.CreateUpDownCounter<int>(...);
        public int LastStuffCountReading { get; set; }

        public void Dispose()
        {
            _meter.Dispose();
        }
    }

    class StuffTracker : IStuffTracker
    {
        CustomStuffInstrumentation _instrumentation;

        // code in this type can access the properties of CustomStuffInstrumentation to 
        // manipulate the counters.
        public StuffTracker(CustomStuffInstrumentation inst) { _instrumentation = inst; }
    }

    public static MeterProviderBuilder AddCustomStuffInstrumentation(
        this MeterProviderBuilder meterProviderBuilder)
    {
        ArgumentNullException.ThrowIfNull(meterProviderBuilder);
        return meterProviderBuilder
            .ConfigureServices(
                services => services
                    .AddSingleton<CustomStuffInstrumentation>()
                    .AddTransient<IStuffTracker,StuffTracker>();
                    // any other types that need to manipulate the values of the counters
                    // could follow a similar pattern as StuffTracker
            )
            .AddMeter(CustomStuffInstrumentation.MeterName);
    }

Let me know if I missed constraints and there are things this wouldn't handle, or if you have an idea of what a further simplification would look like. I think new .NET APIs relating to Meter can certainly be on the table but for now I would assume solutions should still be scoped within the existing capabilities of .NET's DI system.

[EDIT]: Added a missing AddMeter() invocation and made CustomStuffInstrumentation implement IDisposable

[noahfalk]

@tekian

It significantly deteriorates testability. In fact, virtually forces serialization of test case run if multiple testcases would like to assert a single meter/metric value.

Do you have an example of a test you would like to write, but you feel that you can't because of this constraint?

Also I'm not sure why different test cases need to be sharing the same Meter object. Is there something that encourages or forces tests to be written that way?

[CodeBlanch]

The one thing I don't think we've really hit on here is disposal. Meter implements IDisposable. So to do things "correctly" and pass analysis today we need some boilerplate:

sealed class MyClass : IDisposable
{
    private readonly Meter meter;

    public MyClass()
    {
        meter = new("MyMeter");
    }

    public void Dispose()
    {
         meter.Dispose();
    }
}

I think that's where IMeter<T> becomes useful.

sealed class MyClass
{
    private readonly IMeter<MyClass> meter;

    public MyClass(IMeter<MyClass> meter)
    {
        this.meter = meter;
    }
}

Lets the IServiceProvider/host manages the lifetime for us.

Not saying it is a must-have just felt it was worth calling out 😄

[noahfalk]

Sorry @dpk83, I know this has been waiting a while but now I am digging into these :) Right now it feels like there are a number of different goals and I am trying to tease it apart. Let me go through your text and show where I have some questions and then I'm hoping together we can connect the dots.

Currently the Meter is a class which doesn't work well with DI (Dependency Injection) based model.

Can we dig in on what it means for Meter not to work well in DI? We can apply a standard transformation which can do DI with any type, including Meter. Its totally fine if you want to enumerate everything you don't like about the outcome it gives, I'm just trying to get specific on what aspects we are trying to improve. This is my transformation:

// Step 0: starting from a class that doesn't use DI
class Worker
{
    static Meter s_meter = new Meter("Worker");
    public void DoSomething()
    {
        Console.WriteLine(s_meter.Name);
    }
}

// Step 1: extract the dependency via constructor parameterization and put it in the container
class Worker
{
    Meter _meter;
    public Worker(Meter meter) { _meter = meter; }
    public void DoSomething()
    {
        Console.WriteLine(_meter.Name);
    }
}

// Step 2: Different components might want to put different Meters in the container so we need
// to wrap it in a containing class to prevent collisions
class WorkerInstrumentation
{
    public Meter {get;} = new Meter("Worker");
}

class Worker
{
    Meter _meter;
    public Worker(WorkerInstrumentation instrumentation) { _meter = instrumentation.Meter; }
    public void DoSomething()
    {
        Console.WriteLine(_meter.Name);
    }
}

// Step 3: Meter implements IDisposable, previously the service collection would have disposed it
// automatically but now that a container is present we have to do it
class WorkerInstrumentation : IDisposable
{
    public Meter {get;} = new Meter("Worker");
    public void Dispose() { Meter.Dispose(); }
}

DI would improve the testability of the code

Do you have a particular example in mind of how you want to test the code?

it will also allow for better management of creation of Meter object in a consistent fashion

Assuming we implemented either of the suggested APIs above, how do you envision that developers would add a Meter to the DI container?

With DI based approach a meter can be consistently created with the type name of the class where the meter is injected, which opens up a lot of possibilities for further extensions that could simplify the programming model.

Auto-populating a name from the type parameter seemed like syntatic sugar. I'm not saying syntatic sugar is bad or not worthwhile, but it wasn't clear to me how the presence of the syntatic sugar in one place was going to unlock benefits elsewhere. You mentioned R9 configuring enablement and destinations by name, but I assume all those capabilties would still work just as well if the name had been initialized using new Meter("My.Meter.Name")? If so it suggested that all those features can be built on the current Meter API and they are orthogonal from whether we do this feature.

A last thought on this part, what do you think in general about the policy of naming Meters based on type names? For example if one day a dev decided to refactor some code and change a class name, should that also change the name of the Meter? For logs I think its expected that most library authors aren't going to guarantee back-compat on their exact logging output and most logs will be interpreted by humans. For metrics I would have guessed developers will have stronger expectations of compat, and they will hard-code Meter/Instrument names into monitoring systems and expect them to remain constant over time. Although I have a little skepticism here, mostly I am just admitting ignorance. I should probably try to find more real world examples of how developers treated metric naming conventions over time. I could imagine there might be strong differences between metrics in widely used 3rd party libraries vs. metrics in code that only runs in the context of one service.

Based on our experience with IMeter API

One important thing we'll need to reckon with is that .NET's Meter.CreateXXX APIs always allocate a new Instrument whereas it sounds like the R9 implementation had some form of GetOrCreate semantics. This means that scenarios like this which presumably work fine in R9's impl would be memory leaks in .NET's impl if we left it as-is:

class MyHomeController
{
    Counter<int> _counter;
    MyHomeController(IMeter<MyHomeController> meter)
    {
        _counter = meter.CreateCounter<int>("Foo"); // this is allocating a new object on every
                                                    // request
    }
}

I'm not super-excited about having API called Create to also act as a Lookup, but I certainly consider it on the table as part of what we do here.

Thanks @dpk83!

[davidfowl]

I haven't caught up here but I plan to and will drop some feedback. This is very important for ASP.NET Core adoption of the meter APIs as well.

[dpk83]

We can apply a standard transformation which can do DI with any type, including Meter.

While we can do this, it will require us to write a wrapper in our libraries. This means that our customers will have to have dependencies on our libraries even for these core abstractions. This dependency then start causing problems. To elaborate further

We have a library which provide a functionality X. This library emit some metrics so it uses the Meter API. Now this library has 2 options

1. Write the wrapper to perform this transformation to do DI: This doesn't scale as we have 10s of libraries. If we were to take this approach every library will need to write these wrappers and then ensuring that every library is using the exact same pattern to do that becomes challenging.
2. Use a wrapper provided by the our own core Metering abstractions library: This solution scales very well across all our libraries but it brings the dependency of the core metering abstractions library to all these other libraries that need to emit metrics.

Why is option 2 a problem? Consider this case:

• The core abstractions package which provides the required wrapper for DI is package A
• There are 2 libraries B and C which provides certain functionalities. Both these libraries also emit metrics and thus depends on A.
• One of our customer produces a library X which uses library B
• Another customer uses X and C in their service. So both of them have dependency on A.

Now our release cycle is every month, so if the customer needs to upgrade to the latest version of C and if X is hasn't upgraded to latest version of B, then it will cause the issues due to different versions of A.

Given logging and metering APIs are core APIs and are used by almost all the packages that we deliver this additional dependency definitely hurts. It will be great to get this core support directly from .NET so we can avoid this dependency hell to certain extent.

Do you have a particular example in mind of how you want to test the code?

The lack of ability to mock the meter is what hurts testability. In our metering implementation we have an IMeterProvider which is responsible for providing IMeter objects. Everything here is DI injected so we can easily mock it in our tests whereas we can't mock the Meter object as it's new'ed up inside the user's code.

Assuming we implemented either of the suggested APIs above, how do you envision that developers would add a Meter to the DI container?

With our current implementation we register both IMeter and IMeter, developers then add meters by injecting them directly into the class

class MyClass
{
    public IMeter _meter;
    public MyClass(IMeter meter)
    {
         _meter = meter;
    }

   // OR
   public MyClass(IMeter<MyClass> meter)
   {
         _meter = meter;
   }
}

I assume all those capabilties would still work just as well if the name had been initialized using new Meter("My.Meter.Name")

Yes, they will work even if it's created this way. The problem however is to educate the teams and to enforce the consistent naming across our own libraries as well as for customers. The consistent naming pattern provides the advantage that customers can quickly learn and use it consistently, so if they are using 10 different libraries, they can just configure the meter configuration same way for all of them without needing to search through documentation (and/ore code) for each library to find the meter name that the library is using.

A last thought on this part, what do you think in general about the policy of naming Meters based on type names? For example if one day a dev decided to refactor some code and change a class name, should that also change the name of the Meter? For logs I think its expected that most library authors aren't going to guarantee back-compat on their exact logging output and most logs will be interpreted by humans. For metrics I would have guessed developers will have stronger expectations of compat, and they will hard-code Meter/Instrument names into monitoring systems and expect them to remain constant over time.

Generally metric backends work on instrument name, not so much on metric name so the accidental refactoring is less of a concern. However, I agree that it still is a concern because it will break whatever functionality the metric name is used for e.g. ILogger has the same pattern and runs the same risks.

Regardless, meter's using the name of the class is not that much of a concern as long as there is a good DI way to inject meter. Currently we followed the model that ILogger has along with the IMeterProvider and IMeter interfaces and that served our DI needs well enough, so that's what we proposed. But if there is a better model we can have I am all up for it.

[dpk83]

While this issue is opened for DI for Meter, it will be great to apply that to Activity as well

[noahfalk]

Thanks @dpk83!

Write the wrapper to perform this transformation to do DI: This doesn't scale as we have 10s of libraries

By 'doesn't scale well' does that mean the existing pattern uses too many lines of code and we want a pattern that can be written with fewer lines of code? I hope this doesn't seem pedantic, I'm just trying to ensure its clear what we are trying to improve.

The lack of ability to mock the meter is what hurts testability

In the API suggestion above you were proposing either IMeter\ or Meter\. My understanding is that IMeter\ would allow mocking, but Meter\ is no easier than Meter. Is there a good way to mock Meter\ that I'm not thinking of, or we shouldn't treat those solutions as being equally viable because one allows mocking and the other one doesn't? For the IMeter part of the solution did you mean that all the CreateXXX APIs are part of the IMeter\ interface, and they return ICounter, IGauge, IHistogram, etc so that the entire closure of the API surface has been wrapped with interfaces?

[dpk83]

By 'doesn't scale well' does that mean the existing pattern uses too many lines of code and we want a pattern that can be written with fewer lines of code? I hope this doesn't seem pedantic, I'm just trying to ensure its clear what we are trying to improve.

It's not about too many lines of code but it's about repeating those lines in 50 different packages and if we are repeating it then one need to also ensure that everyone is following the right pattern or writing the code in the same way etc.

In the API suggestion above you were proposing either IMeter or Meter. My understanding is that IMeter would allow mocking, but Meter is no easier than Meter. Is there a good way to mock Meter that I'm not thinking of, or we shouldn't treat those solutions as being equally viable because one allows mocking and the other one doesn't? For the IMeter part of the solution did you mean that all the CreateXXX APIs are part of the IMeter interface, and they return ICounter, IGauge, IHistogram, etc so that the entire closure of the API surface has been wrapped with interfaces?

No, there is no good way to mock Meter. IMeter with IMeterProvider is what works best here.

For the IMeter part of the solution did you mean that all the CreateXXX APIs are part of the IMeter interface, and they return ICounter, IGauge, IHistogram, etc so that the entire closure of the API surface has been wrapped with interfaces?

This is what we have which was written before .NET Meter API came into existence so we had ICounter, IHistogram, IGauge interfaces and all CreateXXX APIs returned those interfaces.

[noahfalk]

It's not about too many lines of code but it's about repeating those lines in 50 different packages and if we are repeating it then one need to also ensure that everyone is following the right pattern or writing the code in the same way etc.

It sounds like you are worried about the odds that devs will make a mistake when implementing the pattern. I know lines of code doesn't capture this fully, but there is at least a rough correlation there. Users are much more likely to mess up a pattern that requires them to write 50 lines vs. a pattern that requires 1 line. In any case, I think I've got me a better understanding of what your concern is when you say a given design doesn't scale well. Thanks!

No, there is no good way to mock Meter. IMeter with IMeterProvider is what works best here.

Rats, I was hoping you were going to point out a cool technique I was unaware of :) But I do still have a trick up my sleeve. Even though Meter and Instrument may not be directly mockable they can be unit tested: https://gist.github.com/noahfalk/0e10f4a091dbec68595ff0e2ec0d3260

I think very few people have discovered that you can author that InstrumentRecorder helper which records all the inputs received by an Instrument, but if we publicized it better (and perhaps included it in some assembly) I hope you'll agree it makes testing an instrument pretty straightforward?

I've been exploring if we could use the Meter\ or IMeter\ pattern but it hasn't been as straightforward as I had hoped. Let me share snags I am hitting thus far.

I think users would naturally expect this code not to assert:

public MyController(Meter<MyController> meter)
{
    var counter = meter.AddCounter<int>("foo");
    Debug.Assert(counter.Meter == meter);
}

In order for that assert to pass I can't use the trick that Logger\ or GenevaMeter\ uses where it defines Meter\ as a wrapper that forwards all calls to a 2nd non-generic instance. Instead Meter\ needs to derive directly from Meter so that object identity is preserved. That is doable so far, but then there is a second constraint. The only way the DI container can create arbitrary generic instantiations is by registering the open generic implementation type and invoking its constructor. Logger\ and GenevaMeter\ define a constructor that takes an IWhateverFactory parameter and they use that factory to create the non-generic instance. However because Meter\ is the only instance and it was already created by the DI container there is no opportunity to use a factory. We might be able to say that anyone who wants to set the Meter version or set a Meter name that doesn't come from the type name shouldn't use this pattern, but they are still going to want something that works with DI. For example devs can an inject an ILoggerFactory if they want more control over the ILogger name but I wouldn't want an IMeterFactory to exist if Meter\ isn't going to be using it.

I'm still exploring but just wanted to give an update.

[julealgon]

I think users would naturally expect this code not to assert:

public MyController(Meter<MyController> meter)
{
    var counter = meter.AddCounter<int>("foo");
    Debug.Assert(counter.Meter == meter);
}

In order for that assert to pass I can't use the trick that Logger or GenevaMeter uses where it defines Meter as a wrapper that forwards all calls to a 2nd non-generic instance. Instead Meter needs to derive directly from Meter so that object identity is preserved. That is doable so far, but then there is a second constraint. The only way the DI container can create arbitrary generic instantiations is by registering the open generic implementation type and invoking its constructor. Logger and GenevaMeter define a constructor that takes an IWhateverFactory parameter and they use that factory to create the non-generic instance. However because Meter is the only instance and it was already created by the DI container there is no opportunity to use a factory.

Please... please don't make the same mistake as what was done with ILogger here: that's a dependency injection abomination that only exists to workaround a container limitation where you don't have target type information during the injection process (which some other containers have).

I'd be incredibly sad to see that terrible pattern continuing for Meter.

[noahfalk]

I'm exploring multiple options including the HttpClient style thing you suggested and alternate dependency injection features. You've mentioned a few times you don't like the pattern but what concretely makes it bad in your opinion? For example you've said it is a workaround to not having target type information, but what do you imagine the usage would look like if DI did support target type info and what about that approach would be better?

[dpk83]

I think very few people have discovered that you can author that InstrumentRecorder helper which records all the inputs received by an Instrument, but if we publicized it better (and perhaps included it in some assembly) I hope you'll agree it makes testing an instrument pretty straightforward?

We do have a FakeMeter implementation for better testability. The initial FakeMeter was for our IMeter/IMeterProvider APIs and now we have created one for the .NET Meter APIs which basically creates a FakeMeterListener kind of similar to what you are doing here but it's a meter listener and supports all instrument types, so you basically mock meter instead. Checkout the FakeMeterListener in our codebase.

The only way the DI container can create arbitrary generic instantiations is by registering the open generic implementation type and invoking its constructor. Logger and GenevaMeter define a constructor that takes an IWhateverFactory parameter and they use that factory to create the non-generic instance. However because Meter is the only instance and it was already created by the DI container there is no opportunity to use a factory.

How about introducing MeterProvider which is responsible for creating the meter? This can address the problem of avoiding the creation of multiple meters with the same name and help provide a GetOrCreate semantic instead of the Create semantic.

[julealgon]

I'm exploring multiple options including the HttpClient style thing you suggested and alternate dependency injection features.

I'm glad to hear that.

You've mentioned a few times you don't like the pattern but what concretely makes it bad in your opinion?

It introduces a marker interface that has zero actual behavior and only exists to trick the container into seeing it as a different type. This is almost by definition a bad practice. It leaks a fix for an infrastructure problem into consuming code.

The biggest offender for me is when unit testing and trying to provide a mock for it. If my class is marked internal, and I want to expose my assembly for testing using InternalsVisibleTo, I still cannot rely on mocking libraries such as Moq or NSubstitute to generate a mocked ILogger<MyClass> because they both rely on Castle.Proxies. To then allow the automatic mock to be generated, I have to add an additional InternalsVisibleTo towards "DynamicProxyGenAssembly2" which... again... is a hell of a leak.

I've more than once created my own custom container methods so that I could inject the logger as ILogger instead of as ILogger<SomethingHere> because of reasons like that.

For example you've said it is a workaround to not having target type information, but what do you imagine the usage would look like if DI did support target type info and what about that approach would be better?

If the container allowed registrations to inspect the target type, that entire crazy customization created for HttpClient, where it ties certain clients to certain services could be generalized, and then used for anything else such as Counter or Meter. It solves the underlying issue, dramatically reduces complexity related to how HttpClient is currently injected, opens the door for things like what we are discussing here and even other possible patterns, like named Options resolution without having to call GetValue(name) manually.

Target-type introspection would also open the possibility for other patterns which are exceedingly hard to do today such as composites and decorators. Decorators are implemented in a fairly hacky way by Scrutor, and composites need you to create "tricks" to make the container only try to resolve child implementations without going into infinite recursion.

[davidfowl]

If the container allowed registrations to inspect the target type, that entire crazy customization created for HttpClient, where it ties certain clients to certain services could be generalized, and then used for anything else such as Counter or Meter. It solves the underlying issue, dramatically reduces complexity related to how HttpClient is currently injected, opens the door for things like what we are discussing here and even other possible patterns, like named Options resolution without having to call GetValue(name) manually.

It pushes the complexity into a single place, the DI container. It also means the has to be an explicit registration for things like this:

services.AddSingleton<ILogger>((IServiceProvider sp, Type targetType) => sp.GetRequiredService<ILoggerFactory>().CreateLogger(targetType.Name));

We'd also need to manage situations where there's no target type (naked calls to GetService).

I'm not against this BTW, there's enough signal and workarounds that we should look into it seriously.

[julealgon]

It pushes the complexity into a single place, the DI container.

The way you are wording it makes it sound bad, but to me it's the contrary: it takes a LOT of complexity out of multiple places (i.e. HttpClient, IOptions, ILogger, etc), and unifies the behavior in a simpler manner in DI.

It also means the has to be an explicit registration for things like this:

services.AddSingleton<ILogger>((IServiceProvider sp, Type targetType) => sp.GetRequiredService<ILoggerFactory>().CreateLogger(targetType.Name));

Do you see this as a problem? Feel like this could be easily added in a generic manner by the AddLogging() extension and be completely transparent to all consumers. If they for any reason want to have full control over it, they can just inject ILoggerFactory and perform the call with whatever they need. ILogger<T> would immediately become deprecated.

We'd also need to manage situations where there's no target type (naked calls to GetService).

Can you elaborate on this one? Why would anyone want to have a naked call instead of just injecting the result directly? If it doesn't depend on anything to be instantiated, then it can be instantiated without the indirection in the first place.

If there are any places where there is a desire to delay the initialization, one could just write a delayed initialization decorator that instantiates (and caches on a local Lazy<T>) the actual instance on the method calls instead of at construction time. I've used this approach a few times already to deal with badly written third party libraries that do a ton of computation in the constructor to precisely avoid paying that price upfront.

Suposeddly that's a really bad practice though (having heavy logic inside the ctor) so again if you could provide some example of where this would be needed today that would be great.

I'm not against this BTW, there's enough signal and workarounds that we should look into it seriously.

I think most container authors would really appreciate having target type introspection support in the general DI abstraction. Not only it solves these cases we are talking about here but also gives a ton of power for customizing the injection for other scenarios as well. I'm sure it has been asked before: I remember discussions around the decorator aspects with @dotnetjunkie and @khellang here for example:

• https://github.com/khellang/Scrutor/issues/39

[davidfowl]

The way you are wording it makes it sound bad, but to me it's the contrary: it takes a LOT of complexity out of multiple places (i.e. HttpClient, IOptions, ILogger, etc), and unifies the behavior in a simpler manner in DI.

ILogger<T> isn't complex, open generics are a fine pattern to use here (it's a substitute for a factory where the type is the input). I understand the complexity around manufacturing the typed logger, but I don't think the pattern is a failure or broken. Named options are complex, but the client factory is built on top of them (along with other things).

Do you see this as a problem? Feel like this could be easily added in a generic manner by the AddLogging() extension and be completely transparent to all consumers.

I don't see it as a problem, but I don't see ILogger\<T> as a problem either.

If they for any reason want to have full control over it, they can just inject ILoggerFactory and perform the call with whatever they need. ILogger would immediately become deprecated.

Maybe?

Can you elaborate on this one?

I was reflecting on the feature that needs to be added to DI and how this general mechanism would only work when there is an explicit call to GetService outside of what the DI container does internally (or rather, GetService would also need to support an optional target type).

Why would anyone want to have a naked call instead of just injecting the result directly?

The caller of GetService doesn't always know what the target type will be at injection time (consider a factory that does lazy activation). Basically anywhere you see a call to GetService. But as you said, you can always resolve the logger factory and Create an instance of ILogger.

I think most container authors would really appreciate having target type introspection support in the general DI abstraction.

Target type resolution is one thing but it doesn't solve problems where the key isn't a type. That's why I'm more interested in https://github.com/dotnet/runtime/issues/64427 than target type resolution.

[davidfowl]

I should also add that ILogger\<T> is type safe, as in, you can’t pass the wrong logger category around because it’s encoded in the type system.

[noahfalk]

I've been continuing to explore designs, this is where I am at now: https://gist.github.com/noahfalk/85668c0e820e13650f9e344aafeba135 Feedback much encouraged!

How about introducing MeterProvider which is responsible for creating the meter?

Yep, this is the direction I wound up persuing in the design above, albeit I named it Factory to be closer to LoggerFactory.

I have to add an additional InternalsVisibleTo towards "DynamicProxyGenAssembly2" which... again... is a hell of a leak.

Thanks, that is nice concrete example. We probably have differing opinions on how bad it is that someone would need to write that, but for the moment at least its not something the current design suggestion imposes. The current suggestion neither has Meter\, nor does it require a mocking library to write unit tests.

[dpk83]

I've been continuing to explore designs, this is where I am at now: https://gist.github.com/noahfalk/85668c0e820e13650f9e344aafeba135

This looks good to me.

[julealgon]

@davidfowl

I should also add that ILogger is type safe, as in, you can’t pass the wrong logger category around because it’s encoded in the type system.

Well you can pass whatever type you want to that logger though. That's not very "safe" to me. This works just fine:

public class MyService
{
    public MyService(ILogger<AnotherService> logger) {...}
}

Sure, you don't have to pass strings around, but that's not something I'm proposing either. This is the ideal signature to me:

public class MyService
{
    public MyService(ILogger logger) {...}
}

And the only way to get that today is to provide a custom factory for the registration that manually calls ILoggerFactory.CreateLogger internally using the concrete type.

It is very doable, sure (I've done it several times as mentioned before), but it is noisy and you have to change the registration on every single class that relies on ILogger to use your new AddTransientWithLogger or whatever extension, which is extremely invasive. I can maybe make it work by creating my own custom wrapper for IServiceCollection or IServiceProvider and apply it to all classes, but then the performance cost probably becomes prohibitive.

It is the exact same thing as what happens with AddHttpClient: it is just a hack. Even the name clearly indicates "something is wrong": you have things like "AddTransient..." "AddSingleton..." and then suddenly "AddHttpClient". That makes zero sense. The original methods all talk about lifetime, and then you create another one that doesn't even mention the lifetime but has a different category entirely in the name. Multiple junior devs came to me asking what was that all about: it is just an unintuitive API through and through.

Containers like Autofac or StructureMap (or Unity...) have solved this since the beginning of time, by just allowing you to provide arbitrary arguments to any registration either using an actual instance, or resolved from the container itself.

• https://docs.autofac.org/en/latest/register/parameters.html

Instead of resolving this problem generally, Microsoft decided "nah... we are not going to support this in the abstraction to keep it simple" and then proceeded to create a bunch of nasty workarounds to it that produce the same end result in a much more convoluted, inflexible and hard to use manner.

That T is, again, just a workaround to a missing container feature and something that the consuming class should not have to know about in the first place.

---

@noahfalk

I've been continuing to explore designs, this is where I am at now: https://gist.github.com/noahfalk/85668c0e820e13650f9e344aafeba135 Feedback much encouraged!

Using a listener for testing is interesting. I'd be ok with that, I think.

[From the doc] Meter could be injected directly rather than MeterFactory if a good name can be automatically inferred, saving 1 LoC and potentially improving some naming consistency.

This sounds extremely biased to me as if you are adding it to get to a specific result upfront: ensuring there is zero dependency with container modifications. Saying an entirely new abstraction/indirection is just a single LoC is minimizing it to an extreme degree. This could be a great opportunity to push for the feature in the container (like minimal APIs and minimal hosting pushed some features in other areas) but with this mentality we'll live forever with the workarounds, leaky abstractions and SRP violations.

The current suggestion neither has Meter<T>...

❤️

...nor does it require a mocking library to write unit tests.

Nothing should require a mocking library to be testable. I think this sentence is disingenuous. While things certainly shouldn't require mocking tooling (that would be absolutely terrible), we shouldn't make it harder for people who want to use a mocking library.

[noahfalk]

[@dpk83] This looks good to me.

Great 👍

[@julealgon] This sounds extremely biased to me as if you are adding it to get to a specific result upfront

I wasn't commenting on the total utility of a named or target typed DI feature across all potential use-cases, I was commenting solely about how having that feature would impact the specific API patterns I had proposed for this Meter feature. If you think I am underestimating its impact in those specific cases please point out anything that was missed and I'm glad to get it fixed up.

If you and David want to pursue a target-typed and/or named DI services feature I'm not trying to discourage you in the least. I just don't think we can claim so far that it makes a substantial difference in the API patterns from that design.

[@julealgon] we shouldn't make it harder for people who want to use a mocking library.

[EDIT]: I wasn't sure at first what you meant, but then I realized you probably were still talking about Meter\. I had some discussion about interfaces but pretty sure that was irrelevant. No Meter\ in the current plan, if it ever comes back into the proposal we can dig into then.

[davidfowl]

The original methods all talk about lifetime, and then you create another one that doesn't even mention the lifetime but has a different category entirely in the name. Multiple junior devs came to me asking what was that all about: it is just an unintuitive API through and through.

We can agree to disagree 😄. AddHttpClient does more than add a factory, if named services existed, the implementation might change but not the user experience.

[julealgon]

@noahfalk

I just don't think we can claim so far that it makes a substantial difference in the API patterns from that design.

I still think it does, though. It eliminates the need for a secondary indirection/abstraction (the factory) in the vast majority of cases, which is precisely what the AddHttpClient method also does: you can not only configure the client, but also associate it with a target service so you can just inject HttpClient directly.

The exact same pattern would apply to meters. The idea is the same there: you want to configure a "generic" type (Counter/Meter/etc, again, similar toHttpClient), and pass it along to a certain class that will operate with it but not others (again, exact same thing as with theHttpClient`).

My whole point is that by generalizing what goes on with AddHttpClient results in the cleanest solution for this case (as well as for many others) and this could be an opportunity inside Microsoft to justify the changes to DI. It is much harder to advance something if there are no real examples to justify the advancement, and when they are part of the original design it makes it that much more enticing.

That's what I personally would like to see: simpler and consistent API that people understand without necessarily introducing additional abstractions for even the simplest case.

---

@davidfowl

We can agree to disagree 😄.

So you are saying this is intuitive... nothing strange about it:

AddTransient<TService, TImplementation>
AddScoped<TService, TImplementation>
AddSingleton<TService, TImplementation>
AddHttpClient<TService, TImplementation>

"Find the outlier". Even the name is extremely misleading there: you are not "adding an httpclient", you are "adding a transient service, and _then associating an httpclient to it".

AddHttpClient does more than add a factory, if named services existed, the implementation might change but not the user experience.

I certainly know it does add more than a factory: it is basically a poor-man's custom combination of named registrations plus target-typed injection.

The experience would be similar because similar APIs could be created, but those APIs would be mostly general purpose, like they are on so many DI containers! That's the big difference. Right now, the mechanism behind AddHttpClient is exclusive to HttpClient and cannot be used by anything else. I've had similar/identical needs with other classes but nope... can't have it: FtpClient (from libraries etc), TcpClient, WCF channel/channelfactory, simpler things like connectionstrings, shared IOptions (forcing me to have to use named options which adds burden to consumers), ILogger, and now things like Counter's. Anything that is "reusable" and "needs configuration for a specific use case" applies here: it is an extremely broad category of problem. Even IOptions<T> itself could be made transparent with such mechanisms: the only reason IOptions<T> is injected today is to differentiate it from the underlying T in terms of the container. If a "configure/associate" mechanism was added to DI, people could just inject T, and still have it go through the entire layered Options system before being passed to the consumer. Many people, including myself, think that the IOptions pattern is a less-than-ideal, invasive workaround to a problem that should be fixed in the DI container itself. It leaks an issue that the consumer should not have to know about into all consumer classes, who now have to have a dependency on a Microsoft.Extensions.Options abstraction to be provided a simple settings model.

All that to say that while you guys keep limiting this discussion to either HttpClient or Metrics, it is vastly more far reaching than that and could solve and massively simplify several patterns if implemented at the root where the problem lies: the DI container.

If you go a similar strategy for metrics stuff as was done with HttpClient (which I think would be the ideal API both for usability as well as consistency of use), you'd have to replicate a ton of work that was implemented behind the scenes to add ad-hoc "support" for both named registrations and target-typed injection, and that would still only solve the issue for metrics, but not for all other things I mentioned there

[noahfalk]

It eliminates the need for a secondary indirection/abstraction (the factory) in the vast majority of cases

Frequently eliminating the need for a factory seemed like a fair assessment and I updated the text in the gist. I agree that not needing that factory is an improvement on average, though we likely have different opinions on the magnitude of that improvement.

If you go a similar strategy for metrics stuff as was done with HttpClient (which I think would be the ideal API both for usability as well as consistency of use)

Do you want to give a code example of what you think this should look like? When I imagined patterns similar to HttpClient so far none of them seemed like an improvement over the designs in the gist but I may not be thinking of the same thing you are.

[CodeBlanch]

I'm a big fan of IServiceCollection & IServiceProvider and I love the ask here.

But my opinion is these things...

public MyController(IMeter meter) {}
public MyController(IMeter<MyController> meter) {}
public MyController(IMeterFactory factory) {}

...should be considered anti-patterns.

We don't want to treat Meter like we treat ILogger. What I will postulate here is that is good/encouraged to have fine granularity in our logging and it is bad/discouraged to have the same verbosity in our metrics. To say that another way, we should have many ILoggers and few Meters.

Let's say I use this pattern...

public ControllerA(IMeter<ControllerA> meter) {}
public ControllerB(IMeter<ControllerB> meter) {}
public ControllerC(IMeter<ControllerC> meter) {}

I might end up with metrics like...

myapp.controllera.index_counter myapp.controllerb.index_counter myapp.controllerc.index_counter

That isn't very useful and will probably create issues downstream.

For something like that I should use AspNetCore instrumentation/middleware which will have a single counter and just tag the route/template and path:

aspnet.request_counter route=controllera path=/ aspnet.request_counter route=controllerb path=/ aspnet.request_counter route=controllerc path=/

When it comes to custom metrics we want good strategic data for our libraries.

I think what we ought to do is just recommend a pattern like this...

    internal interface IMyLibraryTelemetry
    {
        void MessagePublished(string messageType);
        void MessageConsumed(string messageType);
    }

    internal sealed class MyLibraryTelemetry : IMyLibraryTelemetry
    {
        private static readonly Meter s_Meter = new("MyLibraryTelemetry");
        private static readonly Counter<int> s_MessagePublishedCounter = s_Meter.CreateCounter<int>("MessagePublished");
        private static readonly Counter<int> s_MessageConsumedCounter = s_Meter.CreateCounter<int>("MessageConsumed");

        public void MessagePublished(string messageType) {
            var tags = default(TagList);

            tags.Add("message.type", messageType);

            s_MessagePublishedCounter.Add(1, in tags);
        }

        public void MessageConsumed(string messageType) {
            var tags = default(TagList);

            tags.Add("message.type", messageType);

            s_MessageConsumedCounter.Add(1, in tags);
        }
    }

services.AddSingleton<IMyLibraryTelemetry, MyLibraryTelemetry>();

public MyController(IMyLibraryTelemetry myLibraryTelemetry) {}

It accomplishes everything from a DI perspective and also providers better separation of concerns. No need for any new APIs. And it isn't tied to any specific metrics API, Meter being used is just an implementation detail.

[noahfalk]

No need for any new APIs

Are you arguing against all the APIs being introduced in the scope of this feature, or just the IMeterFactory? This feature is also introducing InstrumentRecorder to aid with testing and Tags properties on Meters and Instruments. From your text I don't think you were arguing against those?

[FiniteReality]

I have a little experience in this area - I made a small library stylised after Microsoft.Extensions.Logging before System.Diagnostics.Metrics was really available or stable.

In my opinion, an IMeter<T> interface doesn't really make much sense unless you're also coupling it with a source generator to enable situations such as:

[Meter(nameof(PageMeters))]
public static partial class PageMeters
{
    [Counter("page.visit")]
    public static partial void PageVisit(IMeter<PageMeters> meter, [Tag("path")] string path);
}

class MyController
{
    public MyController(IMeter<PageMeters> meters)
    { }

    public ActionResult GetThings()
    {
        PageMeters.PageVisit(meters, "/my/things");
        return Ok();
    }
}

The source generator would generate an implementation of IMeter<PageMeters> containing all of the data, and an implementation of PageVisit which uses that implementation.

Personally, I think the IMeterFactory approach is easier to work with, which is why I went with that kind of approach in my library.

[CodeBlanch]

@noahfalk

Are you arguing against all the APIs being introduced in the scope of this feature, or just the IMeterFactory? This feature is also introducing InstrumentRecorder to aid with testing and Tags properties on Meters and Instruments. From your text I don't think you were arguing against those?

Yes correct. Sorry about that. I think the changes adding tags to the meters/instruments are great. 💯 InstrumentRecorder I don't think we really need. In my code above with IMyLibraryTelemetry I would write tests just to make sure the API is called where it should be, I wouldn't concern myself with what Meter does because that would be testing runtime internals not app logic. But if we want InstrumentRecorder to help people who really want to test the runtime internals, seems well enough to me.

[julealgon]

I would write tests just to make sure the API is called where it should be...

@CodeBlanch you just can't do that with the instrument classes since they don't have a backing interface.

...I wouldn't concern myself with what Meter does because that would be testing runtime internals not app logic. But if we want InstrumentRecorder to help people who really want to test the runtime internals, seems well enough to me.

It's not about testing the internals, but testing the results. Since the instrument classes are all concrete, you can't just mock them (without introducing your own interface + adapter that is...), so listening to the resulting events is the only way to test "that your code called the correct API".

[bartonjs]

Video

• Meter doesn't need to have the factory as typed data. Since it's only needed for ReferenceEquals later, it should just be object? context
• IMeterFactory has been moved from System.Diagnostics.Metrics to Microsoft.Extensions.Metrics.
• InstrumentRecorder seems like low level functionalty, so it should be in System.Diagnostics.Metrics
  • And the IMeterFactory parameter changed to object
  • And we added a Reset method

The pieces that are in System.Diagnostics.Metrics were discussed and are now effectively approved. We ran out of time before finishing some issues in IMeterFactory (should the name be Create, GetOrCreate, etc; the ownership and disposal semantics)

Approved:

namespace System.Diagnostics.Metrics
{
    public abstract partial class Instrument
    {
        protected Instrument(
            Meter meter,
            string name,
            string? unit,
            string? description,
            IEnumerable<KeyValuePair<string, object?>>? tags);

         public IEnumerable<KeyValuePair<string, object?>>? Tags { get; }
    }

    public abstract partial class Instrument<T> : Instrument where T : struct
    {
        protected Instrument(
            Meter meter,
            string name,
            string? unit,
            string? description, IEnumerable<KeyValuePair<string, object?>>? tags);
    }

    public abstract partial class ObservableInstrument<T> : Instrument where T : struct
    {
        protected ObservableInstrument(
            Meter meter,
            string name,
            string? unit,
            string? description,
            IEnumerable<KeyValuePair<string, object?>> tags);
    }

    public partial class Meter : IDisposable
    {
        public Meter(
            string name,
            string? version,
            IEnumerable<KeyValuePair<string, object?>>? tags,
            object? context = null);

        public IEnumerable<KeyValuePair<string, object?>>? Tags { get ; }}

        public object? Context { get; }

        public Counter<T> CreateCounter<T>(
                            string name,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public UpDownCounter<T> CreateUpDownCounter<T>(
                            string name,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public Histogram<T> CreateHistogram<T>(
                            string name,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableCounter<T> CreateObservableCounter<T>(
                            string name,
                            Func<T> observeValue,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableCounter<T> CreateObservableCounter<T>(
                            string name,
                            Func<Measurement<T>> observeValue,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableCounter<T> CreateObservableCounter<T>(
                            string name,
                            Func<IEnumerable<Measurement<T>>> observeValues,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableUpDownCounter<T> CreateObservableUpDownCounter<T>(
                            string name,
                            Func<T> observeValue,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableUpDownCounter<T> CreateObservableUpDownCounter<T>(
                            string name,
                            Func<Measurement<T>> observeValue,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableUpDownCounter<T> CreateObservableUpDownCounter<T>(
                            string name,
                            Func<IEnumerable<Measurement<T>>> observeValues,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableGauge<T> CreateObservableGauge<T>(
                            string name,
                            Func<T> observeValue,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableGauge<T> CreateObservableGauge<T>(
                            string name,
                            Func<Measurement<T>> observeValue,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;

        public ObservableGauge<T> CreateObservableGauge<T>(
                            string name,
                            Func<IEnumerable<Measurement<T>>> observeValues,
                            string? unit,
                            string? description,
                            IEnumerable<KeyValuePair<string, object?>> tags) where T : struct;
    }

    public sealed class InstrumentRecorder<T> : IDisposable where T : struct
    {
        public InstrumentRecorder(Instrument instrument);
        public InstrumentRecorder(
                        object? contextFilter,
                        string meterName,
                        string instrumentName);
        public InstrumentRecorder(
                        Meter meter,
                        string instrumentName);

        public Instrument Instrument { get; }

        public IEnumerable<Measurement<T>> GetMeasurements(bool clear = false);
        public void Dispose();
    }
}

Needs Work (future discussion):

namespace Microsoft.Extensions.Metrics
{
    public interface IMeterFactory : IDisposable
    {
        Meter Create(
            string name,
            string? version = null,
            IEnumerable<KeyValuePair<string, object?>>? tags = null);
    }

    public static class MetricsServiceExtensions
    {
        public static IServiceCollection AddMetrics(this IServiceCollection services);        
    }
}

[CodeBlanch]

@noahfalk @tarekgh If we are planning to do similar (at some point) to Activity/Source, any concern using the term "context" here?

[tarekgh]

@CodeBlanch yes if we'll do the same pattern. What name do you think will work better?

[CodeBlanch]

Off the top of the head, "state" could work?

[tarekgh]

Yes state was suggested too, context preferred more because state pattern is an object passed to the API and then will be passed back to the callbacks while context is attaching some context to the API. Considering the Activity point, I think the state will be reasonable here.

[davidfowl]

Seems like IMeterFactory is marked as "needs work", is that because of the impending configuration APIs that are paired with this?

[tarekgh]

Seems like IMeterFactory is marked as "needs work", is that because of the impending configuration APIs that are paired with this?

This is about the comment We ran out of time before finishing some issues in IMeterFactory (should the name be Create, GetOrCreate, etc; the ownership and disposal semantics)

[tarekgh]

After careful consideration and offline discussions, we have addressed the remaining open issues related to the proposal. Here is the resolution:

• Regarding the naming of IMeterFactory.Create(), we have decided to keep it as it is, without any changes. This decision ensures consistency with the ILoggerFactory pattern, which also uses the term Create.
• For the issue related to Meter disposing, we have decided to clarify the behavior in the documentation. It will state that the factory owns the lifetime of the Meter, and users should refrain from calling Dispose() explicitly. Considering that meter users typically require the meters throughout the entire lifespan of the process, there is currently no need for them to explicitly dispose of the meters.

In addition to these resolutions, we have a small change in the approved APIs:

• The new Meter constructor currently uses the parameter and property named Context However, we have decided to change it to Scope. This modification aims to avoid any confusion with the term Context in the context of diagnostics, such as trace context. It is especially important since we plan to introduce a similar pattern for tracing in the future (e.g., IActivitySourceFactory).
• The InstrumentRecorder.Instrument property may return a nullable instrument. This change allows for creating the recorder before the instrument itself, accommodating such scenarios.

If there are no further concerns or feedback, we kindly request marking the issue as approved.

[JamesNK]

In my hacked-together version of InstrumentRecorder I stored all raw measurements in an in-memory list. I assume that stays true based on the proposed API.

Is there any concern about adding a type with unconstrained memory growth like this? It's ok in tests with a short lifetime, and memory isn't a huge concern, but what about people who decide to use it in production code?

[tarekgh]

In my hacked-together version of InstrumentRecorder I stored all raw measurements in an in-memory list. I assume that stays true based on the proposed API.

Yes, this stays true.

Is there any concern about adding a type with unconstrained memory growth like this? It's ok in tests with a short lifetime, and memory isn't a huge concern, but what about people who decide to use it in production code?

Note that we added the flag bool clear to the GetMeasurement which allows the callers to clear the previously collected data. This can help the callers control the growth of the memory. If we find any concern later, we can still update the implementation to use file system as back store or similar idea. As you mentioned this type is mostly for testing purposes.

[JamesNK]

Yeah, I saw the flag. That's a good improvement.

If you think docs are enough to explain the potential danger of this type then ok. Just want to double check it's been thought about.

[JamesNK]

namespace System.Diagnostics.Metrics
{
    public interface IMeterFactory : IDisposable
    {
        Meter Create(
                string name,
                string? version = null,
                IEnumerable<KeyValuePair<string, object?>>? tags = null);
    }
}

I worry about future new parameters that a Meter might take. We've just added tags. There may be new parameters in the future. An interface is difficult to version.

Have you considered the Create method taking an options class? For example:

public interface IMeterFactory : IDisposable
{
    Meter Create(MeterOptions options);
}
public sealed class MeterOptions
{
    // name, version, tags, etc
}

Future parameters can easily be added to the options class. Then either have an overload with these parameters or an extension method that calls the IMeterFactory.Create(MeterOptions options) method. Meters aren't created often so the overhead of allocating MeterOptions isn't important.

[noahfalk]

@tarekgh - Is the door still open to incorporate James' MeterOptions feedback? It feels pretty reasonable to me and it was something I was worrying about recently as in the future OpenTelemetry might want to add another constructor parameter like SchemaUrl. Perhaps specifically:

public class Meter
{
    public Meter(MeterOptions options) // in addition to the existing ctors
}

public class MeterOptions
{
    public string Name {get;set;}
    public string? Version {get;set;}
    public IEnumerable<KeyValuePair<string,object?>>? Tags {get;set;}
}
public interface IMeterFactory
{
    Meter Create(MeterOptions options);
}
public static class MeterFactoryExtensions
{
    public static Meter Create(this IMeterFactory, string name, string? version = null, IEnumerable<KeyValuePair<string,object?>> tags = null)
}

The extension method could preserve the current suggested use in code as-is but still leaves the door open to non-breaking additions in the future.

[tarekgh]

@noahfalk @JamesNK

The suggestions sound reasonable to me. One thing I'll add to the MeterOptions is the Scope. I'll edit @noahfalk proposal to accommodate that too.

public class MeterOptions
{
    public string Name { get; set;}
    public string? Version { get; set;}
    public IEnumerable<KeyValuePair<string,object?>>? Tags { get; set; }
    public object? Scope { get; set; }
}

public class Meter
{
        public Meter(string name, string? version, IEnumerable<KeyValuePair<string, object?>>? tags, object? scope = null);    // Was in the original proposal and will stay.
        public Meter(MeterOptions options) // adding new constructor
}

public interface IMeterFactory
{
        // removing: Meter Create(string name, string? version = null, IEnumerable<KeyValuePair<string, object?>>? tags = null);   
        Meter Create(MeterOptions options); // replaced the one in the original proposal
}

public static class MeterFactoryExtensions
{
    // Adding extra extension method helper for creating the meter with flat parameters. 
    public static Meter Create(this IMeterFactory, string name, string? version = null, IEnumerable<KeyValuePair<string,object?>> tags = null,  object? scope=null);
}

• Delta changes from last approved

• We are changing Context name to Scope.

• InstrumentRecorder.Instrument to return nullable value. e.g., Instrument?.