-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: measure uploads using sentry metrics #406
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #406 +/- ##
=======================================
Coverage 96.10% 96.10%
=======================================
Files 637 637
Lines 16673 16693 +20
=======================================
+ Hits 16023 16043 +20
Misses 650 650
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 ✅
@@ Coverage Diff @@
## main #406 +/- ##
=======================================
Coverage 96.10% 96.10%
=======================================
Files 637 637
Lines 16673 16693 +20
=======================================
+ Hits 16023 16043 +20
Misses 650 650
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some nit suggestions but looks good 👍
|
||
def get_agent_from_headers(headers): | ||
try: | ||
return headers["User-Agent"].split("/")[0].split("-")[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] add a comment with the expected values for the User-Agent.
It'll make it clearer for the reader why these splits are necessary and what sort of values we expect.
upload/helpers.py
Outdated
err=str(e), | ||
), | ||
) | ||
return "unsupported-user-agent" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] maybe "unknown" is better than "unsupported" here IF (and only if, I actually don't know) we actually process the upload
upload/helpers.py
Outdated
err=str(e), | ||
), | ||
) | ||
return "unsupported-user-agent" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same nit suggestion as function above
0712fc1
to
9fe8e37
Compare
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 Additional details and impacted files@@ Coverage Diff @@
## main #406 +/- ##
=====================================
Coverage 95.80 95.80
=====================================
Files 757 757
Lines 17252 17272 +20
=====================================
+ Hits 16527 16547 +20
Misses 725 725
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
677fcad
to
90b373f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this version management more dangerous?
ba48eb4
to
9b9ffa8
Compare
Purpose/Motivation
Adding Sentry DDM to the various upload endpoints
Links to relevant tickets
Fixes: codecov/engineering-team#1178
What does this PR do?
generate_upload_sentry_metrics_tags
in upload helpers to generate tags for metrics, tags are:get_agent_from_headers
andget_version_from_headers
to parseUser-Agent
header to get agent and version, if it fails to parse it will returnunsupported-user-agent
Note: My auto formatter went off while saving some of the files and the linter did not undo them, so this PR comes with some formatting changes to some of the files that have not been visited recently.