feat: Track Upload Sent events for Coverage/BA/TA (attempt 2)#1195
feat: Track Upload Sent events for Coverage/BA/TA (attempt 2)#1195spalmurray-codecov merged 12 commits intomainfrom
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.87%
=======================================
Files 493 493
Lines 16871 16877 +6
=======================================
+ Hits 16174 16180 +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! |
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 found no issues in your recent changes ✅ |
spalmurray-codecov
force-pushed
the
spalmurray/amplitude-upload-event
branch
from
ad79409 to
0d5d643
Compare
spalmurray-codecov
temporarily deployed
to
staging
GitHub Actions
Inactive
4 participants
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.