Prometheus Metric and Label naming conventions are inconsistent

[dpozinen]

Since 1.13 the behaviour of has been changed.

For metrics the _counter prefix has been removed, as is well documented and the snake case convention remains:

micrometer/implementations/micrometer-registry-prometheus/src/main/java/io/micrometer/prometheusmetrics/PrometheusNamingConvention.java

Lines 47 to 48 in 39e977d

	public String name(String name, Meter.Type type, @Nullable String baseUnit) {
	String conventionName = NamingConvention.snakeCase.name(name, type, baseUnit);

But for labels snake case has been removed in #4866:

micrometer/implementations/micrometer-registry-prometheus/src/main/java/io/micrometer/prometheusmetrics/PrometheusNamingConvention.java

Lines 87 to 89 in 39e977d

	public String tagKey(String key) {
	return PrometheusNaming.sanitizeLabelName(key);
	}

But even though Prometheus client supports dots in metrics and labels since prometheus/client_java@1a955b1 , it still replaces dots with underscores via https://github.com/prometheus/client_java/blob/5e93f4b2af0321061865bad1f39f531f3d19bd0e/prometheus-metrics-model/src/main/java/io/prometheus/metrics/model/snapshots/Labels.java#L119-L130

For people using PrometheusNamingConvention in their own code the change to label handling is undocumented in the migration guide and actually the java doc for the method has an incorrect regex (without .):

micrometer/implementations/micrometer-registry-prometheus/src/main/java/io/micrometer/prometheusmetrics/PrometheusNamingConvention.java

Lines 82 to 87 in 39e977d

	* Label names may contain ASCII letters, numbers, as well as underscores. They must
	* match the regex [a-zA-Z_][a-zA-Z0-9_]*. Label names beginning with __ are reserved
	* for internal use.
	*/
	@Override
	public String tagKey(String key) {

Now, I am generally confused with the . and _ changes in Prometheus where they are allowed and yet... not allowed really? Maybe you know more about this.

I think it is a good idea to either mention the change regarding labels in PrometheusNamingConvention in the guide and fix the javadoc, or revert the snake case change to align with the way metrics are evaluated. To me the latter is preferable.

Let me know what you think, thanks!

[shakuzen]

Thanks for bringing this to our attention. At a minimum, I think we should update the migration guide to mention the change since that is what may affect users in released versions regardless of what we may change from this point.

I'm curious, though, what is your use case for using the PrometheusNamingConvention outside of PrometheusMeterRegistry? As far as I understand, this isn't an issue with labels in scrape output when registered to PrometheusMeterRegistry. For example, the following test passes on main:

@Test
void tagConventions() {
    Counter.builder("c1").tags("a.b", "c").register(registry);
    Counter.builder("c2").tags("a_b", "c").register(registry);
    assertThat(registry.scrape())
        .contains("c1_total{a_b=\"c\"} 0.0")
        .contains("c2_total{a_b=\"c\"} 0.0");
}

Whereas the following test fails when using PrometheusNamingConvention directly:

@Test
void tagKeyDotToSnakeCase() {
    PrometheusNamingConvention convention = new PrometheusNamingConvention();
    assertThat(convention.tagKey("a.b")).isEqualTo("a_b");
}

[dpozinen]

We have a metrics library which creates a bunch of dashboards+graphs without actually taping in to the registry itself, so you could do something like PromGraph.counter(name).tag("a.b", "c") for a Counter metric like in your example. We applied PrometheusNamingConvention to the tags and names, and so the since the tag would no longer get the replacement of . to _ the graph would not match the exposed metric.

[shakuzen]

Thanks for the details. We'll discuss and figure out what to do about this.

Now, I am generally confused with the . and _ changes in Prometheus where they are allowed and yet... not allowed really? Maybe you know more about this.

I think the intention is to allow passing metric names and label names with dots to the API but to maintain the same output as before by converting dots to underscores, but I don't want to speak for @fstab on behalf of the Prometheus Java client.

[dpozinen]

Great! Thanks for the quick response 🙂

[jonatan-ivanov]

I think I would prefer marking this as a bug and restore the old behavior of PrometheusNamingConvention. We could modify the tag method to do this: PrometheusNaming.sanitizeLabelName(PrometheusNaming.prometheusName(key)) (replace + sanitize).
So if we do this:

io.micrometer.prometheus.PrometheusNamingConvention oldConv = new io.micrometer.prometheus.PrometheusNamingConvention();
io.micrometer.prometheusmetrics.PrometheusNamingConvention newConv = new io.micrometer.prometheusmetrics.PrometheusNamingConvention();

System.out.println("old name: " + oldConv.name("test.name", Type.GAUGE));
System.out.println("new name: " + newConv.name("test.name", Type.GAUGE));

System.out.println("old key: " + oldConv.tagKey("test.key"));
System.out.println("new key: " + newConv.tagKey("test.key"));

The current (inconsistent) output is:

old name: test_name
new name: test_name
old key: test_key
new key: test.key

But with the change, the output will be:

old name: test_name
new name: test_name
old key: test_key
new key: test_key

Likewise, in name, we can replace

String conventionName = NamingConvention.snakeCase.name(name, type, baseUnit);

to

String conventionName = PrometheusNaming.prometheusName(name);