micrometer-metrics / micrometer

An application observability facade for the most popular observability tools. Think SLF4J, but for observability.
https://micrometer.io
Apache License 2.0
4.41k stars 974 forks source link

Document the behavior and expectations of NamingConvention #3108

Open sysmat opened 2 years ago

sysmat commented 2 years ago

Describe the bug Prometheus label is not snake case

Environment

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
checketts commented 2 years ago

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?

jonatan-ivanov commented 2 years ago

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

sysmat commented 2 years ago
sysmat commented 2 years ago

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

sysmat commented 2 years ago

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

jonatan-ivanov commented 2 years ago

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

sysmat commented 2 years ago

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

sysmat commented 2 years ago

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

sysmat commented 2 years ago

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

sysmat commented 2 years ago

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

sysmat commented 2 years ago

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

checketts commented 2 years ago

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?

sysmat commented 2 years ago

exectly that

sysmat commented 2 years ago

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

sysmat commented 2 years ago

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

sysmat commented 2 years ago

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 commented 2 years ago
@Bean
    MeterRegistryCustomizer<PrometheusMeterRegistry> metricsCommonTags() {
        return registry -> registry.config()
                                   .namingConvention(NamingConvention.snakeCase);                                  
    }
sysmat commented 2 years ago

demo.zip

checketts commented 2 years ago

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)

wilkinsona commented 2 years ago

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.

sysmat commented 2 years ago
sysmat commented 2 years ago
sysmat commented 2 years ago

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

wilkinsona commented 2 years ago

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 commented 2 years ago

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.

sysmat commented 2 years ago

@wilkinsona thx

shakuzen commented 2 years ago

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.

sysmat commented 2 years ago
shakuzen commented 2 years ago

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.

wilkinsona commented 2 years ago

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?

jjank commented 2 years ago

I agree with @wilkinsona that requiring dot-separated names seems reasonable for 2.0 since it will eliminate a lot of potential guesswork.

It will probably require some transition/migration in 1.x. such as logging a warning when a name does not comply with the requirement.

Since there's a lot of entry points to create meters the only consistent place to validate such a requirement seems to be the constructor of Meter.Id. Thoughts @shakuzen ?

marcingrzejszczak commented 7 months ago

We're not planning to do Micrometer 2.0 release any time soon, so what would be the consensus here for the 1.x branch?