Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MeterProviderSdk should throw when no meters added. #2476

Closed
wants to merge 10 commits into from
+24 −15
Closed

MeterProviderSdk should throw when no meters added. #2476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
52dc054
MeterProvider should throw when no meters added.
Yun-Ting Oct 12, 2021
553f8b4
case
Yun-Ting Oct 12, 2021
5289cb7
case
Yun-Ting Oct 12, 2021
9e023d4
Revert "case"
Yun-Ting Oct 12, 2021
8a7c4eb
comment
Yun-Ting Oct 12, 2021
6fcaf0c
Oops
Yun-Ting Oct 12, 2021
6cb1863
fixed tests
Yun-Ting Oct 12, 2021
f02d689
Trigger CI builds
Yun-Ting Oct 12, 2021
0bb6dd1
Trigger CI builds
Yun-Ting Oct 12, 2021
23cf66e
one more test fix
Yun-Ting Oct 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/OpenTelemetry/Metrics/MeterProviderSdk.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ internal MeterProviderSdk(

AggregationTemporality temporality = AggregationTemporality.Cumulative;

if (meterSources.Count() == 0)
{
throw new ArgumentException("No meter was added to the sdk.");
Copy link
Member

@reyang reyang Oct 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@utpilla utpilla Oct 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

@Yun-Ting Yun-Ting Oct 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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();

Copy link
Member

Choose a reason for hiding this comment

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

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();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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();

}

foreach (var reader in readers)
{
if (reader == null)
Expand Down Expand Up @@ -94,7 +99,7 @@ internal MeterProviderSdk(
var regex = GetWildcardRegex(meterSources);
shouldListenTo = instrument => regex.IsMatch(instrument.Meter.Name);
}
else if (meterSources.Any())
else
{
var meterSourcesToSubscribe = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
foreach (var meterSource in meterSources)
Expand Down
18 changes: 13 additions & 5 deletions test/OpenTelemetry.Extensions.Hosting.Tests/HostingMeterExtensionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ public async Task AddOpenTelemetryMeterProviderInstrumentationCreationAndDisposa
{
services.AddOpenTelemetryMetrics(builder =>
{
builder.AddInstrumentation(() =>
builder
.AddMeter("SomeCompany.SomeProduct.SomeComponent")
.AddInstrumentation(() =>
{
callbackRun = true;
return testInstrumentation;
Expand Down Expand Up @@ -71,7 +73,7 @@ public void AddOpenTelemetryMeterProvider_HostBuilt_OpenTelemetrySdk_RegisteredA
{
var builder = new HostBuilder().ConfigureServices(services =>
{
services.AddOpenTelemetryMetrics();
services.AddOpenTelemetryMetrics(meterSdk => meterSdk.AddMeter("SomeCompany.SomeProduct.SomeComponent"));
});

var host = builder.Build();
Expand All @@ -91,8 +93,10 @@ public void AddOpenTelemetryMeterProvider_ServiceProviderArgument_ServicesRegist
services.AddSingleton(testInstrumentation);
services.AddOpenTelemetryMetrics(builder =>
{
builder.Configure(
(sp, b) => b.AddInstrumentation(() => sp.GetRequiredService<TestInstrumentation>()));
builder
.AddMeter("SomeCompany.SomeProduct.SomeComponent")
.Configure(
(sp, b) => b.AddInstrumentation(() => sp.GetRequiredService<TestInstrumentation>()));
});

var serviceProvider = services.BuildServiceProvider();
Expand Down Expand Up @@ -139,6 +143,7 @@ public void AddOpenTelemetryMeterProvider_NestedConfigureCallbacks()
int configureCalls = 0;
var services = new ServiceCollection();
services.AddOpenTelemetryMetrics(builder => builder
.AddMeter("SomeCompany.SomeProduct.SomeComponent")
.Configure((sp1, builder1) =>
{
configureCalls++;
Expand All @@ -164,6 +169,7 @@ public void AddOpenTelemetryMeterProvider_ConfigureCallbacksUsingExtensions()
services.AddSingleton<TestReader>();

services.AddOpenTelemetryMetrics(builder => builder
.AddMeter("SomeCompany.SomeProduct.SomeComponent")
.Configure((sp1, builder1) =>
{
builder1
Expand Down Expand Up @@ -214,7 +220,9 @@ private static MeterProviderBuilder AddMyFeature(MeterProviderBuilder meterProvi
(meterProviderBuilder.GetServices() ?? throw new NotSupportedException("MyFeature requires a hosting MeterProviderBuilder instance."))
.AddSingleton<TestReader>();

return meterProviderBuilder.AddReader<TestReader>();
return meterProviderBuilder
.AddMeter("SomeCompany.SomeProject.SomeComponent")
.AddReader<TestReader>();
}

internal class TestInstrumentation : IDisposable
Expand Down
1 change: 1 addition & 0 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ void ProcessExport(Batch<Metric> batch)
};
this.meterProvider = Sdk.CreateMeterProviderBuilder()
.AddAspNetCoreInstrumentation()
.AddMeter("SomeCompany.SomeProject.SomeComponent")
.AddReader(metricReader)
.Build();

Expand Down
12 changes: 3 additions & 9 deletions test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ public void MeterSourcesWildcardSupportMatchTest(bool hasView)
[Theory]
[InlineData(true)]
[InlineData(false)]
public void MeterSourcesWildcardSupportNegativeTestNoMeterAdded(bool hasView)
public void SdkCreationShouldThrowWithNoMeterAdded(bool hasView)
{
var meterNames = new[]
{
Expand All @@ -251,14 +251,8 @@ public void MeterSourcesWildcardSupportNegativeTestNoMeterAdded(bool hasView)
meterProviderBuilder.AddView("gauge1", "renamed");
}

using var meterProvider = meterProviderBuilder.Build();
var measurement = new Measurement<int>(100, new("name", "apple"), new("color", "red"));

meter1.CreateObservableGauge("myGauge1", () => measurement);
meter2.CreateObservableGauge("myGauge2", () => measurement);

meterProvider.ForceFlush(MaxTimeToAllowForFlush);
Assert.True(exportedItems.Count == 0);
var ex = Assert.Throws<ArgumentException>(() => meterProviderBuilder.Build());
Assert.Equal("No meter was added to the sdk.", ex.Message);
}

[Theory]
Expand Down
1 change: 1 addition & 0 deletions test/OpenTelemetry.Tests/Metrics/MetricExporterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ public void FlushMetricExporterTest(ExportModes mode)
var reader = new BaseExportingMetricReader(exporter);
using var meterProvider = Sdk.CreateMeterProviderBuilder()
.AddReader(reader)
.AddMeter("SomeCompany.someProduct.SomeComponent")
.Build();

switch (mode)
Expand Down