Bug 1571369 - Store backfill reports per alert summary #5539
Conversation
ionutgoldan
added
WIP
python
db-schema-changes
ionutgoldan
force-pushed
the
store-backfill-reports
branch
9 times, most recently
from
7791bd1 to
55e2040
Compare
Codecov Report
@@ Coverage Diff @@
## master #5539 +/- ##
=======================================
Coverage 38.81% 38.81%
=======================================
Files 197 197
Lines 6505 6505
Branches 1397 1397
=======================================
Hits 2525 2525
Misses 3671 3671
Partials 309 309Continue to review full report at Codecov.
|
ionutgoldan
force-pushed
the
store-backfill-reports
branch
15 times, most recently
from
2ab4094 to
eed59fc
Compare
| for alert, retrigger_context in alert_context_map: | ||
| BackfillRecord.objects.create(alert=alert, | ||
| report=backfill_report, | ||
| context=json.dumps(retrigger_context)) |
There was a problem hiding this comment.
It wouldn't be more appropriate to use a custom serializer instead of json.dumps? Just asking.
There was a problem hiding this comment.
It wouldn't be more appropriate to use a custom serializer instead of json.dumps? Just asking.
I'm not sure this is what Django serializers where created for. @sarah-clements what do you think?
There was a problem hiding this comment.
I think this is what serializers are useful for.
ionutgoldan
force-pushed
the
store-backfill-reports
branch
3 times, most recently
from
fe50819 to
22409c8
Compare
octavian-negru
left a comment
octavian-negru
left a comment
There was a problem hiding this comment.
Looks good for me.
|
FYI @karlht. |
sarah-clements
left a comment
sarah-clements
left a comment
There was a problem hiding this comment.
This looks ok. However, since we are testing pulse ingestion and related work for the Taskcluster migration this Saturday and since this pr is significant, with its changes to the db, please don't merge it until Monday.
|
|
||
| operations = [ | ||
| migrations.CreateModel( | ||
| name='BackfillReport', |
There was a problem hiding this comment.
Do we expire old alert summaries/alerts and will these tables get cleaned up periodically by the cycle_data or other task?
There was a problem hiding this comment.
We're not expiring alert summaries/alerts, because there are still Bugzilla comments pointing back to them via hyperlinks.
Regarding the cleanup of these reports: we will consider this in our future roadmaps, once we turn on the auto retriggering bot. We need to know to which extent we can expire them, without affecting the bot.
There was a problem hiding this comment.
We're not expiring alert summaries/alerts, because there are still Bugzilla comments pointing back to them via hyperlinks.
Is there a need to keep them beyond the one year limit you have for performance datum? I don't know if its a realistic expectation that links in bugzilla comments should always work regardless of how old they are.
Regarding the cleanup of these reports: we will consider this in our future roadmaps, once we turn on the auto retriggering bot. We need to know to which extent we can expire them, without affecting the bot.
Yes, please do add that to your roadmap. We already store a lot of historical data for Perfherder and I'd like to be sure as we add new tables or new columns to tables we have a plan for the data that is no longer needed.
There was a problem hiding this comment.
@davehunt we need to consider expiring the alert summaries & alerts also.
There was a problem hiding this comment.
Could you check with BMO to see if there is a policy regarding how long linked resources should remain available? I think it's reasonable to show a page stating that the alert is no longer available, but not sure what age we should do this at.
No problem. Will wait. |
ionutgoldan
force-pushed
the
store-backfill-reports
branch
from
22409c8 to
7702b90
Compare
ionutgoldan
force-pushed
the
store-backfill-reports
branch
from
7702b90 to
0ede92d
Compare
|
@armenzg we should merge & deploy this today. I've rebased it on latest master. |
6 participants
No description provided.