feat: Track Upload Sent events for Coverage/BA/TA (attempt 2) #1195
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: upload/views/uploads.py
Did you find this useful? React with a 👍 or 👎 |
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 #1195 +/- ##
=======================================
Coverage 95.86% 95.86%
=======================================
Files 493 493
Lines 16870 16876 +6
=======================================
+ Hits 16173 16179 +6
Misses 697 697
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
spalmurray-codecov
force-pushed
the
spalmurray/amplitude-upload-event
branch
from
1272b27 to
1589140
Compare
🚨 Sentry detected 7 potential issues in your recent changes 🚨
Lower risk findings
|
upload/views/uploads.py
Outdated
| "user_ownerid": commit.author.ownerid | ||
| if commit.author | ||
| else UNKNOWN_USER_OWNERID, |
There was a problem hiding this comment.
the only change is these lines
upload/views/uploads.py
Outdated
| if commit.author | ||
| else UNKNOWN_USER_OWNERID, | ||
| "ownerid": commit.repository.author.ownerid, | ||
| "repoid": commit.repository.repoid, |
There was a problem hiding this comment.
putting a note here (mostly for myself) to figure out whether we would gain anything from using author_id and repository_id here instead of accessing those values via attrs
There was a problem hiding this comment.
not sure I follow. Where are author_id and repository_id defined?
There was a problem hiding this comment.
they are not defined explicitly anywhere: here's some context https://docs.djangoproject.com/en/dev/ref/models/fields/#database-representation
there's nothing todo on this PR for this comment, i just wanted to make a note to revisit this and check whether we can avoid some reads from the database here
There was a problem hiding this comment.
ahh okay I'm with you. I've been under the assumption that there are no reads for accessing like this. Assumed it would be populated on the already read commit in scope here (though we may need to explicitly add a select_related here to achieve that). Please lmk if I'm wrong I do not want to be doing more than one fetch for the commit's attrs. Side Q does django automatically cache any of this stuff?
🚨 Sentry detected 10 potential issues in your recent changes 🚨Lower risk findings
Did you find this useful? React with a 👍 or 👎 |
Adds event tracking of 'Upload Sent' events for Coverage, Bundles, and test results.
It seems in some cases commit may not have an author set during upload ingest. In these cases we've decided to attribute the events to a special anonymous user that has a special user id constant in shared.
Last time I merged this PR it was breaking uploads because it seems sometimes commit author is also not defined there, which we were not expecting. This second attempt adds the fallback user id to coverage uploads as well.
Note that we are using commit.id instead of the commit sha because a full commit sha is functionally identifiable information (for public repos at least). No reason to use this when we have an internal id that does the same thing but is only useful to us.