MeterProviderSdk should throw when no meters added.

[Yun-Ting]

MeterProviderSdk contract change:
Introducing a hard check by throwing Argument null exception for the case that no meter was added to the provider.
And fixed some existing metrics tests.

Addressing #2474 (comment)

[reyang]

Curious - do we do the same for traces (when no ActivitySource / legacy source are added)? (and why we want to do it for metrics?)

[cijothomas]

we should be doing for traces as well. We did not originally did the check for ActivitySources, as it was possible to add instrumentations which did "legacy" activities using some adapter. So it made sense for TracerProvide to exist even if no ActivitySource was present. Later we added native legacyactivity support, so if both are empty, then no need for setting up the SDK.
For metrics, there is no purpose served by having a MeterProvider, if no Meters are added, and is likely a user forgetting to add meters.

[utpilla]

nit: With this change you can remove the check before creating meterSourcesToSubscribe:

else if (meterSources.Any())
{
    var meterSourcesToSubscribe = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
    ...
}

[Yun-Ting]

we should be doing for traces as well. We did not originally did the check for ActivitySources, as it was possible to add instrumentations which did "legacy" activities using some adapter. So it made sense for TracerProvide to exist even if no ActivitySource was present. Later we added native legacyactivity support, so if both are empty, then no need for setting up the SDK. For metrics, there is no purpose served by having a MeterProvider, if no Meters are added, and is likely a user forgetting to add meters.

Thank you Cijo for the detailed explanation.
I've also fixed some of the existing metrics test cases by adding a meter after introducing this new check.

[reyang]

For metrics, there is no purpose served by having a MeterProvider, if no Meters are added, and is likely a user forgetting to add meters.

I'm concerned about this part. I understand that if the user forgot to add any Meter, it might be a mistake - and if that's the case we might want to fail fast and let them know. I think there are other cases folks want to comment out the meter just to disable telemetry collection, for troubleshooting or scoping down a performance problem.

 using var meterProvider = Sdk.CreateMeterProviderBuilder()
    /* .AddMeter("MyCompany.MyProduct.MyLibrary") */
    .AddReader(new PeriodicExportingMetricReader(new MyExporter(), 1000))
    .Build();

...

meterProvider.ForceFlush(5000);

...

meterProvider.Shutdown();

If we let the Build() throw, the user might need to hack to code because it's hard to replace all the other places where meterProvider methods are used. So I imagine they would put something like:

 using var meterProvider = Sdk.CreateMeterProviderBuilder()
    .AddMeter("I.Do.Not.Know.Why.I.Have.To.Do.This.But.Anyways")
    .AddReader(new PeriodicExportingMetricReader(new MyExporter(), 1000))
    .Build();

[alanwest]

The development troubleshooting use case did not immediately come to mind, but I agree it's important.

I can definitely see myself doing something like this 😄.
.AddMeter("I.Know.Why.I.Have.To.Do.This.But.Id.Prefer.Not.To.Have.To")

That said, let's say someone mistakingly creates a MeterProvider with no Meters in production, I can see it being desirable from a production troubleshooting scenario to throw so that I can narrow down the problem quickly...

Might be nice to throw here when SomeListOfMetersReadDynamicallyAtStartUp turns out to be empty in production.

 using var meterProviderBuilder = Sdk.CreateMeterProviderBuilder();

foreach (var meter in SomeListOfMetersReadDynamicallyAtStartUp)
{
    meterProviderBuilder.AddMeter(thing);
}

using var meterProvider = meterProviderBuilder
    .AddReader(new PeriodicExportingMetricReader(new MyExporter(), 1000))
    .Build();

[Yun-Ting]

I agree on the above mentioning examples of cases that users do not need the meter but have to add meter to get around this hard check. Tests around HostingMeterExtensionTests.cs are other examples, too.
I will leave the decision of whether to add meter our not to the user to prevent having to do it just to get around this hard check.

[cijothomas]

I was about to say that fail-fasting when user forgot to add any Meter was more important than the awkwardness of AddMeter("I.Do.Not.Know.Why.I.Have.To.Do.This.But.Anyways"), but based on our feedbacks so far - the issue is typically not with user forgetting to add any source/meter, but its missing to add the one they care about. So by throwing if no source/meter added, we won't help that more-common use case.

Agree to leave this as is.

[reyang]

Another thing to consider, in the future we might have lots of instrumentation libraries and the user might have:

using var meterProvider = Sdk.CreateMeterProviderBuilder()
    .AddFooInstrumentation()
    .AddBarInstrumentation()
    .AddBazInstrumentation()
    .AddReader(new PeriodicExportingMetricReader(new MyExporter(), 1000))
    .Build();

[codecov]

Codecov Report

Merging #2476 (6cb1863) into main (4bc68a7) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 6cb1863 differs from pull request most recent head 23cf66e. Consider uploading reports for the commit 23cf66e to get more accurate results

@@            Coverage Diff             @@
##             main    #2476      +/-   ##
==========================================
+ Coverage   79.83%   79.85%   +0.02%     
==========================================
  Files         254      254              
  Lines        8395     8394       -1     
==========================================
+ Hits         6702     6703       +1     
+ Misses       1693     1691       -2     

Impacted Files	Coverage Δ
src/OpenTelemetry/Metrics/MeterProviderSdk.cs	92.14% <100.00%> (+0.03%)	⬆️
...ensions.Hosting/OpenTelemetryServicesExtensions.cs	61.76% <0.00%> (-2.95%)	⬇️
...metry/Trace/BatchExportActivityProcessorOptions.cs	81.81% <0.00%> (-1.52%)	⬇️
src/OpenTelemetry/Metrics/AggregatorStore.cs	85.60% <0.00%> (+0.75%)	⬆️
src/OpenTelemetry.Api/BaseProvider.cs	100.00% <0.00%> (+33.33%)	⬆️

[Yun-Ting]

Decided not to add the hard check based on the discussions.