Skip to content

Commit

Permalink
[receiver/sapm] - follow receiver contract (#35300)
Browse files Browse the repository at this point in the history
**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:**
#5909

**Testing:** Added
  • Loading branch information
VihasMakwana authored Oct 7, 2024
1 parent bf336d4 commit 5edf2fe
Show file tree
Hide file tree
Showing 5 changed files with 96 additions and 4 deletions.
27 changes: 27 additions & 0 deletions .chloggen/sapm-receiver-contract.yaml
Original file line number Diff line number Diff line change
@@ -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: []
23 changes: 23 additions & 0 deletions internal/coreinternal/errorutil/http.go
Original file line number Diff line number Diff line change
@@ -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)
}
2 changes: 1 addition & 1 deletion receiver/sapmreceiver/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions receiver/sapmreceiver/trace_receiver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
}

Expand Down
44 changes: 43 additions & 1 deletion receiver/sapmreceiver/trace_receiver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"compress/gzip"
"context"
"encoding/binary"
"errors"
"fmt"
"net/http"
"testing"
Expand All @@ -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"
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit 5edf2fe

Please sign in to comment.