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

Fix OpenMetrics valid label keys, and specify prometheus conversion for metric name. #2788

Merged
merged 4 commits into from
Sep 19, 2022

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Sep 12, 2022

Changes

This fixes an incorrect statement about the allowed characters in prometheus label keys found here: open-telemetry/opentelemetry-go#3135 (comment).

During the process, I also realized we don't specify conversion rules for the metric name, including the rules for sanitizing the metric name when converting from OTLP to Prom.

Source: https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#abnf

In that spec, metricname-initial-char allows :, but label-name-initial-char does not.

@dashpole
Copy link
Contributor Author

cc @open-telemetry/wg-prometheus

@jmacd
Copy link
Contributor

jmacd commented Sep 16, 2022

@dashpole related question, maybe not answered in this PR:
What is prescribed for empty label keys?
The specification merely requires valid Unicode sequences. I think I would prefer it said non-empty sequences, but I'm not sure what SDKs should do in this case. What do you think?

@dashpole
Copy link
Contributor Author

For Prometheus/OpenMetrics, empty label keys aren't allowed. We should drop + warn for an empty label. For exporters written using prometheus client libraries, that is already done by the client's validation (e.g. prometheus/client_golang). It might be good to include a catchall statement somewhere to the effect of: "exporters should produce valid prometheus/openmetrics metrics, or MUST drop the metric and emit a warning".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants