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

Metrics are in in camelCase #8422

Closed
hyder opened this issue Feb 26, 2024 · 14 comments · Fixed by #9429
Closed

Metrics are in in camelCase #8422

hyder opened this issue Feb 26, 2024 · 14 comments · Fixed by #9429
Assignees
Labels
4.x Version 4.x metrics

Comments

@hyder
Copy link

hyder commented Feb 26, 2024

Environment Details

  • Helidon Version: 4.0.5
  • Helidon MP
  • JDK version: GraalVM 21
  • OS: Ubuntu
  • Docker version (if applicable): NA

Problem Description 1

Metrics created by Helidon are in camelCase. Prometheus community prefers snake_case. To reproduce:

 curl http://localhost:8080/metrics | ./promtool check metrics
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 20365  100 20365    0     0  1385k      0 --:--:-- --:--:-- --:--:-- 1420k
REST_request_unmappedException_total metric names should be written in 'snake_case' not 'camelCase'
classloader_loadedClasses_count metric names should be written in 'snake_case' not 'camelCase'
classloader_loadedClasses_count non-histogram and non-summary metrics should not have "_count" suffix
classloader_loadedClasses_total metric names should be written in 'snake_case' not 'camelCase'
classloader_unloadedClasses_total metric names should be written in 'snake_case' not 'camelCase'
cpu_availableProcessors metric names should be written in 'snake_case' not 'camelCase'
cpu_systemLoadAverage metric names should be written in 'snake_case' not 'camelCase'
createCustomer_total metric names should be written in 'snake_case' not 'camelCase'
createCustomer_total no help text
deleteCustomer_total metric names should be written in 'snake_case' not 'camelCase'
deleteCustomer_total no help text
getCustomerByEmail_total metric names should be written in 'snake_case' not 'camelCase'
getCustomerByEmail_total no help text
getCustomerById_total metric names should be written in 'snake_case' not 'camelCase'
getCustomerById_total no help text
getCustomers_total metric names should be written in 'snake_case' not 'camelCase'
getCustomers_total no help text
helidon_cloudbank_customer_totalCustomers_CustomerResource_total metric names should be written in 'snake_case' not 'camelCase'
helidon_cloudbank_customer_totalCustomers_CustomerResource_total no help text
hikaricp_connections_creation_milliseconds use base unit "seconds" instead of "milliseconds"
hikaricp_connections_creation_milliseconds_max use base unit "seconds" instead of "milliseconds"
hikaricp_connections_usage_milliseconds use base unit "seconds" instead of "milliseconds"
hikaricp_connections_usage_milliseconds_max use base unit "seconds" instead of "milliseconds"
hikaricp_connections_wait_nanoseconds use base unit "seconds" instead of "nanoseconds"
hikaricp_connections_wait_nanoseconds_max use base unit "seconds" instead of "nanoseconds"
memory_committedHeap_bytes metric names should be written in 'snake_case' not 'camelCase'
memory_maxHeap_bytes metric names should be written in 'snake_case' not 'camelCase'
memory_usedHeap_bytes metric names should be written in 'snake_case' not 'camelCase'
thread_count non-histogram and non-summary metrics should not have "_count" suffix
thread_daemon_count non-histogram and non-summary metrics should not have "_count" suffix
thread_max_count non-histogram and non-summary metrics should not have "_count" suffix
updateCustomer_total metric names should be written in 'snake_case' not 'camelCase'
updateCustomer_total no help text

Problem description 2

No help text is generated. See above.

@tjquinno
Copy link
Member

tjquinno commented Feb 26, 2024

  1. The MicroProfile Metrics spec mandates many of these:

    1. For example, REST_request_unmappedException_total https://download.eclipse.org/microprofile/microprofile-metrics-5.0.0/microprofile-metrics-spec-5.0.0.html#_mapped_and_unmapped_exceptions
    2. Base (JVM-related): for example classloader_loadedClasses_* https://download.eclipse.org/microprofile/microprofile-metrics-5.0.0/microprofile-metrics-spec-5.0.0.html#base-metrics
  2. IIRC the Hikari metrics come from Hikari not from Helidon code. I'm not sure if there is a way Helidon could alter the units.

  3. For some of the other metrics, such as helidon_cloudbank_* and xxxCustomer* ones, are they coming from core Helidon? I do not know why they would have missing help.

    A simple reproducer for this last category would be nice.

