fix: don't eager merge sessions into report #336
Conversation
context: https://l.codecov.dev/xvGZaM Because we eagerly merge sessions when processing a report the following scenario can happen: 1. You upload an empty report with a flag set to carryforward 2. There will be a new session for that upload (even though it's empty) 3. In the next commit the session will be carried forward If no real upload was made to clean up old carriedforward sessions from empty uploads they would accumulate in the report (because `_adjust_sessions` only runs *after* the processing is done, and *doesn't run* if it's an empty upload). What prompted this investigation is that a customer was fund with thousands of carryforward session in some of their commits. So I'm exploiting the fact that merges into the report need to be made from within a lock and getting the ID for the session _without_ actually adding it to the original report. If the report is successful we add it and run `_adjust_sessions`. If the report was empty, we just never add it to the report, as if it didn't existed. OBS.: Even though I use the litle exploit I believe that passing the `sessionid` to `ReportBuilder` is unecessary. We can likely not get a sessionid at all when processin the report and it would still work. I decided to leave this for it's own change tho. Also I decided to bump the severity of the messages that indicate a report is empty. Users are not expected to upload empty reports, so it's at least sensible it should be a warning.
|
PS about tests changed in this PR: I started by creating a new test to verify the behavior observed on the investigation could be reproduced. As I am not breaking the behavior accidentally, but very much intentionally, I altered the test to match the new behavior. And left my test too because it was there at this point 😅 |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #336 +/- ##
=======================================
Coverage 98.11% 98.11%
=======================================
Files 385 385
Lines 31988 32006 +18
=======================================
+ Hits 31385 31403 +18
Misses 603 603
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #336 +/- ##
=======================================
Coverage 98.11% 98.11%
=======================================
Files 385 385
Lines 31988 32006 +18
=======================================
+ Hits 31385 31403 +18
Misses 603 603
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
@@ Coverage Diff @@
## main #336 +/- ##
=======================================
Coverage 98.11% 98.11%
=======================================
Files 385 385
Lines 31988 32006 +18
=======================================
+ Hits 31385 31403 +18
Misses 603 603
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Changes have been made to critical files, which contain lines commonly executed in production. Learn more ✅ All tests successful. No failed tests found Additional details and impacted files@@ Coverage Diff @@
## main #336 +/- ##
=======================================
Coverage 98.10% 98.10%
=======================================
Files 416 416
Lines 32688 32706 +18
=======================================
+ Hits 32069 32087 +18
Misses 619 619
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes
|
| """ | ||
| original_report = self._generate_sample_report() | ||
| original_sessions = original_report.sessions | ||
| expected_sessions = { |
There was a problem hiding this comment.
In the previous behaviour, would we have seen 3 sessions instead of 2 here, with two of them being for the flag "somethingold"?
There was a problem hiding this comment.
Yes. You can see a similar behavior in the test I altered. The removed parts are the outcome of the old behavior, where an extra session would be present.
There was a problem hiding this comment.
ah I see now, thanks for pointing that out
context: https://l.codecov.dev/xvGZaM
Because we eagerly merge sessions when processing a report the following scenario can happen:
If no real upload was made to clean up old carriedforward sessions from empty uploads
they would accumulate in the report (because
_adjust_sessionsonly runs after theprocessing is done, and doesn't run if it's an empty upload). What prompted this
investigation is that a customer was fund with thousands of carryforward session in
some of their commits.
So I'm exploiting the fact that merges into the report need to be made from within a lock
and getting the ID for the session without actually adding it to the original report.
If the report is successful we add it and run
_adjust_sessions. If the report was empty,we just never add it to the report, as if it didn't existed.
OBS.: Even though I use the litle exploit I believe that passing the
sessionidtoReportBuilderis unecessary. We can likely not get a sessionid at all when processin the reportand it would still work. I decided to leave this for it's own change tho.
Also I decided to bump the severity of the messages that indicate a report is empty.
Users are not expected to upload empty reports, so it's at least sensible it should be
a warning.
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.