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.

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

metrics-exporter-prometheus should use units set by describe macro. #426

Open
rukai opened this issue Dec 19, 2023 · 1 comment · May be fixed by #535
Open

metrics-exporter-prometheus should use units set by describe macro. #426

rukai opened this issue Dec 19, 2023 · 1 comment · May be fixed by #535
Labels
C-exporter Component: exporters such as Prometheus, TCP, etc. E-intermediate Effort: intermediate. T-ergonomics Type: ergonomics. T-request Type: request.

Comments

@rukai
Copy link
Contributor

rukai commented Dec 19, 2023

Feature request 1

The describe_*! macros set a unit for each metric but metrics-exporter-prometheus does not currently make use of it.
The prometheus protocol itself does not provide a way to set a unit however it is documented that the correct way to handle units in prometheus is to postfix them to the end of the metric name.

So I propose that given a metric like:

describe_counter!("connections", Unit::Count, "The total count of opened connections")
register_counter!("connections"),

It will export as:

# HELP connections The total count of opened connections
# TYPE connections counter
connections_count{} 1

This will allow this library to be truly generic across different metrics protocols, as otherwise the user will need to duplicate unit information in other protocols in order to properly use prometheus.

This feature could be disabled in the prometheus builder if there are cases where the user only uses prometheus and needs greater control.

Feature request 2

Additionally that same page mentions that applications should prefix metrics with the name of the application.
So maybe we should also have an optional setting in the prometheus builder to set the application name such that the previous export will now export as:

# HELP connections The total count of opened connections
# TYPE connections counter
application_connections_count{} 1

This follows the same reasoning as the previous feature, other protocols may not include the application name at the beginning of each name resulting in redundant information.

@tobz
Copy link
Member

tobz commented Dec 23, 2023

Using the units that are captured in description information

In terms of using the specified units, the whole description API sort of needs a rework, mostly due to how rigid it currently is... but your point about following the Prometheus specification/recommendations and actually using it is sound. I would accept a PR for that.

Prefixing metric names

As far as prefixing, that's also fair although the aforementioned Prometheus documentation, it mentions not only that it should be prefixed (versus must) but also that you might not want a prefix across all metrics your application emits, specifically if client libraries are emitting their own.

This is most definitely the case with the overall model that metrics takes, so it could be a bit weird/repetitive to apply a prefix to all metrics, and then end up with something like myapp_hyper_.... There'd probably need to be a little more thought put into this, because it's not trivial to only prefix metrics from the "application" vs dependencies... and users can already take advantage of the prefix layer to all a prefix to all metrics uniformly if they so desire.

@tobz tobz added C-exporter Component: exporters such as Prometheus, TCP, etc. E-intermediate Effort: intermediate. T-ergonomics Type: ergonomics. T-request Type: request. labels Dec 23, 2023
@fayalalebrun fayalalebrun linked a pull request Oct 18, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-exporter Component: exporters such as Prometheus, TCP, etc. E-intermediate Effort: intermediate. T-ergonomics Type: ergonomics. T-request Type: request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants