Skip to content

MeterValue with "d" suffix not parsed as Duration for timer #28302

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

Closed
bespaltovyj opened this issue Oct 12, 2021 · 4 comments
Closed

MeterValue with "d" suffix not parsed as Duration for timer #28302

bespaltovyj opened this issue Oct 12, 2021 · 4 comments
Assignees
Labels
type: regression A regression from a previous release
Milestone

Comments

@bespaltovyj
Copy link

bespaltovyj commented Oct 12, 2021

Spring Boot Version 2.4.8

If you specify a value with the suffix d for the timer when setting slo, then it is parsed as a number, not a duration.

Example:

management:
  metrics:
    distribution:
      slo:
        <YOUR_TIMER_NAME>: 1d, 2d, 3d

The values of the baskets will be 1, 2 ,3, but required 86400s, 172800s, 259200s.

test example

To work around this problem, I specify the duration in hours.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 12, 2021
@polarbear567
Copy link
Contributor

polarbear567 commented Oct 13, 2021

Hi @bespaltovyj ,
In this configuration, if you want to use seconds as the unit, in your example, I think you should use s, as follows:

management:
  metrics:
    distribution:
      slo:
        <YOUR_TIMER_NAME>: 86400s, 172800s, 259200s

But this is an interesting problem. The root cause of this problem is that we will eventually convert the value to a double type, but for the value like 1d, the Double.parseDouble(String s) method will directly convert it to 1.0.
@wilkinsona @snicoll I don't know what is the ideal treatment for string values like "1d" in design, whether it is regarded as a double value or a day?

@wilkinsona
Copy link
Member

The documentation for the slo map states the following:

Values can be specified as a long or as a Duration value (for timer meters, defaulting to ms if no unit specified)

I think we should be aiming to match the implementation to that documentation.

@wilkinsona wilkinsona added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 13, 2021
@wilkinsona wilkinsona added this to the 2.4.x milestone Oct 13, 2021
@wilkinsona
Copy link
Member

wilkinsona commented Oct 13, 2021

It looks like this worked in 2.2 but the behaviour changed due to #20837. Contrary to what I said above, it looks like the documentation for the slo map should have been updated to reflect the switch from long to double. The question now is how to handle a value that's both a valid double and a valid duration such as 1d.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Oct 13, 2021
@wilkinsona
Copy link
Member

The question now is how to handle a value that's both a valid double and a valid duration such as 1d.

If we assume that 1d should be treated as a duration of one day, flipping the order so we trying to parse as a Duration and then fall back to a double seems to work.

@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Oct 13, 2021
@wilkinsona wilkinsona added type: regression A regression from a previous release and removed type: bug A general bug labels Oct 13, 2021
@philwebb philwebb modified the milestones: 2.4.x, 2.4.12 Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: regression A regression from a previous release
Projects
None yet
Development

No branches or pull requests

5 participants