From 5edf2fe1de599fbb87256538306ff5aad0085edf Mon Sep 17 00:00:00 2001 From: VihasMakwana <121151420+VihasMakwana@users.noreply.github.com> Date: Mon, 7 Oct 2024 16:35:45 +0530 Subject: [PATCH] [receiver/sapm] - follow receiver contract (#35300) **Description:** This is my first PR to ensure that all network receivers adhere to [the contract](https://github.com/open-telemetry/opentelemetry-collector/blob/df3c9e38a80ccc3b14705462be2e2e51c628a3b3/receiver/doc.go#L10) and maintain the correct order of operations. I also plan to submit additional PRs for other receivers in the future. Follow receiver contract for `sapmreceiver`. This also includes an internal `errorutil` package which will be used by other network receivers as well. **Link to tracking Issue:** https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/5909 **Testing:** Added --- .chloggen/sapm-receiver-contract.yaml | 27 ++++++++++++ internal/coreinternal/errorutil/http.go | 23 ++++++++++ receiver/sapmreceiver/go.mod | 2 +- receiver/sapmreceiver/trace_receiver.go | 4 +- receiver/sapmreceiver/trace_receiver_test.go | 44 +++++++++++++++++++- 5 files changed, 96 insertions(+), 4 deletions(-) create mode 100644 .chloggen/sapm-receiver-contract.yaml create mode 100644 internal/coreinternal/errorutil/http.go diff --git a/.chloggen/sapm-receiver-contract.yaml b/.chloggen/sapm-receiver-contract.yaml new file mode 100644 index 000000000000..3989acdd0504 --- /dev/null +++ b/.chloggen/sapm-receiver-contract.yaml @@ -0,0 +1,27 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: sapmreceiver + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Respond 503 on non-permanent and 400 on permanent errors + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [35300] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# If your change doesn't affect end users or the exported elements of any package, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [] diff --git a/internal/coreinternal/errorutil/http.go b/internal/coreinternal/errorutil/http.go new file mode 100644 index 000000000000..0a78f602f7ed --- /dev/null +++ b/internal/coreinternal/errorutil/http.go @@ -0,0 +1,23 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package errorutil // import "github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal/errorutil" + +import ( + "net/http" + + "go.opentelemetry.io/collector/consumer/consumererror" +) + +func HTTPError(w http.ResponseWriter, err error) { + if err == nil { + return + } + // non-retryable status + status := http.StatusBadRequest + if !consumererror.IsPermanent(err) { + // retryable status + status = http.StatusServiceUnavailable + } + http.Error(w, err.Error(), status) +} diff --git a/receiver/sapmreceiver/go.mod b/receiver/sapmreceiver/go.mod index 87b7ae455568..04724eee4798 100644 --- a/receiver/sapmreceiver/go.mod +++ b/receiver/sapmreceiver/go.mod @@ -7,6 +7,7 @@ require ( github.com/jaegertracing/jaeger v1.61.0 github.com/klauspost/compress v1.17.10 github.com/open-telemetry/opentelemetry-collector-contrib/internal/common v0.111.0 + github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal v0.111.0 github.com/open-telemetry/opentelemetry-collector-contrib/internal/splunk v0.111.0 github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/jaeger v0.111.0 github.com/signalfx/sapm-proto v0.14.0 @@ -47,7 +48,6 @@ require ( github.com/mitchellh/reflectwalk v1.0.2 // indirect github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.2 // indirect - github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal v0.111.0 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/rs/cors v1.11.1 // indirect go.opentelemetry.io/collector/client v1.17.0 // indirect diff --git a/receiver/sapmreceiver/trace_receiver.go b/receiver/sapmreceiver/trace_receiver.go index 59170b58d237..10d6d9949633 100644 --- a/receiver/sapmreceiver/trace_receiver.go +++ b/receiver/sapmreceiver/trace_receiver.go @@ -22,6 +22,7 @@ import ( "go.opentelemetry.io/collector/receiver" "go.opentelemetry.io/collector/receiver/receiverhelper" + "github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal/errorutil" "github.com/open-telemetry/opentelemetry-collector-contrib/internal/splunk" "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/jaeger" ) @@ -91,8 +92,7 @@ func (sr *sapmReceiver) HTTPHandlerFunc(rw http.ResponseWriter, req *http.Reques // handle the request payload err := sr.handleRequest(req) if err != nil { - // TODO account for this error (throttled logging or metrics) - rw.WriteHeader(http.StatusBadRequest) + errorutil.HTTPError(rw, err) return } diff --git a/receiver/sapmreceiver/trace_receiver_test.go b/receiver/sapmreceiver/trace_receiver_test.go index c3482034d5c5..d2cf1ab12732 100644 --- a/receiver/sapmreceiver/trace_receiver_test.go +++ b/receiver/sapmreceiver/trace_receiver_test.go @@ -8,6 +8,7 @@ import ( "compress/gzip" "context" "encoding/binary" + "errors" "fmt" "net/http" "testing" @@ -23,6 +24,8 @@ import ( "go.opentelemetry.io/collector/component/componentstatus" "go.opentelemetry.io/collector/config/confighttp" "go.opentelemetry.io/collector/config/configtls" + "go.opentelemetry.io/collector/consumer" + "go.opentelemetry.io/collector/consumer/consumererror" "go.opentelemetry.io/collector/consumer/consumertest" "go.opentelemetry.io/collector/pdata/pcommon" "go.opentelemetry.io/collector/pdata/ptrace" @@ -243,7 +246,7 @@ func compressZstd(reqBytes []byte) ([]byte, error) { return buff.Bytes(), nil } -func setupReceiver(t *testing.T, config *Config, sink *consumertest.TracesSink) receiver.Traces { +func setupReceiver(t *testing.T, config *Config, sink consumer.Traces) receiver.Traces { params := receivertest.NewNopSettings() sr, err := newReceiver(params, config, sink) assert.NoError(t, err, "should not have failed to create the SAPM receiver") @@ -432,6 +435,45 @@ func TestAccessTokenPassthrough(t *testing.T) { } } +func TestStatusCode(t *testing.T) { + tlsAddress := testutil.GetAvailableLocalAddress(t) + + tests := []struct { + name string + err error + expectedStatus int + }{ + { + name: "non-permanent error", + err: errors.New("non-permanenet error"), + expectedStatus: http.StatusServiceUnavailable, + }, + { + name: "permanent error", + err: consumererror.NewPermanent(errors.New("non-permanenet error")), + expectedStatus: http.StatusBadRequest, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + config := &Config{ + ServerConfig: confighttp.ServerConfig{ + Endpoint: tlsAddress, + }, + } + sr := setupReceiver(t, config, consumertest.NewErr(test.err)) + sapm := &splunksapm.PostSpansRequest{ + Batches: []*model.Batch{grpcFixture(time.Now().UTC())}, + } + var resp *http.Response + resp, err := sendSapm(config.Endpoint, sapm, "", false, "") + require.NoErrorf(t, err, "should not have failed when sending sapm %v", err) + assert.Equal(t, test.expectedStatus, resp.StatusCode) + require.NoError(t, sr.Shutdown(context.Background())) + }) + } +} + var _ componentstatus.Reporter = (*nopHost)(nil) type nopHost struct {