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

Document the behavior and expectations of NamingConvention #3108

Open
sysmat opened this issue Mar 31, 2022 · 32 comments
Open

Document the behavior and expectations of NamingConvention #3108

sysmat opened this issue Mar 31, 2022 · 32 comments
Labels
doc-update A documentation update module: micrometer-core An issue that is related to our core module type: task A general task waiting for feedback We need additional information before we can continue
Milestone

Comments

@sysmat
Copy link

sysmat commented Mar 31, 2022

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

@checketts
Copy link
Contributor

checketts commented Mar 31, 2022

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
Copy link
Member

jonatan-ivanov commented Mar 31, 2022

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 jonatan-ivanov added the invalid An issue that we don't feel is valid label Mar 31, 2022
@sysmat
Copy link
Author

sysmat commented Apr 1, 2022

@sysmat
Copy link
Author

sysmat commented Apr 1, 2022

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

@sysmat
Copy link
Author

sysmat commented Apr 1, 2022

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
Copy link
Member

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

@sysmat
Copy link
Author

sysmat commented Apr 1, 2022

  • 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
Copy link
Author

sysmat commented Apr 1, 2022

@sysmat
Copy link
Author

sysmat commented Apr 1, 2022

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

@sysmat
Copy link
Author

sysmat commented Apr 1, 2022

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

@sysmat
Copy link
Author

sysmat commented Apr 1, 2022

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

@checketts
Copy link
Contributor

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
Copy link
Author

sysmat commented Apr 1, 2022

exectly that

@sysmat
Copy link
Author

sysmat commented Apr 1, 2022

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

@sysmat
Copy link
Author

sysmat commented Apr 1, 2022

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

@sysmat
Copy link
Author

sysmat commented Apr 1, 2022

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
Copy link
Author

sysmat commented Apr 1, 2022

  • 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
Copy link
Author

sysmat commented Apr 1, 2022

  • 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

@checketts
Copy link
Contributor

checketts commented Apr 1, 2022

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
Copy link
Contributor

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
Copy link
Author

sysmat commented Apr 2, 2022

@sysmat
Copy link
Author

sysmat commented Apr 2, 2022

  • 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
Copy link
Author

sysmat commented Apr 2, 2022

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

@wilkinsona
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

sysmat commented Apr 2, 2022

@wilkinsona thx

@shakuzen
Copy link
Member

shakuzen commented Apr 5, 2022

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
Copy link
Author

sysmat commented Apr 5, 2022

  • 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

@shakuzen shakuzen added doc-update A documentation update type: task A general task module: micrometer-core An issue that is related to our core module and removed invalid An issue that we don't feel is valid labels Apr 5, 2022
@shakuzen shakuzen added this to the 1.7.11 milestone Apr 5, 2022
@shakuzen shakuzen changed the title springboot:prometheus names and labels not snake case Document the behavior and expectations of NamingConvention Apr 5, 2022
@shakuzen
Copy link
Member

shakuzen commented Apr 5, 2022

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 shakuzen reopened this Apr 5, 2022
@wilkinsona
Copy link
Contributor

I’ve opened spring-projects/spring-boot#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?

@shakuzen shakuzen modified the milestones: 1.7.11, 1.7.x Apr 14, 2022
@jjank
Copy link
Contributor

jjank commented Apr 14, 2022

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 ?

@shakuzen shakuzen modified the milestones: 1.7.x, 1.8.x May 11, 2022
@jonatan-ivanov jonatan-ivanov modified the milestones: 1.8.x, 1.9.x Jan 12, 2023
@marcingrzejszczak
Copy link
Contributor

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?

@marcingrzejszczak marcingrzejszczak added the waiting for feedback We need additional information before we can continue label Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-update A documentation update module: micrometer-core An issue that is related to our core module type: task A general task waiting for feedback We need additional information before we can continue
Projects
None yet
Development

No branches or pull requests

7 participants