From 181585b23d1669ecdbd70e4536d5272c266b9d23 Mon Sep 17 00:00:00 2001
From: Christian Simon <simon@swine.de>
Date: Thu, 19 Dec 2024 14:47:51 +0000
Subject: [PATCH] fix(pyroscope.receive_http): Handle metrics registry
 gracefully

This component did not handle duplciate registrations well.

This fixes #2285.
---
 .../pyroscope/receive_http/receive_http.go    | 28 +++++++++----
 .../receive_http/receive_http_test.go         | 42 +++++++++++++++++++
 2 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/internal/component/pyroscope/receive_http/receive_http.go b/internal/component/pyroscope/receive_http/receive_http.go
index 35a1240273..c17868b427 100644
--- a/internal/component/pyroscope/receive_http/receive_http.go
+++ b/internal/component/pyroscope/receive_http/receive_http.go
@@ -10,6 +10,7 @@ import (
 	"sync"
 
 	"github.com/gorilla/mux"
+	"github.com/prometheus/client_golang/prometheus"
 	"golang.org/x/sync/errgroup"
 
 	"github.com/grafana/alloy/internal/component"
@@ -18,6 +19,7 @@ import (
 	"github.com/grafana/alloy/internal/component/pyroscope/write"
 	"github.com/grafana/alloy/internal/featuregate"
 	"github.com/grafana/alloy/internal/runtime/logging/level"
+	"github.com/grafana/alloy/internal/util"
 )
 
 const (
@@ -53,16 +55,21 @@ func (a *Arguments) SetToDefault() {
 }
 
 type Component struct {
-	opts        component.Options
-	server      *fnet.TargetServer
-	appendables []pyroscope.Appendable
-	mut         sync.Mutex
+	opts               component.Options
+	server             *fnet.TargetServer
+	uncheckedCollector *util.UncheckedCollector
+	appendables        []pyroscope.Appendable
+	mut                sync.Mutex
 }
 
 func New(opts component.Options, args Arguments) (*Component, error) {
+	uncheckedCollector := util.NewUncheckedCollector(nil)
+	opts.Registerer.MustRegister(uncheckedCollector)
+
 	c := &Component{
-		opts:        opts,
-		appendables: args.ForwardTo,
+		opts:               opts,
+		uncheckedCollector: uncheckedCollector,
+		appendables:        args.ForwardTo,
 	}
 
 	if err := c.Update(args); err != nil {
@@ -116,7 +123,14 @@ func (c *Component) Update(args component.Arguments) error {
 
 	c.shutdownServer()
 
-	srv, err := fnet.NewTargetServer(c.opts.Logger, "pyroscope_receive_http", c.opts.Registerer, newArgs.Server)
+	// [server.Server] registers new metrics every time it is created. To
+	// avoid issues with re-registering metrics with the same name, we create a
+	// new registry for the server every time we create one, and pass it to an
+	// unchecked collector to bypass uniqueness checking.
+	serverRegistry := prometheus.NewRegistry()
+	c.uncheckedCollector.SetCollector(serverRegistry)
+
+	srv, err := fnet.NewTargetServer(c.opts.Logger, "pyroscope_receive_http", serverRegistry, newArgs.Server)
 	if err != nil {
 		return fmt.Errorf("failed to create server: %w", err)
 	}
diff --git a/internal/component/pyroscope/receive_http/receive_http_test.go b/internal/component/pyroscope/receive_http/receive_http_test.go
index 71929abce6..5c507016cb 100644
--- a/internal/component/pyroscope/receive_http/receive_http_test.go
+++ b/internal/component/pyroscope/receive_http/receive_http_test.go
@@ -287,3 +287,45 @@ func testOptions(t *testing.T) component.Options {
 		Registerer: prometheus.NewRegistry(),
 	}
 }
+
+// TestUpdateArgs verifies that the component can be updated with new arguments. This explictly also makes sure that the server is restarted when the server configuration changes. And there are no metric registration conflicts.
+func TestUpdateArgs(t *testing.T) {
+	ports, err := freeport.GetFreePorts(2)
+	require.NoError(t, err)
+
+	forwardTo := []pyroscope.Appendable{testAppendable(nil)}
+
+	args := Arguments{
+		Server: &fnet.ServerConfig{
+			HTTP: &fnet.HTTPConfig{
+				ListenAddress: "localhost",
+				ListenPort:    ports[0],
+			},
+		},
+		ForwardTo: forwardTo,
+	}
+
+	comp, err := New(testOptions(t), args)
+	require.NoError(t, err)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	t.Cleanup(cancel)
+
+	go func() {
+		require.NoError(t, comp.Run(ctx))
+	}()
+
+	waitForServerReady(t, ports[0])
+
+	comp.Update(Arguments{
+		Server: &fnet.ServerConfig{
+			HTTP: &fnet.HTTPConfig{
+				ListenAddress: "localhost",
+				ListenPort:    ports[1],
+			},
+		},
+		ForwardTo: forwardTo,
+	})
+
+	waitForServerReady(t, ports[1])
+}