Address PR feedback

hashicorp · Apr 28, 2023 · cbbf5ef · cbbf5ef
1 parent 3f875e9
commit cbbf5ef
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 66 deletions.
diff --git a/agent/hcp/client/metrics_client.go b/agent/hcp/client/metrics_client.go
@@ -8,24 +8,22 @@ import (
 	"net/http"
 	"time"
 
-	"golang.org/x/oauth2"
-	"google.golang.org/protobuf/proto"
-
-	colmetricpb "go.opentelemetry.io/proto/otlp/collector/metrics/v1"
-	metricpb "go.opentelemetry.io/proto/otlp/metrics/v1"
-
-	"github.com/hashicorp/consul/version"
 	"github.com/hashicorp/go-cleanhttp"
 	"github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/go-retryablehttp"
 	hcpcfg "github.com/hashicorp/hcp-sdk-go/config"
+	colmetricpb "go.opentelemetry.io/proto/otlp/collector/metrics/v1"
+	metricpb "go.opentelemetry.io/proto/otlp/metrics/v1"
+	"golang.org/x/oauth2"
+	"google.golang.org/protobuf/proto"
 )
 
 const (
 	// HTTP Client config
 	defaultStreamTimeout = 15 * time.Second
 
 	// Retry config
+	// TODO: Evenutally, we'd like to configure these values dynamically.
 	defaultRetryWaitMin = 1 * time.Second
 	defaultRetryWaitMax = 15 * time.Second
 	defaultRetryMax     = 4
@@ -44,36 +42,32 @@ type cloudConfig interface {
 // otlpClient is an implementation of MetricsClient with a retryable http client for retries and to honor throttle.
 // It also holds default HTTP headers to add to export requests.
 type otlpClient struct {
-	client  *retryablehttp.Client
-	headers map[string]string
-}
-
-// TelemetryClientCfg is used to configure the MetricsClient.
-type TelemetryClientCfg struct {
-	CloudCfg cloudConfig
-	Logger   hclog.Logger
+	client *retryablehttp.Client
+	header *http.Header
 }
 
 // NewMetricsClient returns a configured MetricsClient.
 // The current implementation uses otlpClient to provide retry functionality.
-func NewMetricsClient(cfg *TelemetryClientCfg) (MetricsClient, error) {
-	if cfg.CloudCfg == nil || cfg.Logger == nil {
-		return nil, fmt.Errorf("failed to init telemetry client: provide valid TelemetryClientCfg")
+func NewMetricsClient(cfg cloudConfig, logger hclog.Logger) (MetricsClient, error) {
+	if cfg == nil {
+		return nil, fmt.Errorf("failed to init telemetry client: provide valid cloudCfg (Cloud Configuration for TLS)")
+	}
+
+	if logger == nil {
+		return nil, fmt.Errorf("failed to init telemetry client: provide a valid logger")
 	}
 
-	c, err := newHTTPClient(cfg.CloudCfg, cfg.Logger)
+	c, err := newHTTPClient(cfg, logger)
 	if err != nil {
 		return nil, fmt.Errorf("failed to init telemetry client: %v", err)
 	}
 
-	headers := map[string]string{
-		"X-HCP-Source-Channel": fmt.Sprintf("consul %s hcp-go-sdk/%s", version.GetHumanVersion(), version.Version),
-		"Content-Type":         "application/x-protobuf",
-	}
+	header := make(http.Header)
+	header.Set("Content-Type", "application/x-protobuf")
 
 	return &otlpClient{
-		client:  c,
-		headers: headers,
+		client: c,
+		header: &header,
 	}, nil
 }
 
@@ -127,15 +121,13 @@ func (o *otlpClient) ExportMetrics(ctx context.Context, protoMetrics *metricpb.R
 	if err != nil {
 		return fmt.Errorf("failed to export metrics: %v", err)
 	}
+	req.Header = (*o.header).Clone()
 
-	for k, v := range o.headers {
-		req.Header.Set(k, v)
-	}
-
-	resp, err := o.client.Do(req)
+	resp, err := o.client.Do(req.WithContext(ctx))
 	if err != nil {
 		return fmt.Errorf("failed to export metrics: %v", err)
 	}
+	defer resp.Body.Close()
 
 	var respData bytes.Buffer
 	if _, err := io.Copy(&respData, resp.Body); err != nil {

diff --git a/agent/hcp/client/metrics_client_test.go b/agent/hcp/client/metrics_client_test.go
@@ -4,23 +4,18 @@ import (
 	"context"
 	"crypto/tls"
 	"errors"
-	"fmt"
-	"io"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"testing"
 
-	"golang.org/x/oauth2"
-	"google.golang.org/protobuf/proto"
-
-	colpb "go.opentelemetry.io/proto/otlp/collector/metrics/v1"
-	metricpb "go.opentelemetry.io/proto/otlp/metrics/v1"
-
-	"github.com/hashicorp/consul/version"
 	"github.com/hashicorp/go-hclog"
 	hcpcfg "github.com/hashicorp/hcp-sdk-go/config"
 	"github.com/stretchr/testify/require"
+	colpb "go.opentelemetry.io/proto/otlp/collector/metrics/v1"
+	metricpb "go.opentelemetry.io/proto/otlp/metrics/v1"
+	"golang.org/x/oauth2"
+	"google.golang.org/protobuf/proto"
 )
 
 type mockHCPCfg struct{}
@@ -56,38 +51,31 @@ func (m mockErrCloudCfg) HCPConfig(opts ...hcpcfg.HCPConfigOption) (hcpcfg.HCPCo
 func TestNewMetricsClient(t *testing.T) {
 	for name, test := range map[string]struct {
 		wantErr string
-		cfg     *TelemetryClientCfg
+		cfg     cloudConfig
+		logger  hclog.Logger
 	}{
 		"success": {
-			cfg: &TelemetryClientCfg{
-				Logger:   hclog.New(&hclog.LoggerOptions{Output: io.Discard}),
-				CloudCfg: &mockCloudCfg{},
-			},
+			cfg:    &mockCloudCfg{},
+			logger: hclog.NewNullLogger(),
 		},
 		"failsWithoutCloudCfg": {
-			wantErr: "failed to init telemetry client",
-			cfg: &TelemetryClientCfg{
-				Logger:   hclog.New(&hclog.LoggerOptions{Output: io.Discard}),
-				CloudCfg: nil,
-			},
+			wantErr: "failed to init telemetry client: provide valid cloudCfg (Cloud Configuration for TLS)",
+			cfg:     nil,
+			logger:  hclog.NewNullLogger(),
 		},
 		"failsWithoutLogger": {
-			wantErr: "failed to init telemetry client",
-			cfg: &TelemetryClientCfg{
-				Logger:   nil,
-				CloudCfg: &mockErrCloudCfg{},
-			},
+			wantErr: "failed to init telemetry client: provide a valid logger",
+			cfg:     mockCloudCfg{},
+			logger:  nil,
 		},
 		"failsHCPConfig": {
 			wantErr: "failed to init telemetry client",
-			cfg: &TelemetryClientCfg{
-				Logger:   hclog.New(&hclog.LoggerOptions{Output: io.Discard}),
-				CloudCfg: &mockErrCloudCfg{},
-			},
+			cfg:     mockErrCloudCfg{},
+			logger:  hclog.NewNullLogger(),
 		},
 	} {
 		t.Run(name, func(t *testing.T) {
-			client, err := NewMetricsClient(test.cfg)
+			client, err := NewMetricsClient(test.cfg, test.logger)
 			if test.wantErr != "" {
 				require.Error(t, err)
 				require.Contains(t, err.Error(), test.wantErr)
@@ -118,7 +106,6 @@ func TestExportMetrics(t *testing.T) {
 				require.Equal(t, r.Header.Get("Content-Type"), "application/x-protobuf")
 
 				require.Equal(t, r.Header.Get("Authorization"), "Bearer test-token")
-				require.Equal(t, r.Header.Get("X-HCP-Source-Channel"), fmt.Sprintf("consul %s hcp-go-sdk/%s", version.GetHumanVersion(), version.Version))
 
 				body := colpb.ExportMetricsServiceResponse{}
 
@@ -137,12 +124,7 @@ func TestExportMetrics(t *testing.T) {
 			}))
 			defer srv.Close()
 
-			cfg := &TelemetryClientCfg{
-				Logger:   hclog.New(&hclog.LoggerOptions{Output: io.Discard}),
-				CloudCfg: mockCloudCfg{},
-			}
-
-			client, err := NewMetricsClient(cfg)
+			client, err := NewMetricsClient(mockCloudCfg{}, hclog.NewNullLogger())
 			require.NoError(t, err)
 
 			ctx := context.Background()