From 7b40203302e0e1e2326730a367db389080c645cf Mon Sep 17 00:00:00 2001 From: Alex Greenbank Date: Tue, 2 Apr 2024 14:51:39 +0000 Subject: [PATCH] Changes based on review comments Signed-off-by: Alex Greenbank --- storage/remote/client.go | 27 +++++++++++---------------- storage/remote/queue_manager.go | 2 +- storage/remote/queue_manager_test.go | 12 ++++++------ storage/remote/write.go | 4 ++-- 4 files changed, 20 insertions(+), 25 deletions(-) diff --git a/storage/remote/client.go b/storage/remote/client.go index d7838cb72b..7ff17a5a1f 100644 --- a/storage/remote/client.go +++ b/storage/remote/client.go @@ -88,9 +88,8 @@ func init() { // Client allows reading and writing from/to a remote HTTP endpoint. type Client struct { - remoteName string // Used to differentiate clients in metrics. - urlString string // url.String() - rwFormat config.RemoteWriteFormat // For write clients, ignored for read clients. + remoteName string // Used to differentiate clients in metrics. + urlString string // url.String() lastRWHeader string Client *http.Client timeout time.Duration @@ -173,7 +172,6 @@ func NewWriteClient(name string, conf *ClientConfig) (WriteClient, error) { httpClient.Transport = otelhttp.NewTransport(t) return &Client{ - rwFormat: conf.RemoteWriteFormat, remoteName: name, urlString: conf.URL.String(), Client: httpClient, @@ -206,17 +204,14 @@ type RecoverableError struct { } // Attempt a HEAD request against a remote write endpoint to see what it supports. -func (c *Client) GetProtoVersions(ctx context.Context) (string, error) { - // If we are in Version1 mode then don't even bother - if c.rwFormat == Version1 { - return RemoteWriteVersion1HeaderValue, nil - } +func (c *Client) probeRemoteVersions(ctx context.Context) error { + // We assume we are in Version2 mode otherwise we shouldn't be calling this httpReq, err := http.NewRequest("HEAD", c.urlString, nil) if err != nil { // Errors from NewRequest are from unparsable URLs, so are not // recoverable. - return "", err + return err } // Set the version header to be nice @@ -229,7 +224,7 @@ func (c *Client) GetProtoVersions(ctx context.Context) (string, error) { httpResp, err := c.Client.Do(httpReq.WithContext(ctx)) if err != nil { // We don't attempt a retry here - return "", err + return err } // See if we got a header anyway @@ -250,11 +245,11 @@ func (c *Client) GetProtoVersions(ctx context.Context) (string, error) { // a response that confirms it can speak 2.0 c.lastRWHeader = promHeader } - return promHeader, fmt.Errorf(httpResp.Status) + return fmt.Errorf(httpResp.Status) } - // All ok, return header and no error - return promHeader, nil + // All ok, return no error + return nil } // Store sends a batch of samples to the HTTP endpoint, the request is the proto marshalled @@ -448,8 +443,8 @@ func NewTestClient(name, url string) WriteClient { return &TestClient{name: name, url: url} } -func (c *TestClient) GetProtoVersions(_ context.Context) (string, error) { - return "2.0;snappy,0.1.0", nil +func (c *TestClient) probeRemoteVersions(_ context.Context) error { + return nil } func (c *TestClient) Store(_ context.Context, req []byte, _ int, _ config.RemoteWriteFormat, _ string) error { diff --git a/storage/remote/queue_manager.go b/storage/remote/queue_manager.go index c5b2655835..7a3234de6c 100644 --- a/storage/remote/queue_manager.go +++ b/storage/remote/queue_manager.go @@ -395,7 +395,7 @@ type WriteClient interface { // Endpoint is the remote read or write endpoint for the storage client. Endpoint() string // Get the protocol versions supported by the endpoint - GetProtoVersions(ctx context.Context) (string, error) + probeRemoteVersions(ctx context.Context) error // Get the last RW header received from the endpoint GetLastRWHeader() string } diff --git a/storage/remote/queue_manager_test.go b/storage/remote/queue_manager_test.go index b0aaed4a64..a570d433d4 100644 --- a/storage/remote/queue_manager_test.go +++ b/storage/remote/queue_manager_test.go @@ -1317,8 +1317,8 @@ func (c *TestWriteClient) Endpoint() string { return "http://test-remote.com/1234" } -func (c *TestWriteClient) GetProtoVersions(_ context.Context) (string, error) { - return "2.0;snappy,0.1.0", nil +func (c *TestWriteClient) probeRemoteVersions(_ context.Context) error { + return nil } func (c *TestWriteClient) GetLastRWHeader() string { @@ -1361,8 +1361,8 @@ func (c *TestBlockingWriteClient) Endpoint() string { return "http://test-remote-blocking.com/1234" } -func (c *TestBlockingWriteClient) GetProtoVersions(_ context.Context) (string, error) { - return "2.0;snappy,0.1.0", nil +func (c *TestBlockingWriteClient) probeRemoteVersions(_ context.Context) error { + return nil } func (c *TestBlockingWriteClient) GetLastRWHeader() string { @@ -1378,8 +1378,8 @@ func (c *NopWriteClient) Store(context.Context, []byte, int, config.RemoteWriteF } func (c *NopWriteClient) Name() string { return "nopwriteclient" } func (c *NopWriteClient) Endpoint() string { return "http://test-remote.com/1234" } -func (c *NopWriteClient) GetProtoVersions(_ context.Context) (string, error) { - return "2.0;snappy,0.1.0", nil +func (c *NopWriteClient) probeRemoteVersions(_ context.Context) error { + return nil } func (c *NopWriteClient) GetLastRWHeader() string { return "2.0;snappy,0.1.0" } diff --git a/storage/remote/write.go b/storage/remote/write.go index 0b79cfa2c2..7f087ca24e 100644 --- a/storage/remote/write.go +++ b/storage/remote/write.go @@ -203,8 +203,8 @@ func (rws *WriteStorage) ApplyConfig(conf *config.Config) error { // If this newer remote write format is enabled then we need to probe the remote server // to work out the desired protocol version and compressions // The value of the header is kept in the client so no need to see it here - _, _ = c.GetProtoVersions(context.Background()) - // TODO(alexg): Since they're never used should I remove the return values of GetProtoVersion()? + _ = c.probeRemoteVersions(context.Background()) + // We ignore any error here, at some point we may choose to log it } // Redacted to remove any passwords in the URL (that are