Skip to content
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

refactor: default to prometheus.DefaultRegisterer #722

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lidel
Copy link
Member

@lidel lidel commented Nov 18, 2024

This PR contains changes on top of #718

It replaces prometheus.NewRegistry() calls with prometheus.DefaultRegisterer to ensure that by default boxo users who do not specify an explicit registry are not missing some of the metrics.

This convention is what go-libp2p does already, and I feel boxo should follow, to remove confusion why some metrics are present, and some are missing.

TODO

@lidel lidel requested a review from 2color November 18, 2024 22:05
@lidel lidel changed the title refactor: remove prometheus.NewRegistry() refactor: defult to prometheus.DefaultRegisterer Nov 18, 2024
@lidel lidel force-pushed the use-default-metrics-registry branch 3 times, most recently from 119816e to fb39749 Compare November 18, 2024 22:40
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 76.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 60.33%. Comparing base (8673560) to head (6b292d9).

Files with missing lines Patch % Lines
gateway/backend_car.go 72.72% 2 Missing and 1 partial ⚠️
gateway/blockstore.go 0.00% 3 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #722      +/-   ##
==========================================
+ Coverage   60.31%   60.33%   +0.02%     
==========================================
  Files         244      244              
  Lines       31034    31036       +2     
==========================================
+ Hits        18717    18725       +8     
+ Misses      10644    10639       -5     
+ Partials     1673     1672       -1     
Files with missing lines Coverage Δ
gateway/metrics.go 82.85% <100.00%> (+0.28%) ⬆️
routing/http/server/server.go 74.57% <100.00%> (+0.26%) ⬆️
gateway/backend_car.go 50.00% <72.72%> (-0.47%) ⬇️
gateway/blockstore.go 0.00% <0.00%> (ø)

... and 14 files with indirect coverage changes

@lidel lidel changed the title refactor: defult to prometheus.DefaultRegisterer refactor: default to prometheus.DefaultRegisterer Nov 18, 2024
@lidel lidel changed the base branch from add-instrumentation to main November 19, 2024 00:07
@lidel lidel force-pushed the use-default-metrics-registry branch from fb39749 to edd18aa Compare November 19, 2024 00:09
@lidel
Copy link
Member Author

lidel commented Nov 25, 2024

I think the approach of special-casing tests is too tedious, especially if people elaready use boxo modules in their tests, we will be breaking them forcing people to add similar wrappers.

We have a different approach in boxo/gateway/metrics where we catch "duplicate registration" error and log error instead. This way all tests can run in parallel without extra wrapper.

So next steps here:

  • undo changes to tests and refactor to use approach from gateway

@lidel lidel force-pushed the use-default-metrics-registry branch from edd18aa to 678be0d Compare November 26, 2024 20:20
@lidel
Copy link
Member Author

lidel commented Nov 26, 2024

Pushed refactored version.

Boxo will no longer panic when prometheus.DefaultRegisterer is used, and someone runs ours (or theirs) tests in parallel.

This way

  • things work: boxo users are never missing metrics when they run with defaults
  • ux remains good: existing boxo users will not have to refactor their existing tests due to vague panic occuring after boxo update

@lidel lidel force-pushed the use-default-metrics-registry branch from 678be0d to e345d49 Compare November 26, 2024 20:34
replace NewRegistry() call with global `prometheus.DefaultRegisterer`
so by default boxo users who did not specify custom registry
are not missing any metrics
@lidel lidel force-pushed the use-default-metrics-registry branch from e345d49 to 6b292d9 Compare November 26, 2024 20:36
@lidel lidel marked this pull request as ready for review November 26, 2024 21:00
@lidel lidel requested a review from a team as a code owner November 26, 2024 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant