From fb39749e4d15af097820a83f368b1b6e15690517 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Mon, 18 Nov 2024 23:00:44 +0100 Subject: [PATCH] refactor: remove prometheus.NewRegistry() replace it with prometheus.DefaultRegisterer so by default boxo users who do not specify registry are not missing metrics --- CHANGELOG.md | 3 ++- examples/car-file-fetcher/main.go | 8 +++++++- gateway/backend_car.go | 2 +- gateway/backend_car_test.go | 23 +++++++++++++++-------- gateway/blockstore.go | 5 ++--- routing/http/client/client_test.go | 2 ++ routing/http/server/server.go | 2 +- routing/http/server/server_test.go | 21 ++++++++++++++------- 8 files changed, 44 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index faca07433..1b8efa006 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/examples/car-file-fetcher/main.go b/examples/car-file-fetcher/main.go index 96458be5f..e14513bbd 100644 --- a/examples/car-file-fetcher/main.go +++ b/examples/car-file-fetcher/main.go @@ -12,6 +12,7 @@ import ( "github.com/ipfs/boxo/files" "github.com/ipfs/boxo/gateway" "github.com/ipfs/boxo/path" + "github.com/prometheus/client_golang/prometheus" ) func main() { @@ -54,10 +55,15 @@ func fetch(gatewayURL, ipfsPath, outputPath, userAgent string, limit int64) erro }, } + // Most of the time you want to use prometheus.DefaultRegisterer, + // but here we create custom one to demonstrate how custom configuration can + // passed to the backend (and to allow tests to run in parallel) + metrics := prometheus.NewRegistry() + // Create the remote CAR gateway backend pointing to the given gateway URL and // using our [http.Client]. A custom [http.Client] is not required and the called // function would create a new one instead. - backend, err := gateway.NewRemoteCarBackend([]string{gatewayURL}, httpClient) + backend, err := gateway.NewRemoteCarBackend([]string{gatewayURL}, httpClient, gateway.WithPrometheusRegistry(metrics)) if err != nil { return err } diff --git a/gateway/backend_car.go b/gateway/backend_car.go index d2b33a0fc..da3a0521d 100644 --- a/gateway/backend_car.go +++ b/gateway/backend_car.go @@ -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 } diff --git a/gateway/backend_car_test.go b/gateway/backend_car_test.go index eebd8e19b..d757573ba 100644 --- a/gateway/backend_car_test.go +++ b/gateway/backend_car_test.go @@ -24,6 +24,7 @@ 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" ) @@ -31,6 +32,12 @@ import ( //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() @@ -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")) @@ -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") @@ -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)) @@ -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)) @@ -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)) @@ -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)) @@ -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")) @@ -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) diff --git a/gateway/blockstore.go b/gateway/blockstore.go index 68cd729ac..8ced5d0b4 100644 --- a/gateway/blockstore.go +++ b/gateway/blockstore.go @@ -35,8 +35,7 @@ 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 { @@ -44,7 +43,7 @@ func NewCacheBlockStore(size int, reg prometheus.Registerer) (blockstore.Blockst } if reg == nil { - reg = prometheus.NewRegistry() + reg = prometheus.DefaultRegisterer } cacheHitsMetric := prometheus.NewCounter(prometheus.CounterOpts{ diff --git a/routing/http/client/client_test.go b/routing/http/client/client_test.go index da36f666f..5c707cbe1 100644 --- a/routing/http/client/client_test.go +++ b/routing/http/client/client_test.go @@ -18,6 +18,7 @@ import ( "github.com/ipfs/boxo/routing/http/types" "github.com/ipfs/boxo/routing/http/types/iter" "github.com/ipfs/go-cid" + "github.com/prometheus/client_golang/prometheus" "github.com/libp2p/go-libp2p/core/crypto" "github.com/libp2p/go-libp2p/core/peer" @@ -99,6 +100,7 @@ func makeTestDeps(t *testing.T, clientsOpts []Option, serverOpts []server.Option const testUserAgent = "testUserAgent" peerID, addrs, identity := makeProviderAndIdentity() router := &mockContentRouter{} + serverOpts = append(serverOpts, server.WithPrometheusRegistry(prometheus.NewRegistry())) // avoid panic: duplicate metrics collector registration recordingHandler := &recordingHandler{ Handler: server.Handler(router, serverOpts...), f: []func(*http.Request){ diff --git a/routing/http/server/server.go b/routing/http/server/server.go index 0650092b1..c9241e856 100644 --- a/routing/http/server/server.go +++ b/routing/http/server/server.go @@ -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 diff --git a/routing/http/server/server_test.go b/routing/http/server/server_test.go index bf84e4155..455930253 100644 --- a/routing/http/server/server_test.go +++ b/routing/http/server/server_test.go @@ -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() @@ -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 @@ -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) @@ -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) @@ -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 @@ -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() @@ -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()