dotnet / dotnet-monitor

This repository contains the source code for .NET Monitor - a tool that allows you to gather diagnostic data from running applications using HTTP endpoints
MIT License
642 stars 113 forks source link

dotnet-monitor Bad Prometheus Metric Names for S.D.Metrics #7647

Open pinkfloydx33 opened 2 weeks ago

pinkfloydx33 commented 2 weeks ago

Description

When configured to export a Meter from System.Diagnostics.Metics , the Prometheus metrics names reported via /metrics are a bit off. The metrics end up with trailing underscores and also groups of double underscores. This does not match the behavior of exporting Event Counters, nor does it match most metrics I've seen exported in other Prometheus-based systems. Also, dotnet-monitor is appending the units of the Instrument which I believe is incorrect as metrics following Otel/Prometheus conventions should already include that portion (in non-abbreviated form) in their names.

Here are some of the metric names being reported:

For what it's worth, here's how Prometheus.NET reports these same metrics:

As you can see, the units field isn't included in the name at all. They just concatenate the instrument and meter name before normalizing the remaining characters.

This isn't necessarily a bug in dot-monitor; it's creating metric names based on a specific algorithm and the offending source metrics seem to be following a pattern for the unit field (the UCUM standard) that dotnet-monitor does not recognize (see below). That said I think there's room for improvement here as these names don't conform to most you'll see (or that I've seen) when working with Prometheus. To boot, the metric names in my examples already include the units in appropriate pluralized form, so you end up with double units in the name where the ones appended here are singular (and may be abbreviated, ex. s vs seconds which would be in the name already)

I think that dotnet-monitor should only use the units field of an Instrument in the HELP section of the export, and not when building the metric names. Anyone that has been following open-telemetry and Prometheus naming conventions is already naming their Meter/Instruments correctly. At the very least the runtime is which should be a big flag here.

I realize that changing metric names can break anyone already relying them. Perhaps an option could be added to indicate whether or not the units should be appended to the name.

Configuration

Other information

In the case of the first three items above, the units are being reported as {request}, {connection} and {command} (respectively) and the non-alphanumeric characters are being converted to _. Apparently this is how npgsql and even the runtime reports some of it's units, which tells me that this is some sort of convention that should probably be recognized by the Prometheus exporter (assuming it keeps appending the units). EDIT: this is the "UCUM" standard and is reflected in dotnet's "best practices" for Metrics see here

In the case of the fourth item, the unit of measure is 1 which I think makes sense in this case, yet the exporter drops it

github-actions[bot] commented 2 weeks ago

Welcome to dotnet-monitor!

Thanks for creating your first issue; let us know what you think of dotnet-monitor by filling out our survey.