Reuse graphite time format constant #2588
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2588 +/- ##
=======================================
Coverage 54.20% 54.20%
=======================================
Files 558 558
Lines 40634 40626 -8
=======================================
- Hits 22026 22023 -3
+ Misses 18608 18603 -5
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Kudos, SonarCloud Quality Gate passed!
|
stveit
force-pushed
the
graphite_time_format
branch
from
92ae92b to
084b762
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
python/nav/metrics/thresholds.py
Outdated
| def interval_to_graphite_start(delta): | ||
| """Converts a timedelta to a usable Graphite time specification. | ||
|
|
||
| :type delta: datetime.timedelta | ||
|
|
||
| """ | ||
| secs = delta.total_seconds() | ||
| if secs % YEAR == 0: | ||
| return "{0}year".format(int(secs / YEAR)) | ||
| elif secs % DAY == 0: | ||
| return "{0}day".format(int(secs / DAY)) | ||
| elif secs % MINUTE == 0: | ||
| return "{0}min".format(int(secs / MINUTE)) | ||
| else: | ||
| return "{0}s".format(int(secs)) | ||
| now = datetime.now(tz=ZoneInfo(settings.TIME_ZONE)) | ||
| start = now - delta | ||
| graphite_start = start.strftime(GRAPHITE_TIME_FORMAT) | ||
| return graphite_start |
There was a problem hiding this comment.
I'd like to know why the previous version was so clunky, maybe graphite got a more sensible format and this is technical debt?
There was a problem hiding this comment.
Graphite supports multiple formats, both absolute and relative. The original function becomes "clunky" because it specifically translates a timedelta to Graphite's "time delta syntax". Graphite probably also supports using a number of relative seconds, which could make this less clunky - but the URL would also be a lot less friendly if consumed by a human.
This function specifically works with relative time specifications for at least two reasons:
- The generated URL is reusable (and therefore cachable) at any time, and will always be relative to the current time.
- The Graphite server can potentially be in a different timezone from the NAV server, and we avoid trying to translate between them by working with relative time specs.
The refactored version will break both of these principles, unfortunately, so I wouldn't approve of it.
There was a problem hiding this comment.
Looks good to me but I hope @lunkwill42 can shed some light on some clunk.
There was a problem hiding this comment.
For the reasons specifically stated in https://github.com/Uninett/nav/pull/2588/files#r1204178538, I think you should leave the interval_to_graphite() function untouched. It was written specifically to translate timedeltas to relative time specifications, and you just changed it to translate to absolutes instead.
Other than that, everything seems hunky-dory :)
stveit
force-pushed
the
graphite_time_format
branch
from
8635ba8 to
e779d49
Compare
stveit
force-pushed
the
graphite_time_format
branch
from
e779d49 to
8b463ba
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Fixes #2543.
Put constant for graphite time format in python/nav/metrics/data.py, as this seems to be the core module for getting data from graphite, so it makes sense to have it there instead of in the view of sortedstats.
Updates some logic that uses relative time (years/months etc) to instead use timestamp.
Tested this manually to see if it broke anything, and it seems like everything is fine