From c4ac4ebf03346bf7b90ebe16acfdd81af27c4ec6 Mon Sep 17 00:00:00 2001 From: Dennis Wielepsky Date: Tue, 4 Jun 2024 00:26:37 +0200 Subject: [PATCH 1/6] fail on unexpected response status code --- samplers/aws/xray/internal/client.go | 4 ++++ samplers/aws/xray/internal/client_test.go | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/samplers/aws/xray/internal/client.go b/samplers/aws/xray/internal/client.go index eb65391af60..31393fdf562 100644 --- a/samplers/aws/xray/internal/client.go +++ b/samplers/aws/xray/internal/client.go @@ -139,6 +139,10 @@ func (c *xrayClient) getSamplingRules(ctx context.Context) (*getSamplingRulesOut } defer output.Body.Close() + if output.StatusCode != http.StatusOK { + return nil, fmt.Errorf("xray client: unable to retrieve sampling settings, expected response status code 200, got: %d", output.StatusCode) + } + var samplingRulesOutput *getSamplingRulesOutput if err := json.NewDecoder(output.Body).Decode(&samplingRulesOutput); err != nil { return nil, fmt.Errorf("xray client: unable to unmarshal the response body: %w", err) diff --git a/samplers/aws/xray/internal/client_test.go b/samplers/aws/xray/internal/client_test.go index a6b712cda89..cf486c364b7 100644 --- a/samplers/aws/xray/internal/client_test.go +++ b/samplers/aws/xray/internal/client_test.go @@ -261,3 +261,22 @@ func TestEndpointIsNotReachable(t *testing.T) { _, err = client.getSamplingRules(context.Background()) assert.Error(t, err) } + +func TestRespondsWithErrorStatusCode(t *testing.T) { + testServer := httptest.NewServer(http.HandlerFunc(func(res http.ResponseWriter, _ *http.Request) { + res.WriteHeader(http.StatusForbidden) + _, err := res.Write([]byte(`{}`)) + require.NoError(t, err) + })) + t.Cleanup(testServer.Close) + + u, err := url.Parse(testServer.URL) + require.NoError(t, err) + + client, err := newClient(*u) + require.NoError(t, err) + + samplingRules, err := client.getSamplingRules(context.Background()) + require.Error(t, err) + require.Nil(t, samplingRules) +} From 8b93a942b598b7ca56aebefb44605394ab47ade9 Mon Sep 17 00:00:00 2001 From: Dennis Wielepsky Date: Tue, 4 Jun 2024 10:31:38 +0200 Subject: [PATCH 2/6] fix typo --- samplers/aws/xray/internal/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/samplers/aws/xray/internal/client.go b/samplers/aws/xray/internal/client.go index 31393fdf562..3a13f0f9faa 100644 --- a/samplers/aws/xray/internal/client.go +++ b/samplers/aws/xray/internal/client.go @@ -140,7 +140,7 @@ func (c *xrayClient) getSamplingRules(ctx context.Context) (*getSamplingRulesOut defer output.Body.Close() if output.StatusCode != http.StatusOK { - return nil, fmt.Errorf("xray client: unable to retrieve sampling settings, expected response status code 200, got: %d", output.StatusCode) + return nil, fmt.Errorf("xray client: unable to retrieve sampling settings, expected response status code 200, got: %d", output.StatusCode) } var samplingRulesOutput *getSamplingRulesOutput From 54c88836e3037bd9d79a5501d74734d18ce46ed5 Mon Sep 17 00:00:00 2001 From: Dennis Wielepsky Date: Tue, 4 Jun 2024 10:36:36 +0200 Subject: [PATCH 3/6] add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a97a8eb1a8..55312e96f17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The superfluous `response.WriteHeader` call in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` when the response writer is flushed. (#5634) - Custom attributes targeting metrics recorded by the `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` are not ignored anymore. (#5129) +- Erroneous response code of `getSamplingRules` in `go.opentelemetry.io/contrib/samplers/aws/xray/internal/client.go` will result in an error despite an empty sampling manifest. (#) ### Deprecated From dde95ba1a29e3963c2f5317fed7d49a047dbdf05 Mon Sep 17 00:00:00 2001 From: Dennis Wielepsky Date: Tue, 4 Jun 2024 11:15:50 +0200 Subject: [PATCH 4/6] Update CHANGELOG Co-authored-by: Damien Mathieu <42@dmathieu.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55312e96f17..2314d16b915 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The superfluous `response.WriteHeader` call in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` when the response writer is flushed. (#5634) - Custom attributes targeting metrics recorded by the `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` are not ignored anymore. (#5129) -- Erroneous response code of `getSamplingRules` in `go.opentelemetry.io/contrib/samplers/aws/xray/internal/client.go` will result in an error despite an empty sampling manifest. (#) +- Non-200 HTTP status codes when retrieving sampling rules in `go.opentelemetry.io/contrib/samplers/aws/xray` now return an error. (#5718) ### Deprecated From 82112a5e540beeb58185912a03d3e63874886113 Mon Sep 17 00:00:00 2001 From: Dennis Wielepsky Date: Thu, 27 Jun 2024 11:10:39 +0200 Subject: [PATCH 5/6] rework error messages including tests --- samplers/aws/xray/internal/client.go | 16 +++-- samplers/aws/xray/internal/client_test.go | 86 +++++++++++++++-------- 2 files changed, 67 insertions(+), 35 deletions(-) diff --git a/samplers/aws/xray/internal/client.go b/samplers/aws/xray/internal/client.go index 3a13f0f9faa..faee4498f4e 100644 --- a/samplers/aws/xray/internal/client.go +++ b/samplers/aws/xray/internal/client.go @@ -130,22 +130,22 @@ func (c *xrayClient) getSamplingRules(ctx context.Context) (*getSamplingRulesOut req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.samplingRulesURL, body) if err != nil { - return nil, fmt.Errorf("xray client: failed to create http request: %w", err) + return nil, fmt.Errorf("unable to retrieve sampling rules, error on http request: %w", err) } output, err := c.httpClient.Do(req) if err != nil { - return nil, fmt.Errorf("xray client: unable to retrieve sampling settings: %w", err) + return nil, fmt.Errorf("xray client: unable to retrieve sampling rules, error on http request: %w", err) } defer output.Body.Close() if output.StatusCode != http.StatusOK { - return nil, fmt.Errorf("xray client: unable to retrieve sampling settings, expected response status code 200, got: %d", output.StatusCode) + return nil, fmt.Errorf("xray client: unable to retrieve sampling rules, expected response status code 200, got: %d", output.StatusCode) } var samplingRulesOutput *getSamplingRulesOutput if err := json.NewDecoder(output.Body).Decode(&samplingRulesOutput); err != nil { - return nil, fmt.Errorf("xray client: unable to unmarshal the response body: %w", err) + return nil, fmt.Errorf("xray client: unable to retrieve sampling rules, unable to unmarshal the response body: %w", err) } return samplingRulesOutput, nil @@ -170,13 +170,17 @@ func (c *xrayClient) getSamplingTargets(ctx context.Context, s []*samplingStatis output, err := c.httpClient.Do(req) if err != nil { - return nil, fmt.Errorf("xray client: unable to retrieve sampling settings: %w", err) + return nil, fmt.Errorf("xray client: unable to retrieve sampling targets, error on http request: %w", err) } defer output.Body.Close() + if output.StatusCode != http.StatusOK { + return nil, fmt.Errorf("xray client: unable to retrieve sampling targets, expected response status code 200, got: %d", output.StatusCode) + } + var samplingTargetsOutput *getSamplingTargetsOutput if err := json.NewDecoder(output.Body).Decode(&samplingTargetsOutput); err != nil { - return nil, fmt.Errorf("xray client: unable to unmarshal the response body: %w", err) + return nil, fmt.Errorf("xray client: unable to retrieve sampling targets, unable to unmarshal the response body: %w", err) } return samplingTargetsOutput, nil diff --git a/samplers/aws/xray/internal/client_test.go b/samplers/aws/xray/internal/client_test.go index cf486c364b7..bcb433e0e69 100644 --- a/samplers/aws/xray/internal/client_test.go +++ b/samplers/aws/xray/internal/client_test.go @@ -5,6 +5,7 @@ package internal import ( "context" + "fmt" "net/http" "net/http/httptest" "net/url" @@ -15,7 +16,12 @@ import ( ) func createTestClient(t *testing.T, body []byte) *xrayClient { + return createTestClientWithStatusCode(t, http.StatusOK, body) +} + +func createTestClientWithStatusCode(t *testing.T, status int, body []byte) *xrayClient { testServer := httptest.NewServer(http.HandlerFunc(func(res http.ResponseWriter, _ *http.Request) { + res.WriteHeader(status) _, err := res.Write(body) require.NoError(t, err) })) @@ -26,7 +32,6 @@ func createTestClient(t *testing.T, body []byte) *xrayClient { client, err := newClient(*u) require.NoError(t, err) - return client } @@ -222,22 +227,11 @@ func TestGetSamplingTargetsMissingValues(t *testing.T) { client := createTestClient(t, body) - samplingTragets, err := client.getSamplingTargets(ctx, nil) + samplingTargets, err := client.getSamplingTargets(ctx, nil) require.NoError(t, err) - assert.Nil(t, samplingTragets.SamplingTargetDocuments[0].Interval) - assert.Nil(t, samplingTragets.SamplingTargetDocuments[0].ReservoirQuota) -} - -func TestNilContext(t *testing.T) { - client := createTestClient(t, []byte(``)) - samplingRulesOutput, err := client.getSamplingRules(context.TODO()) - require.Error(t, err) - require.Nil(t, samplingRulesOutput) - - samplingTargetsOutput, err := client.getSamplingTargets(context.TODO(), nil) - require.Error(t, err) - require.Nil(t, samplingTargetsOutput) + assert.Nil(t, samplingTargets.SamplingTargetDocuments[0].Interval) + assert.Nil(t, samplingTargets.SamplingTargetDocuments[0].ReservoirQuota) } func TestNewClient(t *testing.T) { @@ -258,25 +252,59 @@ func TestEndpointIsNotReachable(t *testing.T) { client, err := newClient(*endpoint) require.NoError(t, err) - _, err = client.getSamplingRules(context.Background()) + actualRules, err := client.getSamplingRules(context.Background()) assert.Error(t, err) + assert.ErrorContains(t, err, "xray client: unable to retrieve sampling rules, error on http request: ") + assert.Nil(t, actualRules) + + actualTargets, err := client.getSamplingTargets(context.Background(), nil) + assert.Error(t, err) + assert.ErrorContains(t, err, "xray client: unable to retrieve sampling targets, error on http request: ") + assert.Nil(t, actualTargets) } func TestRespondsWithErrorStatusCode(t *testing.T) { - testServer := httptest.NewServer(http.HandlerFunc(func(res http.ResponseWriter, _ *http.Request) { - res.WriteHeader(http.StatusForbidden) - _, err := res.Write([]byte(`{}`)) - require.NoError(t, err) - })) - t.Cleanup(testServer.Close) + client := createTestClientWithStatusCode(t, http.StatusForbidden, []byte("{}")) - u, err := url.Parse(testServer.URL) - require.NoError(t, err) + actualRules, err := client.getSamplingRules(context.Background()) + assert.Error(t, err) + assert.EqualError(t, err, fmt.Sprintf("xray client: unable to retrieve sampling rules, expected response status code 200, got: %d", http.StatusForbidden)) + assert.Nil(t, actualRules) - client, err := newClient(*u) - require.NoError(t, err) + actualTargets, err := client.getSamplingTargets(context.Background(), nil) + assert.Error(t, err) + assert.EqualError(t, err, fmt.Sprintf("xray client: unable to retrieve sampling targets, expected response status code 200, got: %d", http.StatusForbidden)) + assert.Nil(t, actualTargets) +} - samplingRules, err := client.getSamplingRules(context.Background()) - require.Error(t, err) - require.Nil(t, samplingRules) +func TestInvalidResponseBody(t *testing.T) { + type scenarios struct { + name string + response string + } + for _, scenario := range []scenarios{ + { + name: "empty response", + response: "", + }, + { + name: "malformed json", + response: "", + }, + } { + t.Run(scenario.name, func(t *testing.T) { + client := createTestClient(t, []byte(scenario.response)) + + actualRules, err := client.getSamplingRules(context.TODO()) + + assert.Error(t, err) + assert.Nil(t, actualRules) + assert.ErrorContains(t, err, "xray client: unable to retrieve sampling rules, unable to unmarshal the response body:"+scenario.response) + + actualTargets, err := client.getSamplingTargets(context.TODO(), nil) + assert.Error(t, err) + assert.Nil(t, actualTargets) + assert.ErrorContains(t, err, "xray client: unable to retrieve sampling targets, unable to unmarshal the response body: "+scenario.response) + }) + } } From 1c24da84ee5daa2aef9593da4129ac8a0ad244f7 Mon Sep 17 00:00:00 2001 From: Dennis Wielepsky Date: Wed, 25 Sep 2024 15:12:03 +0200 Subject: [PATCH 6/6] update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 805222ab84a..7d9be18acb7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed - Possible nil dereference panic in `go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace`. (#5965) +- Non-200 HTTP status codes when retrieving sampling rules in `go.opentelemetry.io/contrib/samplers/aws/xray` now return an error. (#5718) ### Removed @@ -102,7 +103,6 @@ The next release will require at least [Go 1.22]. - The superfluous `response.WriteHeader` call in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` when the response writer is flushed. (#5634) - Use `c.FullPath()` method to set `http.route` attribute in `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`. (#5734) - Out-of-bounds panic in case of invalid span ID in `go.opentelemetry.io/contrib/propagators/b3`. (#5754) -- Non-200 HTTP status codes when retrieving sampling rules in `go.opentelemetry.io/contrib/samplers/aws/xray` now return an error. (#5718) ### Deprecated