@tjquinno tjquinno self-assigned this Feb 26, 2024
@m0mus m0mus added metrics 4.x Version 4.x labels Feb 26, 2024
@hyder
Copy link
Author

hyder commented Feb 27, 2024

  1. The MicroProfile Metrics spec mandates many of these:

    1. For example, REST_request_unmappedException_total https://download.eclipse.org/microprofile/microprofile-metrics-5.0.0/microprofile-metrics-spec-5.0.0.html#_mapped_and_unmapped_exceptions
    2. Base (JVM-related): for example classloader_loadedClasses_* https://download.eclipse.org/microprofile/microprofile-metrics-5.0.0/microprofile-metrics-spec-5.0.0.html#base-metrics
  2. IIRC the Hikari metrics come from Hikari not from Helidon code. I'm not sure if there is a way Helidon could alter the units.
    Whatever we can control.

  3. For some of the other metrics, such as heildon_cloudbank_* and xxxCustomer* ones, are they coming from core Helidon? I do not know why they would have missing help.
    These ones are coming from the app.

A simple reproducer for this last category would be nice.

Please see: https://github.com/helidon-io/helidon-cloudbank/blob/main/app/customer/src/main/java/helidon/cloudbank/customer/CustomerResource.java

@tjquinno
Copy link
Member

Thanks for the pointer. I blindly tried but cannot build the customer app because tests fail with data source problems, I assume because microprofile-config.properties declares but does not assign values to various data source properties.

That said, I will see about using the way the CustomerResource is written with metrics annotations to see if I can reproduce the missing help problem in a separate app.

@tjquinno
Copy link
Member

tjquinno commented Feb 28, 2024

If the developer's annotation does not specify a description then although the # HELP line is present in the output there is no text on the rest of the line and that is what promtool seems to be complaining about.

I suppose Helidon could generate a description if none is provided, but I don't think that's a good idea. I think we have to accept what the developer tells us...if the description is omitted or is specified as an empty string (which is the default value) then we take the developer at her word and use that, regardless of what promtool says.

Of course if the developer cares about promtool "acceptance" then she can always specify a description on all metrics annotations.

@barchetta
Copy link
Member

barchetta commented Mar 4, 2024

Some additional background info:

  1. Prometheus use of snake_case and conversion of camelCase: Metric Naming
  2. MicroProfile metrics decision to stop converting camelCase (in Metrics 5): stop automatically converting camelCase to snake_case in OpenMetrics exporter eclipse/microprofile-metrics#357

Two options come to mind for addressing this:

  1. Support a configuration option for our Prometheus exporter to auto map camelCase to snake_case.
  2. Support a configuration option to use snake_case instead of camelCase for our (built-in) vendor metrics.

The first option seems problematic because there are cases where the mapping will do a poor job.

The second option seems more reasonable.

@tonychoe
Copy link

Can anything be done at Helidon level?

Consequence is Helidon users will have to convert metric names in order to get them collected by Prometheus.

@tjquinno
Copy link
Member

tjquinno commented Apr 11, 2024

The MP metrics-prescribed naming might not conform to Prometheus recommendations but in my experience the Prometheus server scrapes them successfully.

If you are seeing actual errors please share them and whatever set-up you are using.

@hyder
Copy link
Author

hyder commented Apr 22, 2024

Sorry for the delay. Finally got round to testing this. Adding the description in each annotation as @tjquinno suggested helps fix the "no help text". I agree with Tim, we should not generate the help text here. I wonder whether we should make the description as mandatory. If the annotation is added, then the description must also be added.

@hyder
Copy link
Author

hyder commented Apr 22, 2024

On another note, I see 2 additional issues:

  1. I named my aggregated metric "totalCustomers". See https://github.com/helidon-io/helidon-cloudbank/blob/main/app/customer/src/main/java/helidon/cloudbank/customer/rest/CustomerResource.java#L29. However, the metric comes out as "helidon_cloudbank_customer_rest_totalCustomers_CustomerResource_total" which is equivalent to the format package.metric_name.classname. I think if the user provides a name, then the metric should use that name. Please note this affects only aggregated metrics.
  2. The method signature is also added to the method label:
