Remove Meter<T>

[noahfalk]

dotnet/extensions defines a Meter<T> type that doesn't work with the newly introduced MeterFactory and doesn't align with guidance we plan to give users for writing metrics in a DI-aware scenario. We should remove this type.

[geeknoid]

@dpk83 Can you provide a quick rationale for keeping Meter? I believe it's mostly about usability, reducing the ceremony for customers.

@noahfalk Can you provide a quick explanation why it doesn't work with MeterFactory? I believe it's related to instrument reference?

[dpk83]

@dpk83 Can you provide a quick rationale for keeping Meter? I believe it's mostly about usability, reducing the ceremony for customers.

• Simpler way to inject Meter of T. One step process Meter instead of 2 step process inject IMeterFactory and then IMeterFactory.Create
• Less prone to errors in naming meters. Meter will pick up T's name, if developer needs to inject IMeterFactory and then create meter on their own, some of them might end up creating meter with custom names instead IMeterFactory.Create("meterName"). So for organizations/services then wants to ensure that T is used for meter name, need to build something additional to ensure the naming consistency

[noahfalk]

Can you provide a quick explanation why it doesn't work with MeterFactory?

There is Meter<T> as it is currently implemented in dotnet/extensions, and then the less simple explanation that tries to anticipate possible alternate implementations of Meter<T> and what problems arise there.

As currently implemented Meter<T> does not populate the new scope parameter of the Meter constructor. This parameter is used to distinguish Meters in different DI containers. For example this pattern we planned to tell users to do would not work:

// assume we have some type created in DI with constructor: MyWidget(Meter<MyWidget> meter)
var factory = services.GetRequiredService<IMeterFactory>();
var recorder = new InstrumentRecorder(factory, "MyWidget", "MyCounter");
var widget = services.GetRequiredService<MyWidget>();

// act
widget.DoSomething();

// assert
Assert(1, recorder.GetMeasurements().Count); // this fails because the MyWidget meter isn't associated
                                             // with the factory

There has also been some vocal feedback from folks who do not like particular designs, including opposition to Meter<T> on principle. Regardless of anyone's specific position that Meter<T> is a good or bad design choice, discussion on that topic ended when we said we were no longer pursuing it on the public issue. If we reverse our earlier decision, its reasonable that folks who were opposed to it before would expect opportunity for debate would resume.

Another issue is that Meter<T> doesn't handle cases where users want to specify any other Meter constructor argument like version or tags. Switching between injecting the factory or the Meter<T> is of course doable, but complicates the guidance and isn't easily discoverable without resorting to docs.

And then a bunch of hypotheticals:

• What if users passed null instead of factory to the recorder? In isolation the unit test would work, but run in parallel it would fail because the Meters aren't being associated with the unique DI container in each test.
• What if we changed Meter<T> to look like public Meter<T>(IMeterFactory factory) : Meter(..., scope:factory)?
  The test passes, but we didn't ever call IMeterFactory.Create() to create these Meters which defies user's expectations of what it means to have a factory.
• What if we do what ILogger<T> does and create a 2nd inner type that is constructed by the factory?
  Now unit tests that test by factory+name pass, but unit tests that identify the Meter<T> by reference fail because there is a difference in object identity between the Meter that was passed to constructor and the Meter that was used for recording measurements. It also has very counter-intuitive behavior where meter.CreateCounter().Meter != meter.

Maybe the right clever design can solve all the concerns or we could start suggesting broader changes that replace IMeterFactory with something else but the biggest issue is that we don't have time to play that process out. IMeterFactory, Meter<T> and IMeter<T> were already considered and resolved during a 4 month period of design and feedback. Its certainly unfortunate if the impact of the decisions being made there weren't clear and it feels like something went awry in the review or communication that we need to improve on. However I don't see any path that brings in Meter<T> without effectively resetting the discussion back to where we were in February and starting again.

[joperezr]

@noahfalk @geeknoid Is there something we still want to do for extensions here?

[xakep139]

@klauco did we agree on removing Meter<T>?

[xakep139]

Done via #4342