micrometer icon indicating copy to clipboard operation
micrometer copied to clipboard

Document the behavior and expectations of NamingConvention

Open sysmat opened this issue 3 years ago • 31 comments

Describe the bug Prometheus label is not snake case

Environment

  • spring-boot-starter-parent:2.6.5
  • micrometer-registry-prometheus: 1.8.4
  • spring-boot-starter-actuator:2.6.5
  • even when setting
@Bean
MeterRegistryCustomizer<PrometheusMeterRegistry> metricsCommonTags() {
        return registry -> registry.config()
                .namingConvention(NamingConvention.snakeCase)
                .commonTags("env", config.getMode().name());
    }
  • OS: WIN, Linux
  • Java version: 8, 11, 17

To Reproduce

management.endpoint.metrics.enabled=true
management.endpoints.web.exposure.include=*
management.endpoint.prometheus.enabled=true
management.metrics.export.prometheus.enabled=true

Expected behavior Prometheus parser fail for label(tag) main-application-class it should be main_application_class

Additional context

# TYPE application_ready_time gauge
application_ready_time{env="DEVELOPMENT",main-application-class="si.app.myApplication",} 2.489
# TYPE application_started_time gauge
application_started_time{env="DEVELOPMENT",main-application-class="si.app.myApplication",} 2.445

sysmat avatar Mar 31 '22 14:03 sysmat

I think the naming convention only applies to dot notation separation, not hyphens

You mention 'name and labels', I only see the label being mentioned. Did I overlook a name example?

checketts avatar Mar 31 '22 19:03 checketts

If I do this:

PrometheusMeterRegistry registry = new PrometheusMeterRegistry(PrometheusConfig.DEFAULT);
Counter.builder("test.counter")
        .tag("test.tag", "0")
        .register(registry)
        .increment();

System.out.println(registry.scrape());

I get this:

# HELP test_counter_total  
# TYPE test_counter_total counter
test_counter_total{test_tag="0",} 1.0

You need to use the dot notation when you name your meters and tags otherwise Micrometer will not apply the naming convention and take it verbatim. See the docs: https://micrometer.io/docs/concepts#_naming_meters

jonatan-ivanov avatar Mar 31 '22 22:03 jonatan-ivanov

  • here is a definition: https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels
  • @jonatan-ivanov I'm not building metrics, it is spring boot actuator + micrometer-registry-prometheus

sysmat avatar Apr 01 '22 05:04 sysmat

@jonatan-ivanov @checketts In repo you have PrometheusNamingConvention which is for name of metric and name of labels aka tags(tagKey, name func)

sysmat avatar Apr 01 '22 05:04 sysmat

so if PrometheusMeterRegistry is an exporter from actuator then it should follow https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels

sysmat avatar Apr 01 '22 05:04 sysmat

I'm not sure I understand what you are trying to say.

jonatan-ivanov avatar Apr 01 '22 05:04 jonatan-ivanov

  • ok, it is bug in spring-boot-starter-parent:2.6.5, with actuator:2.6.5 and micrometer-registry-prometheus:1.8.4
  • in spring-boot-starter-parent:2.6.4 with actuator:2.6.4 and micrometer-registry-prometheus:1.8.3 it works

application_started_time_seconds{env="DEVELOPMENT",main_application_class="si.app.myApplication",} 12.755

sysmat avatar Apr 01 '22 05:04 sysmat

https://github.com/spring-projects/spring-boot/issues/30497

sysmat avatar Apr 01 '22 05:04 sysmat

spring-boot-starter-parent:2.6.5 and micrometer-registry-prometheus:1.8.3 it works

sysmat avatar Apr 01 '22 06:04 sysmat

from spring-boot, they are saying the bug is in micrometer-registry-prometheus:1.8.4

sysmat avatar Apr 01 '22 09:04 sysmat

it is very bad in production because not valid Prometheus label Prometheus scraper will not save any metric in DB

sysmat avatar Apr 01 '22 12:04 sysmat

Ok. So you are saying with 1.8.3 it was working for you and with 1.8.4 it is now writing out names with hyphens instead of snake case?

checketts avatar Apr 01 '22 12:04 checketts

exectly that

sysmat avatar Apr 01 '22 12:04 sysmat

not every metric name or label(tag), only main-application-class="si.app.myApplication"

sysmat avatar Apr 01 '22 12:04 sysmat

ok, when using MeterRegistryCustomizer<MeterRegistry> to add commonTags, then main-application-class appears

sysmat avatar Apr 01 '22 13:04 sysmat

ok, when using namingConvention and commonTags , then main-application-class appears

@Bean
MeterRegistryCustomizer<MeterRegistry> metricsCommonTags() {
      return registry -> registry.config()
                                    .namingConvention(NamingConvention.snakeCase)
                                   .commonTags("env", "DEVELOPMENT");
}

sysmat avatar Apr 01 '22 13:04 sysmat

  • even if I use spring conf management.metrics.tags.env=moj test
@Bean
    MeterRegistryCustomizer<PrometheusMeterRegistry> metricsCommonTags() {
        return registry -> registry.config()
                                   .namingConvention(NamingConvention.snakeCase);                                  
    }
  • main-application-class appears in metric

sysmat avatar Apr 01 '22 13:04 sysmat

  • demo springboot, java 11, start main or jar
  • hit url: http://localhost:8080/actuator/prometheus
  • you will see: application_started_time{env="moj test",main-application-class="com.example.demo.DemoApplication",} 1.262

demo.zip

sysmat avatar Apr 01 '22 13:04 sysmat

Thanks for that demo. It is a bug in SpringBoot actuator: https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/startup/StartupTimeMetricsListener.java#L126

I didn't debug into why adding the customizer breaks it for you though.

