Skip to content

Commit

Permalink
[receiver/zipkin] follow receiver contract (open-telemetry#28627)
Browse files Browse the repository at this point in the history
I came across `zipkinreceiver` and observed we don't
follow the receiver
[contract](https://github.com/open-telemetry/opentelemetry-collector/blob/b2961b799e2c1ec128f0539764af1fa10c839e04/receiver/doc.go#L21).
We return `InternalServerError` straight away without checking
permanent/non-permanent errors.

We should probably return BadRequest in case of permanent errors

open-telemetry/opentelemetry-collector#4335

**Testing:** Added test cases

Co-authored-by: Andrzej Stencel <[email protected]>
  • Loading branch information
VihasMakwana and andrzej-stencel committed Nov 1, 2023
1 parent 5c69f33 commit 9edd6a9
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 5 deletions.
27 changes: 27 additions & 0 deletions .chloggen/update-zipkin-permanent.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: bug_fix

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: zipkinreceiver

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Return BadRequest in case of permanent errors

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [4335]

# (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: []
17 changes: 12 additions & 5 deletions receiver/zipkinreceiver/trace_receiver.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/consumer"
"go.opentelemetry.io/collector/consumer/consumererror"
"go.opentelemetry.io/collector/pdata/ptrace"
"go.opentelemetry.io/collector/receiver"
"go.opentelemetry.io/collector/receiver/receiverhelper"
Expand All @@ -32,6 +33,7 @@ const (
)

var errNextConsumerRespBody = []byte(`"Internal Server Error"`)
var errBadRequestRespBody = []byte(`"Bad Request"`)

// zipkinReceiver type is used to handle spans received in the Zipkin format.
type zipkinReceiver struct {
Expand Down Expand Up @@ -239,17 +241,22 @@ func (zr *zipkinReceiver) ServeHTTP(w http.ResponseWriter, r *http.Request) {
receiverTagValue = zipkinV1TagValue
}
obsrecv.EndTracesOp(ctx, receiverTagValue, td.SpanCount(), consumerErr)
if consumerErr == nil {
// Send back the response "Accepted" as
// required at https://zipkin.io/zipkin-api/#/default/post_spans
w.WriteHeader(http.StatusAccepted)
return
}

if consumerErr != nil {
if consumererror.IsPermanent(consumerErr) {
w.WriteHeader(http.StatusBadRequest)
_, _ = w.Write(errBadRequestRespBody)
} else {
// Transient error, due to some internal condition.
w.WriteHeader(http.StatusInternalServerError)
_, _ = w.Write(errNextConsumerRespBody)
return
}

// Finally send back the response "Accepted" as
// required at https://zipkin.io/zipkin-api/#/default/post_spans
w.WriteHeader(http.StatusAccepted)
}

func transportType(r *http.Request, asZipkinv1 bool) string {
Expand Down
23 changes: 23 additions & 0 deletions receiver/zipkinreceiver/trace_receiver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"go.opentelemetry.io/collector/component/componenttest"
"go.opentelemetry.io/collector/config/confighttp"
"go.opentelemetry.io/collector/consumer"
"go.opentelemetry.io/collector/consumer/consumererror"
"go.opentelemetry.io/collector/consumer/consumertest"
"go.opentelemetry.io/collector/pdata/ptrace"
"go.opentelemetry.io/collector/receiver/receivertest"
Expand Down Expand Up @@ -288,6 +289,28 @@ func TestReceiverConsumerError(t *testing.T) {
require.Equal(t, "\"Internal Server Error\"", req.Body.String())
}

func TestReceiverConsumerPermanentError(t *testing.T) {
body, err := os.ReadFile(zipkinV2Single)
require.NoError(t, err)

r := httptest.NewRequest("POST", "/api/v2/spans", bytes.NewBuffer(body))
r.Header.Add("content-type", "application/json")

cfg := &Config{
HTTPServerSettings: confighttp.HTTPServerSettings{
Endpoint: "localhost:9411",
},
}
zr, err := newReceiver(cfg, consumertest.NewErr(consumererror.NewPermanent(errors.New("consumer error"))), receivertest.NewNopCreateSettings())
require.NoError(t, err)

req := httptest.NewRecorder()
zr.ServeHTTP(req, r)

require.Equal(t, 400, req.Code)
require.Equal(t, "\"Bad Request\"", req.Body.String())
}

func thriftExample() []byte {
now := time.Now().Unix()
zSpans := []*zipkincore.Span{
Expand Down

0 comments on commit 9edd6a9

Please sign in to comment.