refactor: Use package local metrics #1605
Conversation
SuperQ
force-pushed
the
local_metrics_2
branch
2 times, most recently
from
9d1d131
to
54b3271
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1605 +/- ##
==========================================
- Coverage 93.88% 93.85% -0.04%
==========================================
Files 78 80 +2
Lines 6361 6574 +213
==========================================
+ Hits 5972 6170 +198
- Misses 300 320 +20
+ Partials 89 84 -5 ☔ View full report in Codecov by Sentry. |
|
I'm working on an improvement to the tests using the client_golang testutil. |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
Not really stale, but I haven't had a chance to work on the coverage change. |
Part 2 (0xERR0R#1578) Improve performance of metrics by moving them to the package that needs them. This reduces the overhead to a simple atomic increment for basic counters like cache hits/misses. This also uses `promauto` to avoid the second step of having to register metrics. Signed-off-by: SuperQ <[email protected]>
SuperQ
force-pushed
the
local_metrics_2
branch
from
54b3271
to
823c916
Compare
|
@0xERR0R, how do you feel about the coverage change here? |
| var ( | ||
| lastListGroupRefreshTimestamp = promauto.With(metrics.Reg).NewGauge( | ||
| prometheus.GaugeOpts{ | ||
| Name: "blocky_last_list_group_refresh_timestamp_seconds", |
There was a problem hiding this comment.
Do we have the same definition in metrics_event_publisher.go?
| ) | ||
| BeforeEach(func() { | ||
| var err error | ||
|
|
||
| sutConfig, err = config.WithDefaults[config.Downloader]() | ||
| Expect(err).Should(Succeed()) | ||
|
|
||
| failedDownloadCountEvtChannel = make(chan string, 5) |
There was a problem hiding this comment.
now we don't test if metrics counters are being incremented. Maybe we can try to read the counter value and assert it?
There was a problem hiding this comment.
Yes, that is something we could test for. But I find it not very useful. The Prometheus client Inc() type functions are basically a simple Go atomic increment. There's basically no way for it to go wrong in a way that testing would help with.
It is always nice to cover more code, but it is not an ultimate goal to have 100% coverage :) |
Part 2 (#1578)
Improve performance of metrics by moving them to the package that needs them. This reduces the overhead to a simple atomic increment for basic counters like cache hits/misses. This also uses
promautoto avoid the second step of having to register metrics.