Refactor metrics client out of sink and use di

agent/hcp/client/client.go

@@ -313,7 +313,7 @@ func (t *TelemetryConfig) Enabled() (string, bool) {
 }
 
 // DefaultLabels returns a set of <key, value> string pairs that must be added as attributes to all exported telemetry data.
-func (t *TelemetryConfig) DefaultLabels(cfg CloudConfig) map[string]string {
+func (t *TelemetryConfig) DefaultLabels(cfg config.CloudConfig) map[string]string {
 	labels := make(map[string]string)
 	nID, nodeName := cfg.NodeMeta()
 	nodeID := string(nID)

agent/hcp/client/client_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"testing"
 
+	"github.com/hashicorp/consul/agent/hcp/config"
 	"github.com/hashicorp/consul/types"
 	"github.com/hashicorp/hcp-sdk-go/clients/cloud-consul-telemetry-gateway/preview/2023-04-14/client/consul_telemetry_service"
 	"github.com/hashicorp/hcp-sdk-go/clients/cloud-consul-telemetry-gateway/preview/2023-04-14/models"
@@ -151,11 +152,11 @@ func TestConvertTelemetryConfig(t *testing.T) {
 
 func Test_DefaultLabels(t *testing.T) {
 	for name, tc := range map[string]struct {
-		mockCloudCfg   CloudConfig
+		cfg            config.CloudConfig
 		expectedLabels map[string]string
 	}{
 		"Success": {
-			mockCloudCfg: MockCloudCfg{
+			cfg: config.CloudConfig{
 				NodeID:   types.NodeID("nodeyid"),
 				NodeName: "nodey",
 			},
@@ -166,7 +167,7 @@ func Test_DefaultLabels(t *testing.T) {
 		},
 
 		"NoNodeID": {
-			mockCloudCfg: MockCloudCfg{
+			cfg: config.CloudConfig{
 				NodeID:   types.NodeID(""),
 				NodeName: "nodey",
 			},
@@ -175,7 +176,7 @@ func Test_DefaultLabels(t *testing.T) {
 			},
 		},
 		"NoNodeName": {
-			mockCloudCfg: MockCloudCfg{
+			cfg: config.CloudConfig{
 				NodeID:   types.NodeID("nodeyid"),
 				NodeName: "",
 			},
@@ -184,18 +185,16 @@ func Test_DefaultLabels(t *testing.T) {
 			},
 		},
 		"Empty": {
-			mockCloudCfg: MockCloudCfg{
+			cfg: config.CloudConfig{
 				NodeID:   "",
 				NodeName: "",
 			},
-			expectedLabels: map[string]string{
-				"node_id": "nodeyid",
-			},
+			expectedLabels: map[string]string{},
 		},
 	} {
 		t.Run(name, func(t *testing.T) {
 			tCfg := &TelemetryConfig{}
-			labels := tCfg.DefaultLabels(tc.mockCloudCfg)
+			labels := tCfg.DefaultLabels(tc.cfg)
 			require.Equal(t, labels, tc.expectedLabels)
 		})
 	}

agent/hcp/client/metrics_client.go

@@ -56,7 +56,7 @@ type otlpClient struct {
 
 // NewMetricsClient returns a configured MetricsClient.
 // The current implementation uses otlpClient to provide retry functionality.
-func NewMetricsClient(cfg CloudConfig, ctx context.Context) (MetricsClient, error) {
+func NewMetricsClient(ctx context.Context, cfg CloudConfig) (MetricsClient, error) {
 	if cfg == nil {
 		return nil, fmt.Errorf("failed to init telemetry client: provide valid cloudCfg (Cloud Configuration for TLS)")
 	}

agent/hcp/client/metrics_client_test.go

@@ -51,7 +51,7 @@ func TestNewMetricsClient(t *testing.T) {
 		},
 	} {
 		t.Run(name, func(t *testing.T) {
-			client, err := NewMetricsClient(test.cfg, test.ctx)
+			client, err := NewMetricsClient(test.ctx, test.cfg)
 			if test.wantErr != "" {
 				require.Error(t, err)
 				require.Contains(t, err.Error(), test.wantErr)
@@ -95,7 +95,7 @@ func TestExportMetrics(t *testing.T) {
 			}))
 			defer srv.Close()
 
-			client, err := NewMetricsClient(MockCloudCfg{}, context.Background())
+			client, err := NewMetricsClient(context.Background(), MockCloudCfg{})
 			require.NoError(t, err)
 
 			ctx := context.Background()

agent/hcp/deps.go

@@ -25,6 +25,10 @@ type Deps struct {
 }
 
 func NewDeps(cfg config.CloudConfig, logger hclog.Logger) (Deps, error) {
+	logger = logger.Named("sink")
+	ctx := context.Background()
+	ctx = hclog.WithContext(ctx, logger)
+
 	client, err := hcpclient.NewClient(cfg)
 	if err != nil {
 		return Deps{}, fmt.Errorf("failed to init client: %w", err)
@@ -35,7 +39,13 @@ func NewDeps(cfg config.CloudConfig, logger hclog.Logger) (Deps, error) {
 		return Deps{}, fmt.Errorf("failed to init scada: %w", err)
 	}
 
-	sink := sink(client, &cfg, logger.Named("sink"))
+	metricsClient, err := hcpclient.NewMetricsClient(ctx, &cfg)
+	if err != nil {
+		logger.Error("failed to init metrics client", "error", err)
+		return Deps{}, fmt.Errorf("failed to init metrics client: %w", err)
+	}
+
+	sink := sink(ctx, client, metricsClient, cfg)
 
 	return Deps{
 		Client:   client,
@@ -47,10 +57,13 @@ func NewDeps(cfg config.CloudConfig, logger hclog.Logger) (Deps, error) {
 // sink provides initializes an OTELSink which forwards Consul metrics to HCP.
 // The sink is only initialized if the server is registered with the management plane (CCM).
 // This step should not block server initialization, so errors are logged, but not returned.
-func sink(hcpClient hcpclient.Client, cfg hcpclient.CloudConfig, logger hclog.Logger) metrics.MetricSink {
-	ctx := context.Background()
-	ctx = hclog.WithContext(ctx, logger)
-
+func sink(
+	ctx context.Context,
+	hcpClient hcpclient.Client,
+	metricsClient hcpclient.MetricsClient,
+	cfg config.CloudConfig,
+) metrics.MetricSink {
+	logger := hclog.FromContext(ctx)
 	reqCtx, cancel := context.WithTimeout(ctx, 5*time.Second)
 	defer cancel()
 
@@ -71,12 +84,6 @@ func sink(hcpClient hcpclient.Client, cfg hcpclient.CloudConfig, logger hclog.Lo
 		return nil
 	}
 
-	metricsClient, err := hcpclient.NewMetricsClient(cfg, ctx)
-	if err != nil {
-		logger.Error("failed to init metrics client", "error", err)
-		return nil
-	}
-
 	sinkOpts := &telemetry.OTELSinkOpts{
 		Ctx:     ctx,
 		Reader:  telemetry.NewOTELReader(metricsClient, u, telemetry.DefaultExportInterval),

agent/hcp/deps_test.go

@@ -1,10 +1,11 @@
 package hcp
 
 import (
+	"context"
 	"fmt"
 	"testing"
 
-	"github.com/hashicorp/go-hclog"
+	"github.com/hashicorp/consul/agent/hcp/config"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 
@@ -16,7 +17,7 @@ func TestSink(t *testing.T) {
 	t.Parallel()
 	for name, test := range map[string]struct {
 		expect       func(*client.MockClient)
-		mockCloudCfg client.CloudConfig
+		cloudCfg     config.CloudConfig
 		expectedSink bool
 	}{
 		"success": {
@@ -28,7 +29,7 @@ func TestSink(t *testing.T) {
 					},
 				}, nil)
 			},
-			mockCloudCfg: client.MockCloudCfg{
+			cloudCfg: config.CloudConfig{
 				NodeID:   types.NodeID("nodeyid"),
 				NodeName: "nodey",
 			},
@@ -43,26 +44,13 @@ func TestSink(t *testing.T) {
 					},
 				}, nil)
 			},
-			mockCloudCfg: client.MockCloudCfg{},
+			cloudCfg: config.CloudConfig{},
 		},
 		"noSinkWhenCCMVerificationFails": {
 			expect: func(mockClient *client.MockClient) {
 				mockClient.EXPECT().FetchTelemetryConfig(mock.Anything).Return(nil, fmt.Errorf("fetch failed"))
 			},
-			mockCloudCfg: client.MockCloudCfg{},
-		},
-		"noSinkWhenMetricsClientInitFails": {
-			mockCloudCfg: client.MockCloudCfg{
-				ConfigErr: fmt.Errorf("test bad hcp config"),
-			},
-			expect: func(mockClient *client.MockClient) {
-				mockClient.EXPECT().FetchTelemetryConfig(mock.Anything).Return(&client.TelemetryConfig{
-					Endpoint: "https://test.com",
-					MetricsConfig: &client.MetricsConfig{
-						Endpoint: "",
-					},
-				}, nil)
-			},
+			cloudCfg: config.CloudConfig{},
 		},
 		"failsWithFetchTelemetryFailure": {
 			expect: func(mockClient *client.MockClient) {
@@ -96,9 +84,12 @@ func TestSink(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 			c := client.NewMockClient(t)
-			l := hclog.NewNullLogger()
+			mc := client.MockMetricsClient{}
+
 			test.expect(c)
-			s := sink(c, test.mockCloudCfg, l)
+			ctx := context.Background()
+
+			s := sink(ctx, c, mc, test.cloudCfg)
 			if !test.expectedSink {
 				require.Nil(t, s)
 				return