Replace Sentry metrics with Prometheus metrics #814
Conversation
|
This PR includes changes to |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #814 +/- ##
=======================================
Coverage 98.01% 98.01%
=======================================
Files 444 444
Lines 36469 36473 +4
=======================================
+ Hits 35745 35749 +4
Misses 724 724
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #814 +/- ##
=======================================
Coverage 98.01% 98.01%
=======================================
Files 444 444
Lines 36469 36473 +4
=======================================
+ Hits 35745 35749 +4
Misses 724 724
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #814 +/- ##
=======================================
Coverage 98.01% 98.01%
=======================================
Files 444 444
Lines 36469 36473 +4
=======================================
+ Hits 35745 35749 +4
Misses 724 724
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #814 +/- ##
=======================================
Coverage 98.01% 98.01%
=======================================
Files 444 444
Lines 36469 36473 +4
=======================================
+ Hits 35745 35749 +4
Misses 724 724
Flags with carried forward coverage won't be shown. Click here to find out more.
|
services/bundle_analysis/report.py
Outdated
| result="upload_error" if self.error else "processed", | ||
| plugin_name="n/a", | ||
| repository=self.commit.repository.repoid, | ||
| ).inc() | ||
| except Exception: |
There was a problem hiding this comment.
I wonder if this is actually fallible?
having a fn indirection just to slap a try/except on this call seems a bit weird, in particular as the call with the extra labels argument is a bit more unwieldy than just using the prometheus API directly.
There was a problem hiding this comment.
That's a good point, I think the existing code with the try/except block should work as is, and the inc_counter change here is not necessary.
Another thing I'm curious about this metric though is that there are 4 different results, some number of unique plugin_name and so for each unique repo_id there will be 4*count(plugin_name) time series created, and such high cardinality might be a performance issue with Prometheus. What do you think about this or am I overlooking anything?
There was a problem hiding this comment.
You are absolutely right.
IMO its okay to just remove the repo_id tag here. You can also check in with @adrian-codecov who has recently worked on getting some limited form of repo-tagged metrics going using an allowlist to only tag very specific repos.
There was a problem hiding this comment.
I've removed repo_id for now. The allowlist method can probably be implemented later when we have determined which label(s) a repo_id should be classified into.
|
This PR includes changes to |
Replace Sentry metrics with Prometheus metrics.
Closes https://github.com/codecov/infrastructure-team/issues/461.
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.