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

Correct Prometheus output for timer and JSON output for SimpleTimer (2.x) #4242

Merged
merged 2 commits into from
May 23, 2022
Merged

Correct Prometheus output for timer and JSON output for SimpleTimer (2.x) #4242

merged 2 commits into from
May 23, 2022

Conversation

tjquinno
Copy link
Member

@tjquinno tjquinno commented May 19, 2022

Resolves #4237
Resolves #4240

There were two separate errors in creating metrics output.

  1. Prometheus output for timers did not correctly scale the numeric values. In Prometheus, time values must always be expressed in seconds. Internally, Helidon stores the timer data in nanoseconds. So, to correctly format time values in Prometheus format, the timer code should always convert from nanoseconds (the stored values) to seconds. But the code incorrectly used the units stored in the metadata for the timer. That would've been OK if the values stored in the timer were in that unit, but our timer implementation always stored nanoseconds. And this is the correct thing to do in general, for other types of scaled quantities (such as storage) but not for time.

    The fix here was to generalize a bit of the Prometheus output code so rather than always using the units stored in the metric metadata to decide how to convert values for output, the units object could be passed in. The timer now always passes in a fixed nanoseconds-to-seconds converter, rather than using a converter based on whatever units were declared in the timer's metadata.

  2. While working on the Prometheus/timer problem, and while adding tests for JSON/timer output and for Prom. and JSON SimpleTimer output, I found that the SimpleTimer JSON output incorrectly always expressed the values as seconds, rather than in the specified units. HelidonSimpleTimer now correctly scales the values according to the units specified in the metadata. This fix involved refactoring some code (the conversionFactor method) from HelidonTimer to MetricImpl so it was available to HelidonSimpleTimer as well.

Signed-off-by: tim.quinn@oracle.com tim.quinn@oracle.com

…add tests

Signed-off-by: tim.quinn@oracle.com <tim.quinn@oracle.com>
@tjquinno tjquinno self-assigned this May 19, 2022
@tjquinno tjquinno changed the title Correct Prometheus output for timer and JSON output for SimpleTimer Correct Prometheus output for timer and JSON output for SimpleTimer (2.x) May 19, 2022
@tjquinno tjquinno requested a review from spericas May 19, 2022 21:20
Signed-off-by: tim.quinn@oracle.com <tim.quinn@oracle.com>
@tjquinno tjquinno merged commit bb31c26 into helidon-io:helidon-2.x May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants