Conversation
mikewilli
left a comment
mikewilli
left a comment
There was a problem hiding this comment.
Will this work with our error checks? Right now we inject an error inside the client code if we expect results but the file doesn't exist. I think with this new feature we'd just skip over them.
Thank you @mikewilli for the review! You are right, the changes would've skipped the error checks and I didn’t notice that. I added a new function so that the task also re-fetches when we expect a window whose file isn't in the bucket, so now the client should still inject the error. It's bounded by the analysis in the exact way we originally used so a result that never arrives doesn't just keep refetching forever. This might be a messy solution but I couldn’t think of another approach. Let me know what you think! |
jaredlockhart
left a comment
jaredlockhart
left a comment
There was a problem hiding this comment.
Okay I understand better, yes it makes sense to compute whether we 'expect' results to compute errors but if there's anything newer we always fetch it anyways so it still catches the 'long since ended but forced reran' catches so we don't have to manually reload. This is good, great test coverage. One idea about using the stored results date instead of adding a new field 🙏
Because * the fetch task used experiment status and a fixed time window to decide when to fetch results This commit * adds a field to track when an experiment's results were last loaded * finds the newest modified time among each experiment's statistics, metadata, and errors files * refreshes when that timestamp is newer than the stored one, and saves the new timestamp after loading * still refreshes when an expected results window is missing from the bucket so that missing-results errors are injected Fixes #15737
Because * the fetch task used experiment status and a fixed time window to decide when to fetch results This commit * adds a field to track when an experiment's results were last loaded * finds the newest modified time among each experiment's statistics, metadata, and errors files * refreshes when that timestamp is newer than the stored one, and saves the new timestamp after loading * still refreshes when an expected results window is missing from the bucket so that missing-results errors are injected Fixes #15737
Because * the fetch task used experiment status and a fixed time window to decide when to fetch results This commit * adds a field to track when an experiment's results were last loaded * finds the newest modified time among each experiment's statistics, metadata, and errors files * refreshes when that timestamp is newer than the stored one, and saves the new timestamp after loading * still refreshes when an expected results window is missing from the bucket so that missing-results errors are injected Fixes #15737
jaredlockhart
left a comment
jaredlockhart
left a comment
There was a problem hiding this comment.
Right yep we looked at this together I thought I had approved it sorry for the delay, yes let's ship this and then if we find there's some case we missed in prod we can update. Thanks for tackling this @RJAK11 🙏 🎉 This will be such a help to not have to manually refresh results all the time 🙏 🙏 🙏 🙏
Because * the fetch task used experiment status and a fixed time window to decide when to fetch results * #15871 changed this to fetch whenever an experiment has newer results, but was reverted in #16152 after invalid metadata for older experiments crashed the fetch loop This commit * reland the reverted #15871 changes that fetch results when an experiment's analysis start time is newer than the stored one Fixes #15737
Because * Jetstream writes metadata for some older experiments that is missing fields the current schema requires (e.g. analysis_bases) * validating that metadata raises a ValidationError that we do not catch * this crashed the whole fetch loop and is why #15871 was reverted (#16152) This commit * report the error to Sentry and skip the experiment instead of raising, in both the main loop (get_latest_analysis_start_time) and the fetch task (fetch_experiment_data), so invalid data is never stored or surfaced and one bad experiment cannot abort the fetch for the others * add tests that reproduce the schema validation error reported in Sentry Fixes #16203
Because * Jetstream writes metadata for some older experiments that is missing fields the current schema requires (e.g. analysis_bases) * validating that metadata raises a ValidationError that we do not catch * this crashed the whole fetch loop and is why #15871 was reverted (#16152) This commit * report the error to Sentry and skip the experiment instead of raising, in both the main loop (get_latest_analysis_start_time) and the fetch task (fetch_experiment_data), so invalid data is never stored or surfaced and one bad experiment cannot abort the fetch for the others * add tests that reproduce the schema validation error reported in Sentry Fixes #16203
Because * the fetch task used experiment status and a fixed time window to decide when to fetch results * mozilla#15871 changed this to refresh whenever an experiment has newer results, but was reverted in mozilla#16152 after metadata for some older experiments failed schema validation and aborted the fetch task This PR * relands the mozilla#15871 changes that fetch results when an experiment has newer results * catches ValidationError when reading metadata, reports it to Sentry and skips only that experiment so a single experiment with invalid metadata no longer breaks the whole fetch Fixes mozilla#16203
3 participants
Because
This commit
Fixes #15737