Bundle Analysis: fix trends when no data in query range#1267
Bundle Analysis: fix trends when no data in query range#1267JerrySentry merged 5 commits intomainfrom
Conversation
✅ Sentry found no issues in your recent changes ✅ |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #1267 +/- ##
==========================================
+ Coverage 96.31% 96.32% +0.01%
==========================================
Files 487 487
Lines 16891 16891
==========================================
+ Hits 16269 16271 +2
+ Misses 622 620 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -1178,9 +1178,12 @@ def test_no_dataset(self, trigger_backfill): | |||
| repository_id__in=[self.repo1.pk, self.repo2.pk], | |||
| ) | |||
| ) | |||
There was a problem hiding this comment.
not sure why this test breaks when I introduce the new tests from above.. namely the ordering of the input is flipped.. going to check that either ordering direction works, in trigger_backfill it doesn't matter for the ordering anyways.
There was a problem hiding this comment.
could also confirm the items in the set as well
| before: datetime, | ||
| after: Optional[datetime] = None, | ||
| branch: Optional[str] = None, | ||
| ) -> Dict[int, List[Dict[str, Any]]]: |
There was a problem hiding this comment.
was this change on purpose? Should we also change the type on 41 as well
There was a problem hiding this comment.
yeah because when this function was made the measurable_id was only ints representing the flag IDs for flag measurements, but it can be also str for component names or BA reports.
Yes updating line 41.
| carryover = dict(measurements[0]) | ||
| carryover["timestamp_bin"] = self.after | ||
| carryover = dict(measurements[0]) if measurements else {} | ||
| carryover["timestamp_bin"] = self.after.replace( |
There was a problem hiding this comment.
is this to "zero out" the timestamp?
There was a problem hiding this comment.
yes, in the downstream when building the list of measurements to return it needs to have 00:00:00.000 as the time, that is expected because normally we pull the data from the aggregate tables, naturally it always agg by n days and so the time is always 0s. But in this case when we carry over the last datapoint we don't query from the aggregate tables, instead it comes from the un-agg'd table so the time is when it got inserted. So this part would zero out the times to comply with the logic later.
2 participants
When there are no measurement datapoints for the specified query range then instead of return nothing we will look back in time past the start of the range to look for the first datapoint and let that be the first data point in the query range. This basically carryovers the nearest measurement so that the trend graph renders with a starting point
closes: codecov/engineering-team#2886
closes: codecov/engineering-team#3358
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.