REST_request_seconds_max{class="helidon.cloudbank.customer.rest.CustomerResource",method="deleteCustomer_java.lang.String",mp_scope="base",} 0.0
REST_request_seconds_max{class="helidon.cloudbank.customer.rest.CustomerResource",method="createCustomer_helidon.cloudbank.customer.dto.CustomerDto",mp_scope="base",} 0.0

I don't see the need for adding method signature in the label.

@hyder
Copy link
Author

hyder commented Apr 22, 2024

hikari units are in milliseconds:

# TYPE hikaricp_connections_creation_milliseconds summary
hikaricp_connections_creation_milliseconds{mp_scope="vendor",pool="cloudbank",quantile="0.5",} 608.0

This is probably coming from the hikari lib itself. Would it be possible to submit a PR to hikari so it also uses seconds?

@hyder
Copy link
Author

hyder commented Apr 22, 2024

Re: snake vs camel cases, I agree with @barchetta. If we can have a configuration option to use snake case, that would be preferable.

@tjquinno
Copy link
Member

tjquinno commented Sep 26, 2024

This issue has morphed into a collection of several different notes/comments/requests. (And thanks to @hyder for reminding me this issue is still open.) I will try to sort some of this out.

The main action item for the Helidon team from this issue: look into adding a metrics config setting so the built-in system metrics can be registered using snake_case names instead of camelCase.

Original Description

Problem Statement 1 - camelCase vs. snake_case

As Joe pointed out in his 4 Mar post, the MP Metrics spec does not require translation of camelCase to snake_case. If users want to register their metrics using snake_case they can.

Helidon can add a metrics config option users could set so Helidon would register the built-in vendor metrics using snake_case. (If users set this then their server would no longer be MP Metrics-compliant, but this would be their choice.)

Problem Statement 2 - Missing HELP

This was an omission in the application as noted in @hyder 's first 21 April note.

@hyder 's second 21 Apr note

"If the user provides a name, then the metric should use that name."

The metric naming Helidon uses follows the spec: https://download.eclipse.org/microprofile/microprofile-metrics-5.0.0/microprofile-metrics-spec-5.0.0.html#annotated-naming-convention .

The absolute = true setting in the metric annotations allows you more control over the naming, but if you annotate the type then the generated metrics must incorporate the method name (or the type name as the name of the constructor) in order to register distinct metrics for each of the executables.

"I don't see the need for adding method signature in the label."

Including the method name is required by the spec.

The spec mandates that potentially several metrics be registered when you annotate the type, one for each method and constructor. The spec chose to require the method name in the metric name to distinguish these metrics from each other.

@hyder 's 22 Apr note - Hikari connections creation units

Any solution to this is entirely separate from the other concerns described in this issue. I've opened #9290.

@hyder
Copy link
Author

hyder commented Oct 9, 2024

@tjquinno re: naming in 21 Apr note. Got it. Reading the specs, it seems that the default value for absolute is false. If we can set this to true and include that table in our docs with a link to mp metrics for the full doc, I think that should be sufficient.

re: method signature in labels, I don't have any issue with including the method name itself, but do we need to also add the types of parameters of the method to the labels?

@tjquinno
Copy link
Member

tjquinno commented Oct 9, 2024

TL;DR - Yes, we must include the parameter types in the method tag value for REST.request metrics.

Longer explanation...

We have to be very specific as to which measured methods we are talking about here, because the spec mandates different behavior depending on how the metrics arise.

  1. If the developer annotates a type with a metrics annotation (as the CloudBank example you linked to does) the spec requires the system to register metrics for each method and constructor on the type and is to include the method or constructor name in each such metric name. The parameter types are ignored and the system creates no tags to distinguish overloaded methods--all invocations of a methods with the same name update the same metric. https://download.eclipse.org/microprofile/microprofile-metrics-5.1.1/microprofile-metrics-spec-5.1.1.html#annotated-naming-convention

    One other quick note about that... In an earlier comment you described such a metric as an "aggregate" metric. That's potentially misleading because there is not a single metric registered which measures all the constructors and methods; rather, the system registers multiple metrics, one for all constructors and one for each distinctly-named method.

  2. For the automatically-registered REST.request metrics it's a different story. The class and method information are stored in tags, not the metric name, and the tag containing the method information must include the parameter types by spec. https://download.eclipse.org/microprofile/microprofile-metrics-5.1.1/microprofile-metrics-spec-5.1.1.html#_rest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x Version 4.x metrics
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants