Skip to content

Commit

Permalink
refactor: remove prometheus.NewRegistry()
Browse files Browse the repository at this point in the history
replace it with prometheus.DefaultRegisterer
so by default boxo users who do not specify registry
are not missing metrics
  • Loading branch information
lidel committed Nov 18, 2024
1 parent 2d8efc3 commit 618c7f2
Show file tree
Hide file tree
Showing 6 changed files with 35 additions and 21 deletions.
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ The following emojis are used to highlight certain changes:

### Added

- `routing/http/server`: added Prometheus instrumentation to http delegated routing endpoints.
- `gateway`: `NewCacheBlockStore` and `NewCarBackend` will use `prometheus.DefaultRegisterer` when a custom one is not specified via `WithPrometheusRegistry`
- `routing/http/server`: exposes Prometheus metrics on `prometheus.DefaultRegisterer` and a custom one can be provided via `WithPrometheusRegistry`

### Changed

Expand Down
2 changes: 1 addition & 1 deletion gateway/backend_car.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func NewCarBackend(f CarFetcher, opts ...BackendOption) (*CarBackend, error) {
return nil, err
}

var promReg prometheus.Registerer = prometheus.NewRegistry()
var promReg prometheus.Registerer = prometheus.DefaultRegisterer
if compiledOptions.promRegistry != nil {
promReg = compiledOptions.promRegistry
}
Expand Down
23 changes: 15 additions & 8 deletions gateway/backend_car_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,20 @@ import (
carv2 "github.com/ipld/go-car/v2"
carbs "github.com/ipld/go-car/v2/blockstore"
"github.com/ipld/go-car/v2/storage"
"github.com/prometheus/client_golang/prometheus"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

//go:embed testdata/directory-with-multilayer-hamt-and-multiblock-files.car
var dirWithMultiblockHAMTandFiles []byte

func getTestNewCarBackend(f CarFetcher, opts ...BackendOption) (*CarBackend, error) {
// unique prometheus registry to allow for tests to run in parallell without registration conflict
opts = append(opts, WithPrometheusRegistry(prometheus.NewRegistry()))
return NewCarBackend(f, opts...)
}

func TestCarBackendTar(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
Expand Down Expand Up @@ -161,7 +168,7 @@ func TestCarBackendTar(t *testing.T) {
fetcher, err := NewRetryCarFetcher(bs, 3)
require.NoError(t, err)

backend, err := NewCarBackend(fetcher)
backend, err := getTestNewCarBackend(fetcher)
require.NoError(t, err)

p := path.FromCid(cid.MustParse("bafybeid3fd2xxdcd3dbj7trb433h2aqssn6xovjbwnkargjv7fuog4xjdi"))
Expand Down Expand Up @@ -331,7 +338,7 @@ func TestCarBackendTarAtEndOfPath(t *testing.T) {
fetcher, err := NewRetryCarFetcher(bs, 3)
require.NoError(t, err)

backend, err := NewCarBackend(fetcher)
backend, err := getTestNewCarBackend(fetcher)
require.NoError(t, err)

p, err := path.Join(path.FromCid(cid.MustParse("bafybeid3fd2xxdcd3dbj7trb433h2aqssn6xovjbwnkargjv7fuog4xjdi")), "hamtDir")
Expand Down Expand Up @@ -493,7 +500,7 @@ func TestCarBackendGetFile(t *testing.T) {
fetcher, err := NewRetryCarFetcher(bs, 3)
require.NoError(t, err)

backend, err := NewCarBackend(fetcher)
backend, err := getTestNewCarBackend(fetcher)
require.NoError(t, err)

trustedGatewayServer := httptest.NewServer(NewHandler(Config{DeserializedResponses: true}, backend))
Expand Down Expand Up @@ -597,7 +604,7 @@ func TestCarBackendGetFileRangeRequest(t *testing.T) {
fetcher, err := NewRetryCarFetcher(bs, 3)
require.NoError(t, err)

backend, err := NewCarBackend(fetcher)
backend, err := getTestNewCarBackend(fetcher)
require.NoError(t, err)

trustedGatewayServer := httptest.NewServer(NewHandler(Config{DeserializedResponses: true}, backend))
Expand Down Expand Up @@ -707,7 +714,7 @@ func TestCarBackendGetFileWithBadBlockReturned(t *testing.T) {
fetcher, err := NewRetryCarFetcher(bs, 3)
require.NoError(t, err)

backend, err := NewCarBackend(fetcher)
backend, err := getTestNewCarBackend(fetcher)
require.NoError(t, err)

trustedGatewayServer := httptest.NewServer(NewHandler(Config{DeserializedResponses: true}, backend))
Expand Down Expand Up @@ -812,7 +819,7 @@ func TestCarBackendGetHAMTDirectory(t *testing.T) {
fetcher, err := NewRetryCarFetcher(bs, 3)
require.NoError(t, err)

backend, err := NewCarBackend(fetcher)
backend, err := getTestNewCarBackend(fetcher)
require.NoError(t, err)

trustedGatewayServer := httptest.NewServer(NewHandler(Config{DeserializedResponses: true}, backend))
Expand Down Expand Up @@ -921,7 +928,7 @@ func TestCarBackendGetCAR(t *testing.T) {
fetcher, err := NewRetryCarFetcher(bs, 3)
require.NoError(t, err)

backend, err := NewCarBackend(fetcher)
backend, err := getTestNewCarBackend(fetcher)
require.NoError(t, err)

p := path.FromCid(cid.MustParse("bafybeid3fd2xxdcd3dbj7trb433h2aqssn6xovjbwnkargjv7fuog4xjdi"))
Expand Down Expand Up @@ -1031,7 +1038,7 @@ func TestCarBackendPassthroughErrors(t *testing.T) {
}}, 3)
require.NoError(t, err)

backend, err := NewCarBackend(fetcher)
backend, err := getTestNewCarBackend(fetcher)
require.NoError(t, err)

err = traversal(ctx, imPath, backend)
Expand Down
5 changes: 2 additions & 3 deletions gateway/blockstore.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,16 +35,15 @@ var _ blockstore.Blockstore = (*cacheBlockStore)(nil)
// in memory using a two queue cache. It can be useful, for example, when paired
// with a proxy blockstore (see [NewRemoteBlockstore]).
//
// If the given [prometheus.Registerer] is nil, a new one will be created using
// [prometheus.NewRegistry].
// If the given [prometheus.Registerer] is nil, a [prometheus.DefaultRegisterer] will be used.
func NewCacheBlockStore(size int, reg prometheus.Registerer) (blockstore.Blockstore, error) {
c, err := lru.New2Q[string, []byte](size)
if err != nil {
return nil, err
}

if reg == nil {
reg = prometheus.NewRegistry()
reg = prometheus.DefaultRegisterer
}

cacheHitsMetric := prometheus.NewCounter(prometheus.CounterOpts{
Expand Down
2 changes: 1 addition & 1 deletion routing/http/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ func Handler(svc ContentRouter, opts ...Option) http.Handler {
}

if server.promRegistry == nil {
server.promRegistry = prometheus.NewRegistry()
server.promRegistry = prometheus.DefaultRegisterer
}

// Create middleware with prometheus recorder
Expand Down
21 changes: 14 additions & 7 deletions routing/http/server/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,20 @@ import (
"github.com/libp2p/go-libp2p/core/routing"
b58 "github.com/mr-tron/base58/base58"
"github.com/multiformats/go-multiaddr"
"github.com/prometheus/client_golang/prometheus"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
)

func getTestHandler(router ContentRouter, opts ...Option) http.Handler {
// unique prometheus registry to allow for tests to run in parallell without registration conflict
opts = append(opts, WithPrometheusRegistry(prometheus.NewRegistry()))
return Handler(router, opts...)
}

func TestHeaders(t *testing.T) {
router := &mockContentRouter{}
server := httptest.NewServer(Handler(router))
server := httptest.NewServer(getTestHandler(router))
t.Cleanup(server.Close)
serverAddr := "http://" + server.Listener.Addr().String()

Expand Down Expand Up @@ -144,7 +151,7 @@ func TestProviders(t *testing.T) {
}

router := &mockContentRouter{}
server := httptest.NewServer(Handler(router))
server := httptest.NewServer(getTestHandler(router))
t.Cleanup(server.Close)
serverAddr := "http://" + server.Listener.Addr().String()
limit := DefaultRecordsLimit
Expand Down Expand Up @@ -246,7 +253,7 @@ func TestProviders(t *testing.T) {
t.Run("404 when router returns routing.ErrNotFound", func(t *testing.T) {
t.Parallel()
router := &mockContentRouter{}
server := httptest.NewServer(Handler(router))
server := httptest.NewServer(getTestHandler(router))
t.Cleanup(server.Close)
serverAddr := "http://" + server.Listener.Addr().String()
router.On("FindProviders", mock.Anything, cid, DefaultRecordsLimit).Return(nil, routing.ErrNotFound)
Expand All @@ -262,7 +269,7 @@ func TestProviders(t *testing.T) {

func TestPeers(t *testing.T) {
makeRequest := func(t *testing.T, router *mockContentRouter, contentType, arg, filterAddrs, filterProtocols string) *http.Response {
server := httptest.NewServer(Handler(router))
server := httptest.NewServer(getTestHandler(router))
t.Cleanup(server.Close)

urlStr := fmt.Sprintf("http://%s/routing/v1/peers/%s", server.Listener.Addr().String(), arg)
Expand Down Expand Up @@ -662,7 +669,7 @@ func TestIPNS(t *testing.T) {
require.NoError(t, err)

makeRequest := func(t *testing.T, router *mockContentRouter, path string, accept string) *http.Response {
server := httptest.NewServer(Handler(router))
server := httptest.NewServer(getTestHandler(router))
t.Cleanup(server.Close)
serverAddr := "http://" + server.Listener.Addr().String()
urlStr := serverAddr + path
Expand Down Expand Up @@ -814,7 +821,7 @@ func TestIPNS(t *testing.T) {
router := &mockContentRouter{}
router.On("PutIPNS", mock.Anything, name1, record1).Return(nil)

server := httptest.NewServer(Handler(router))
server := httptest.NewServer(getTestHandler(router))
t.Cleanup(server.Close)
serverAddr := "http://" + server.Listener.Addr().String()
urlStr := serverAddr + "/routing/v1/ipns/" + name1.String()
Expand All @@ -833,7 +840,7 @@ func TestIPNS(t *testing.T) {

router := &mockContentRouter{}

server := httptest.NewServer(Handler(router))
server := httptest.NewServer(getTestHandler(router))
t.Cleanup(server.Close)
serverAddr := "http://" + server.Listener.Addr().String()
urlStr := serverAddr + "/routing/v1/ipns/" + name2.String()
Expand Down

0 comments on commit 618c7f2

Please sign in to comment.