Skip to content

Commit a11de37

Browse files
VihasMakwanaAkhigbeEromo
authored andcommitted
[receiver/sapm] - follow receiver contract (open-telemetry#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:** open-telemetry#5909 **Testing:** Added
1 parent 48a25e0 commit a11de37

File tree

5 files changed

+96
-4
lines changed

5 files changed

+96
-4
lines changed

.chloggen/sapm-receiver-contract.yaml

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
# Use this changelog template to create an entry for release notes.
2+
3+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
4+
change_type: enhancement
5+
6+
# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
7+
component: sapmreceiver
8+
9+
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
10+
note: Respond 503 on non-permanent and 400 on permanent errors
11+
12+
# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
13+
issues: [35300]
14+
15+
# (Optional) One or more lines of additional information to render under the primary note.
16+
# These lines will be padded with 2 spaces and then inserted directly into the document.
17+
# Use pipe (|) for multiline entries.
18+
subtext:
19+
20+
# If your change doesn't affect end users or the exported elements of any package,
21+
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
22+
# Optional: The change log or logs in which this entry should be included.
23+
# e.g. '[user]' or '[user, api]'
24+
# Include 'user' if the change is relevant to end users.
25+
# Include 'api' if there is a change to a library API.
26+
# Default: '[user]'
27+
change_logs: []
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// Copyright The OpenTelemetry Authors
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package errorutil // import "github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal/errorutil"
5+
6+
import (
7+
"net/http"
8+
9+
"go.opentelemetry.io/collector/consumer/consumererror"
10+
)
11+
12+
func HTTPError(w http.ResponseWriter, err error) {
13+
if err == nil {
14+
return
15+
}
16+
// non-retryable status
17+
status := http.StatusBadRequest
18+
if !consumererror.IsPermanent(err) {
19+
// retryable status
20+
status = http.StatusServiceUnavailable
21+
}
22+
http.Error(w, err.Error(), status)
23+
}

receiver/sapmreceiver/go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ require (
77
github.com/jaegertracing/jaeger v1.61.0
88
github.com/klauspost/compress v1.17.10
99
github.com/open-telemetry/opentelemetry-collector-contrib/internal/common v0.111.0
10+
github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal v0.111.0
1011
github.com/open-telemetry/opentelemetry-collector-contrib/internal/splunk v0.111.0
1112
github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/jaeger v0.111.0
1213
github.com/signalfx/sapm-proto v0.14.0
@@ -47,7 +48,6 @@ require (
4748
github.com/mitchellh/reflectwalk v1.0.2 // indirect
4849
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
4950
github.com/modern-go/reflect2 v1.0.2 // indirect
50-
github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal v0.111.0 // indirect
5151
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
5252
github.com/rs/cors v1.11.1 // indirect
5353
go.opentelemetry.io/collector/client v1.17.0 // indirect

receiver/sapmreceiver/trace_receiver.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"go.opentelemetry.io/collector/receiver"
2323
"go.opentelemetry.io/collector/receiver/receiverhelper"
2424

25+
"github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal/errorutil"
2526
"github.com/open-telemetry/opentelemetry-collector-contrib/internal/splunk"
2627
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/jaeger"
2728
)
@@ -91,8 +92,7 @@ func (sr *sapmReceiver) HTTPHandlerFunc(rw http.ResponseWriter, req *http.Reques
9192
// handle the request payload
9293
err := sr.handleRequest(req)
9394
if err != nil {
94-
// TODO account for this error (throttled logging or metrics)
95-
rw.WriteHeader(http.StatusBadRequest)
95+
errorutil.HTTPError(rw, err)
9696
return
9797
}
9898

receiver/sapmreceiver/trace_receiver_test.go

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"compress/gzip"
99
"context"
1010
"encoding/binary"
11+
"errors"
1112
"fmt"
1213
"net/http"
1314
"testing"
@@ -23,6 +24,8 @@ import (
2324
"go.opentelemetry.io/collector/component/componentstatus"
2425
"go.opentelemetry.io/collector/config/confighttp"
2526
"go.opentelemetry.io/collector/config/configtls"
27+
"go.opentelemetry.io/collector/consumer"
28+
"go.opentelemetry.io/collector/consumer/consumererror"
2629
"go.opentelemetry.io/collector/consumer/consumertest"
2730
"go.opentelemetry.io/collector/pdata/pcommon"
2831
"go.opentelemetry.io/collector/pdata/ptrace"
@@ -243,7 +246,7 @@ func compressZstd(reqBytes []byte) ([]byte, error) {
243246
return buff.Bytes(), nil
244247
}
245248

246-
func setupReceiver(t *testing.T, config *Config, sink *consumertest.TracesSink) receiver.Traces {
249+
func setupReceiver(t *testing.T, config *Config, sink consumer.Traces) receiver.Traces {
247250
params := receivertest.NewNopSettings()
248251
sr, err := newReceiver(params, config, sink)
249252
assert.NoError(t, err, "should not have failed to create the SAPM receiver")
@@ -432,6 +435,45 @@ func TestAccessTokenPassthrough(t *testing.T) {
432435
}
433436
}
434437

438+
func TestStatusCode(t *testing.T) {
439+
tlsAddress := testutil.GetAvailableLocalAddress(t)
440+
441+
tests := []struct {
442+
name string
443+
err error
444+
expectedStatus int
445+
}{
446+
{
447+
name: "non-permanent error",
448+
err: errors.New("non-permanenet error"),
449+
expectedStatus: http.StatusServiceUnavailable,
450+
},
451+
{
452+
name: "permanent error",
453+
err: consumererror.NewPermanent(errors.New("non-permanenet error")),
454+
expectedStatus: http.StatusBadRequest,
455+
},
456+
}
457+
for _, test := range tests {
458+
t.Run(test.name, func(t *testing.T) {
459+
config := &Config{
460+
ServerConfig: confighttp.ServerConfig{
461+
Endpoint: tlsAddress,
462+
},
463+
}
464+
sr := setupReceiver(t, config, consumertest.NewErr(test.err))
465+
sapm := &splunksapm.PostSpansRequest{
466+
Batches: []*model.Batch{grpcFixture(time.Now().UTC())},
467+
}
468+
var resp *http.Response
469+
resp, err := sendSapm(config.Endpoint, sapm, "", false, "")
470+
require.NoErrorf(t, err, "should not have failed when sending sapm %v", err)
471+
assert.Equal(t, test.expectedStatus, resp.StatusCode)
472+
require.NoError(t, sr.Shutdown(context.Background()))
473+
})
474+
}
475+
}
476+
435477
var _ componentstatus.Reporter = (*nopHost)(nil)
436478

437479
type nopHost struct {

0 commit comments

Comments
 (0)