-
Notifications
You must be signed in to change notification settings - Fork 819
avoid the breaking change in PrometheusNaming #1561
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
...etheus-metrics-core/src/test/java/io/prometheus/metrics/core/metrics/CallbackMetricTest.java
Outdated
Show resolved
Hide resolved
* replaced with underscores in Prometheus exposition formats. However, if metrics are exposed in | ||
* OpenTelemetry format the dots are retained. | ||
*/ | ||
public class PrometheusNames { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a name we could use that has a more drastic contrast to PrometheusNaming
? Or maybe even PrometheusNamingV2
? while reviewing I got confused as to which is which a few times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah - that seems to be the hardest part
- PrometheusUnicodeNaming
- same name but different package
- sanitizer interface:
PrometheusNaming.UNICODE.sanitize(..)
- would also work if we want to have a variant that allows
_total
suffix in combination with add option to disable metric suffixes #1557
- would also work if we want to have a variant that allows
} | ||
} | ||
|
||
public static boolean needsEscaping(String name, EscapingScheme scheme) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just want to double check that all the public methods in this class are intentionally public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch - and I also moved it to NameEscaper
I'm a bit hesitant to add internal classes now - since it would be potentially a breaking change
Alternatively, only new classes as internal classes - but that might make it look like the other classes are all public - which they are not.
...s-metrics-model/src/test/java/io/prometheus/metrics/model/snapshots/PrometheusNamesTest.java
Outdated
Show resolved
Hide resolved
…odel/snapshots/PrometheusNamesTest.java Co-authored-by: Jay DeLuca <jaydeluca4@gmail.com> Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Fixes #1548
PrometheusNaming changes were incompatible - so deprecating instead and create e new PrometheusNames