-
Notifications
You must be signed in to change notification settings - Fork 337
feat: Support negative duration in new function ParseDurationAllowNegative #793
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
feat: Support negative duration in new function ParseDurationAllowNegative #793
Conversation
beorn7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. I just have a nit about redundant code comments and overly short variable names.
However, there is also a more general concern: My assumption was this function is just used for templating, but it is used for many other purposes, too. It is not unlikely that those other purposes rely on the return never being negative.
Let's play safe for now and better introduce a new function that allows negative durations (e.g. ParseDurationAllowNegative or something). It will be easy to wire it to parseDuration in the templating.
WDYT?
|
@beorn7 actually has similar question before, i agree. what would you say now? mean what think about |
beorn7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. The code is correct, but I have a bunch of suggestions to make it more DRY and compact.
|
@beorn7 i really like everything, thanks, and i also improved TestParseDuration function in tests |
beorn7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. A few more tweaks.
|
@beorn7 LGTY? |
beorn7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I'll also tag a new release so that you can use this change in prometheus/prometheus#16619
Signed-off-by: Dmitry Ponomaryov <[email protected]>
Signed-off-by: Dmitry Ponomaryov <[email protected]>
Signed-off-by: Dmitry Ponomaryov <[email protected]>
Signed-off-by: Dmitry Ponomaryov <[email protected]>
Signed-off-by: Dmitry Ponomaryov <[email protected]>
Signed-off-by: Dmitry Ponomaryov <[email protected]>
Signed-off-by: Dmitry Ponomaryov <[email protected]>
Signed-off-by: Dmitry Ponomaryov <[email protected]>
Signed-off-by: Dmitry Ponomaryov <[email protected]>
Signed-off-by: Dmitry Ponomaryov <[email protected]>
21391c0 to
c099408
Compare
This PR extend the
ParseDurationfunction to support negative durations.negative durations need be useful in Loki alerts templates where time offsets such as '-5m' are common in range calculations and comparisons.
Changes:
ParseDurationto handle negative values like-1h,-30m, etc.related to promethes/template changes for Loki alerting - see prometheus/prometheus#16669