That shouldn't break the Prometheus ingestion though (just any queries you were expecting it)

checketts avatar Apr 01 '22 14:04 checketts

So you are saying with 1.8.3 it was working for you and with 1.8.4 it is now writing out names with hyphens instead of snake case?

FWIW, I tried to sample with both Micrometer 1.8.3 and 1.8.4 and I didn't see any change in behaviour. The sample produced main-application-class in both cases.

It is a bug in SpringBoot actuator

I don't believe it is. AIUI, it's the naming convention's job to make the tag keys compatible. NamingConvention.snakeCase converts . to _ but leaves - unchanged, in other words it doesn't handle kebab-case to snake_case conversion. Arguably, this could be a bug in Micrometer's snake case naming convention.

I didn't debug into why adding the customizer breaks it for you though.

It replaces PrometheusNamingConvention with NamingConvention.snakeCase so the kebab-case to snake_case conversion is lost. If PrometheusMeterRegistry is left in its default configuration so that PrometheusNamingConvention is used, the main-application-class tag key is correctly converted to main_application_class.

wilkinsona avatar Apr 02 '22 09:04 wilkinsona

  • From spring they still think something is wrong in micrometer, ref: https://github.com/spring-projects/spring-boot/issues/30504#issuecomment-1086602322

sysmat avatar Apr 02 '22 13:04 sysmat

  • even with that and a common tag in conf(management.metrics.tags.env=moj test) output is still kebab-case for main-application-class
@Bean
    MeterRegistryCustomizer<PrometheusMeterRegistry> metricsCommonTags() {
        return registry -> registry.config()
                                   .namingConvention(PrometheusNamingConvention.snakeCase);
    }
  • this is very use case with springboot >= 2.6.5, but works with springboot <= 2.6.4

sysmat avatar Apr 02 '22 13:04 sysmat

is there any way with application configuration to add a prefix to each metric?

sysmat avatar Apr 02 '22 14:04 sysmat

That's not the behaviour that I see with your sample with the following MeterConfiguration:

@Configuration
public class MetricConfiguration {

    @Bean
    MeterRegistryCustomizer<PrometheusMeterRegistry> metricsCommonTags() {
        return registry -> registry.config()
                                   .namingConvention(new PrometheusNamingConvention());
    }

}
# HELP application_started_time_seconds Time taken (ms) to start the application
# TYPE application_started_time_seconds gauge
application_started_time_seconds{env="moj test",main_application_class="com.example.demo.DemoApplication",} 2.029

Can you provide an updated sample that reproduces the behaviour you have described when using PrometheusNamingConvention?

wilkinsona avatar Apr 02 '22 15:04 wilkinsona

is there any way with application configuration to add a prefix to each metric?

You've already asked this in Spring Boot's issue tracker. Please don't ask the same question in multiple places. It risks wasting the time of those trying to help you.

wilkinsona avatar Apr 02 '22 15:04 wilkinsona

@wilkinsona thx

sysmat avatar Apr 02 '22 17:04 sysmat

AIUI, it's the naming convention's job to make the tag keys compatible. NamingConvention.snakeCase converts . to _ but leaves - unchanged, in other words it doesn't handle kebab-case to snake_case conversion. Arguably, this could be a bug in Micrometer's snake case naming convention.

In general, Micrometer expects its canonical naming (dot case) to be used for NamingConvention implementations to have the expected conversion. We should improve (have at all) JavaDocs to make this point, but it was the intention for it to be conveyed in this section of the reference documentation. How parts of input that are not dot case will be converted depends on the implementation.

NamingConvention.snakeCase is a general implementation not specific to any backend, and therefore it only handles converting from canonical form (dot case) to snake case. It doesn't do anything with - (or other special characters), and what should be done depends on the specific backend, requiring a more specific implementation if the backend has limitations or preferences. A metrics backend other than Prometheus may be fine with one_two-words_one as a name from the input one.two-words.one. PrometheusNamingConvention can be more specific, and Prometheus doesn't allow many characters - so it is by chance that kebab case also gets converted, because - is not an allowed character for Prometheus names and the implementation converts all disallowed characters to _.

The problem here is that NamingConvention.snakeCase should not be used with PrometheusMeterRegistry. There's no need to configure the NamingConvention since the default for PrometheusMeterRegistry is PrometheusNamingConvention.

shakuzen avatar Apr 05 '22 04:04 shakuzen

  • wrong usage of api
  • I don't know internals but is it possible to be private, protected this naming conventions
  • snakeCase it sounds for me: ok everything will be snake case regardless of preconditions

sysmat avatar Apr 05 '22 05:04 sysmat

is it possible to be private, protected this naming conventions

They're not internal code. They are appropriate to use with some other metrics backends that are not as restrictive on allowed characters as Prometheus. They also can serve as a building block for users to make their own custom naming conventions, which requires them to be public API.

snakeCase it sounds for me: ok everything will be snake case regardless of preconditions

What is "everything"? Maybe it seems obvious for your use case that it includes . and -, but what about every other special character? Then we will have users saying they didn't expect the naming convention to convert some character to _. In general, we don't want to change things any more than we have to for a backend's requirements and secondarily its conventions. We think this offers the least surprising experience.

I've reopened the ticket to better document this so it is hopefully more clear going forward.

shakuzen avatar Apr 05 '22 06:04 shakuzen

I’ve opened https://github.com/spring-projects/spring-boot/issues/30536 to review things on the Boot side.

The recommendation for dot-separated names seems to be pretty close to a requirement. I wonder if Micrometer 2.0 could make it a requirement or at least make the current API require dot-separated names with another API that’s more relaxed?

wilkinsona avatar Apr 05 '22 07:04 wilkinsona