Why can 'System.Diagnostics.Metrics' instruments be created with invalid names as per the OpenTelemetry spec?

[julealgon]

I just spent over 5h debugging my API application due to the fact my custom metrics were not being output on the exporters (console and OTLP): turned out my counter names had spaces in them.

Why does the Meter's CreateCounter<T> method not validate the name of the instrument to see if it matches OpenTelemetry specs to avoid this whole problem at the root?

I created a whole sample app and then compared the sample to my API to understand why the sample was working and the API was not... only to test this difference hours later: the counter used in the sample used a - as the word separator in the name, while my counter used a space.

I was unaware of the whole internal logging in the library, so I thought I was doing something wrong in the implementation. After finding out that changing the space to a - solved the problem, I was ready to open an issue in here as a bug, only to find out by looking at the implementation that there is an explicit regex check on the instrument name, and all instruments that have non-compliant names are ignored and logged... that's when I found out the library logged to EventSource and then I found out about the self-diagnostic thing with the json config file. After enabling that, I could finally confirm that the issue I was having was indeed the counter name, as the counter being ignored on load was logged there.

This journey was not a lot of fun, to say the least. I understand that I can't really open a bug on the library now, as this was all "working as intended", however I don't understand why you'd even allow one to create an instrument with an invalid name in the first place. Why not restrict the name rules at creation time? Throw an ArgumentException for the name and explain there it needs to follow a few rules.

These days, I expect to take a dependency on a library and start using it intuitively, but that's basically just impossible to do with OpenTelemetry integration. The very fact it logs using EventSource is baffling to me: couldn't you log using ILogger like AspNetCore itself does?

[cijothomas]

Within OTel, instrument creation is not considered a Startup thing. i.e Instruments can be created any time in the application lifetime. And the general principle in OpenTelemetry, is NOT to throw exceptions at runtime. (The only time OTel will throw exception is at the startup i.e when building the OTel providers, and for any other time - only option is to log and move on..)

Also, " Meter's CreateCounter" , is part of the .NET's runtime (https://github.com/dotnet/runtime), and it can be used even outside of OpenTelemetry. So it is unlikely that it can do validations based on OpenTelemetry rules and throw.

I think it'd be good to point users to the troubleshooting guide right from the getting-started guides, so it is easier to quickly root cause these issues. Do you agree this doc fix is a good solution, given the limitations I shared above?

(Of course, it won't help you now 😆 as you already figured out... but for future users?)

[cijothomas]

We had an open PR to integrate ILogger logging, when used in conjunction with OTel.Extensions.Hosting package. It did not get merged, but something to be considered as future improvements.

[julealgon]

Thanks for the link @cijothomas , that's an interesting topic, I'll read it in detail later. For now, I do still think it is a massive mistake not to go with ILogger by default here, so I hope it gets integrated at some point.

[cijothomas]

Finally found the original discussions initiated by @ejsmith #1258, who also did the PR to add this to Hosting package.

[ejsmith]

Yeah, #1867 was "Closed as inactive". Even though it's been extremely useful to me trying to get everything working and reporting correctly.

[cijothomas]

Now that the main goal of shipping Metrics is done, we'll come back to all pending issues. https://github.com/open-telemetry/opentelemetry-dotnet/milestones Ext.Hosting is a top ask, and ILogger support inside it (the "inactive" PR) should be revisited then :)