fix/weekly cache metrics endpoint#1942
Conversation
| ) | ||
| end_datetime = datetime.combine( | ||
| today - timedelta(days=end_days_ago), | ||
| time.max, |
There was a problem hiding this comment.
Trying to work out the SQL below, I think this will be an 8-day window? Because it causes SQL to be at the end of the 7-day window (i.e. on the last day it will be at midnight).
There was a problem hiding this comment.
yes, you are correct.
The cached query is happening at the start of the day (just like the mail notification), so the 8th day would end up empty.
But still, it is not robust to rely on time of the call.
2ed607d to
98b56ae
Compare
mentonin
left a comment
There was a problem hiding this comment.
Added some comments. I think this works in general, but should be seen as a palliative solution. A better solution might be a rollup table. I think we could also use a set of prometheus metrics (we are tracking difference between timestamps of time series with low cardinality).
| (SELECT COUNT(*) FROM checkouts WHERE _timestamp BETWEEN | ||
| NOW() - INTERVAL %(start_days_ago)s | ||
| AND NOW() - INTERVAL %(end_days_ago)s) | ||
| (SELECT COUNT(*) FROM checkouts WHERE _timestamp >= |
There was a problem hiding this comment.
I believe you can merge this SELECT with the previous one for (minor) performance gains
| from django.utils import timezone as django_timezone | ||
|
|
||
|
|
||
| def seeded_timestamp(*, days_ago: int = 1) -> datetime: |
There was a problem hiding this comment.
It would be great if we could include some specific tests related to the timestamp: out-of-order rows, cases before/after query intervals, check open or closed interval boundaries, etc.
There was a problem hiding this comment.
GH won't let me mark this as resolved, but tests are looking very robust now 👍🏼
| const today = new Date(); | ||
| const weekEndDaysAgo = (today.getUTCDay() + 1) % DAYS_IN_WEEK; | ||
| const endDaysAgo = Math.max(weekEndDaysAgo - 1, 0); | ||
| const startDaysAgo = endDaysAgo + activeDays + (weekEndDaysAgo > 0 ? 1 : 0); |
There was a problem hiding this comment.
IIUC this gets an interval of activeDays + 1 complete days (e.g. it includes 2 Fridays instead of 1 when activeDays=7)
There was a problem hiding this comment.
On a broader note, I don't like the interface here. /metrics?i=n returns metrics from n days before the last Saturday, which is not really intuitive. I think you can only set i=7 or i=14 from the UI, but then why do we expose that as an integer for the user to edit?
There was a problem hiding this comment.
Nice catch, I changed the backend behaviour and forgot to update here at some point.
There was a problem hiding this comment.
On the topic of the url query param, I think I am with you.
I am removing this parameter altogether, unless users actually show need to share specific ranges via url. And even if they do, we might limit the options.
| 'This is the legacy version of the {page}, please refer to the new, optimized version {newPageLink}. If you find any bugs or divergences, please report to {gitHubLink}.', | ||
| 'metricsPage.computedAt': 'Computed {computedAt}', | ||
| 'metricsPage.period.previousTwoWeeks': 'Previous 2 weeks', | ||
| 'metricsPage.period.previousWeek': 'Previous week', |
There was a problem hiding this comment.
| 'metricsPage.period.previousWeek': 'Previous week', | |
| 'metricsPage.period.previousWeek': 'Last week', |
98b56ae to
4b6cdd5
Compare
| <p> | ||
| {formatMessage( | ||
| { id: 'metricsPage.computedAt' }, | ||
| { computedAt: formatDate(data.created_at, false, true) }, |
There was a problem hiding this comment.
Should we display "Created at" or the actual interval spanned by the data?
There was a problem hiding this comment.
In this case I really want to show the time data was computed, mostly because it is a long cache.
We could also show the range of the last week, but I am not sure if we gain much from doing it.
There was a problem hiding this comment.
Wince we are slicing by _timestamp, I don't think the computed time is useful: the metrics for a specific time interval (in the past) should be constant
There was a problem hiding this comment.
Fair. I am removing, since it was introducing unecessary complexity
| # UTC midnight, so each lands exactly on an interval_params day boundary. This lets | ||
| # adjacent metrics windows return distinct, non-zero counts to exercise the half-open | ||
| # [start, end) boundaries on both sides. | ||
| SEED_DAY_SPAN = 8 |
There was a problem hiding this comment.
IIUC, this creates data at 00:00:00 at current day up to 7 days ago. The default query fetches events from 7 days ago, 00:00:00 up to yesterday, 23:59:59. So this does not test start date
There was a problem hiding this comment.
Good point. I have moved the span further, not only to include prev responses, but also to help the test be more robust, and fail if we do not properly filter start date.
| from django.utils import timezone as django_timezone | ||
|
|
||
|
|
||
| def seeded_timestamp(*, days_ago: int = 1) -> datetime: |
There was a problem hiding this comment.
GH won't let me mark this as resolved, but tests are looking very robust now 👍🏼
| "n_tests": 26, | ||
| "n_issues": 17, | ||
| "n_incidents": 7, | ||
| "prev_n_trees": 0, |
There was a problem hiding this comment.
previous values should probably also be seeded and tested for
| 'messages.olderPageVersion': | ||
| 'This is the legacy version of the {page}, please refer to the new, optimized version {newPageLink}. If you find any bugs or divergences, please report to {gitHubLink}.', | ||
| 'metricsPage.computedAt': 'Computed {computedAt}', | ||
| 'metricsPage.period.previousTwoWeeks': 'Previous 2 weeks', |
There was a problem hiding this comment.
| 'metricsPage.period.previousTwoWeeks': 'Previous 2 weeks', | |
| 'metricsPage.period.previousTwoWeeks': 'Last 2 weeks', |
For consistency, since you accepted my suggestion
917fb5f to
7bed5fe
Compare
* Use fixed UTC date bounds for metrics intervals so cache keys stay stable across the week * Expose created_at on the API * Align the dashboard with the email window, and warm the cache after the Saturday metrics email. * Change seeds to include older timestamp Closes kernelci#1940 Signed-off-by: Alan Peixinho <alan.peixinho@profusion.mobi>
7bed5fe to
9ab53c0
Compare
What it is
Makes the metrics page cache its response for a week, so we can:
How to test:
Test warming cache crontab
poetry run python3 manage.py crontab addpoetry run python3 manage.py crontab showpoetry run python3 manage.py crontab run <hash of cronjob>* It is important to point here that the warm is supposed to run at the same time as email metrics (saturday), so they might not be aligned to the same days as frontend requests.
* Alternatively to this, we could change the system clock.
metricsTotalObjects,metricsBuildIncidents,metricsNewBuildIncidents,metricsLabSummary) (via redis-cli) or via python script.poetry run python3 manage.py crontab removeTesting endpoint
Closes #1940