Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

return a clear error when http/protobuf server returns json #263

Merged
merged 5 commits into from
Sep 20, 2023
Merged

return a clear error when http/protobuf server returns json #263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f47522d
go mod tidy
Sep 12, 2023
6c51bd9
check for servers out of spec
Sep 13, 2023
26302d4
make code a bit more panic-safe and report missing content-type header
Sep 13, 2023
353c1d2
add tests, make code gooderer
Sep 13, 2023
e168d3a
remove debug log
Sep 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 1 addition & 11 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,25 +79,17 @@ github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+
github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
github.com/stretchr/testify v1.8.3 h1:RP3t2pwF7cMEbC1dqtB6poj3niw/9gnV4Cjg5oW5gtY=
github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
github.com/xo/terminfo v0.0.0-20210125001918-ca9a967f8778/go.mod h1:2MuV+tbUrU1zIOPMxZ5EncGwgmMJsa+9ucAQZXxsObs=
github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e h1:JVG44RsyaB9T2KIHavMF/ppJZNG9ZpyihvCd0w101no=
github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e/go.mod h1:RbqR21r5mrJuqunuUZ/Dhy/avygyECGrLceyNeo4LiM=
github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY=
go.opentelemetry.io/otel v1.16.0 h1:Z7GVAX/UkAXPKsy94IU+i6thsQS4nb7LviLpnaNeW8s=
go.opentelemetry.io/otel v1.16.0/go.mod h1:vl0h9NUa1D5s1nv3A5vZOYWn8av4K8Ml6JDeHrT/bx4=
go.opentelemetry.io/otel v1.18.0 h1:TgVozPGZ01nHyDZxK5WGPFB9QexeTMXEH7+tIClWfzs=
go.opentelemetry.io/otel v1.18.0/go.mod h1:9lWqYO0Db579XzVuCKFNPDl4s73Voa+zEck3wHaAYQI=
go.opentelemetry.io/otel/metric v1.16.0 h1:RbrpwVG1Hfv85LgnZ7+txXioPDoh6EdbZHo26Q3hqOo=
go.opentelemetry.io/otel/metric v1.16.0/go.mod h1:QE47cpOmkwipPiefDwo2wDzwJrlfxxNYodqc4xnGCo4=
go.opentelemetry.io/otel/metric v1.18.0 h1:JwVzw94UYmbx3ej++CwLUQZxEODDj/pOuTCvzhtRrSQ=
go.opentelemetry.io/otel/metric v1.18.0/go.mod h1:nNSpsVDjWGfb7chbRLUNW+PBNdcSTHD4Uu5pfFMOI0k=
go.opentelemetry.io/otel/sdk v1.16.0 h1:Z1Ok1YsijYL0CSJpHt4cS3wDDh7p572grzNrBMiMWgE=
go.opentelemetry.io/otel/sdk v1.16.0/go.mod h1:tMsIuKXuuIWPBAOrH+eHtvhTL+SntFtXF9QD68aP6p4=
go.opentelemetry.io/otel/sdk v1.18.0 h1:e3bAB0wB3MljH38sHzpV/qWrOTCFrdZF2ct9F8rBkcY=
go.opentelemetry.io/otel/sdk v1.18.0/go.mod h1:1RCygWV7plY2KmdskZEDDBs4tJeHG92MdHZIluiYs/M=
go.opentelemetry.io/otel/trace v1.16.0 h1:8JRpaObFoW0pxuVPapkgH8UhHQj+bJW8jJsCZEu5MQs=
go.opentelemetry.io/otel/trace v1.16.0/go.mod h1:Yt9vYq1SdNz3xdjZZK7wcXv1qv2pwLkqr2QVwea0ef0=
go.opentelemetry.io/otel/trace v1.18.0 h1:NY+czwbHbmndxojTEKiSMHkG2ClNH2PwmcHrdo0JY10=
go.opentelemetry.io/otel/trace v1.18.0/go.mod h1:T2+SGJGuYZY3bjj5rgh/hN7KIrlpWC5nS8Mjvzckz+0=
go.opentelemetry.io/proto/otlp v1.0.0 h1:T0TX0tmXU8a3CbNXzEKGeU5mIVOdf0oykP+u2lIVU/I=
Expand Down Expand Up @@ -126,8 +118,6 @@ golang.org/x/sys v0.0.0-20220319134239-a9b59b0215f8/go.mod h1:oPkhp1MJrh7nUepCBc
golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.8.0 h1:EBmGv8NaZBZTWvrbjNoL6HVt+IVy3QDQpJs7VRIw3tU=
golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.12.0 h1:CM0HF96J0hcLAwsHPJZjfdNzs0gftsLfgKt57wWHJ0o=
golang.org/x/sys v0.12.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
Expand Down
8 changes: 8 additions & 0 deletions otlpclient/otlp_client_http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,14 @@ func (hc *HttpClient) UploadTraces(ctx context.Context, rsps []*tracepb.Resource
// processHTTPStatus takes the http.Response and body, returning the same bool, error
// as retryFunc. Mostly it's broken out so it can be unit tested.
func processHTTPStatus(ctx context.Context, resp *http.Response, body []byte) (context.Context, bool, time.Duration, error) {
// #262 a vendor OTLP server is out of spec and returns JSON instead of protobuf
ctype := resp.Header.Get("Content-Type")
if ctype == "" {
return ctx, false, 0, fmt.Errorf("server is out of specification: Content-Type header is missing or mangled")
} else if ctype != "application/x-protobuf" {
return ctx, false, 0, fmt.Errorf("server is out of specification: expected content type application/x-protobuf but got %q", ctype)
}

if resp.StatusCode >= 200 && resp.StatusCode < 300 {
// success & partial success
// spec says server MUST send 200 OK, we'll be generous and accept any 200
Expand Down
33 changes: 32 additions & 1 deletion otlpclient/otlp_client_http_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ import (
)

func TestProcessHTTPStatus(t *testing.T) {
headers := http.Header{
"Content-Type": []string{"application/x-protobuf"},
}

for _, tc := range []struct {
resp *http.Response
Expand All @@ -25,6 +28,7 @@ func TestProcessHTTPStatus(t *testing.T) {
{
resp: &http.Response{
StatusCode: 200,
Header: headers,
},
body: etsrSuccessBody(),
keepgoing: false,
Expand All @@ -34,6 +38,7 @@ func TestProcessHTTPStatus(t *testing.T) {
{
resp: &http.Response{
StatusCode: 200,
Header: headers,
},
body: etsrPartialSuccessBody(),
keepgoing: false,
Expand All @@ -43,6 +48,7 @@ func TestProcessHTTPStatus(t *testing.T) {
{
resp: &http.Response{
StatusCode: 500,
Header: headers,
},
body: errorBody(500, "xyz"),
keepgoing: false,
Expand All @@ -52,6 +58,7 @@ func TestProcessHTTPStatus(t *testing.T) {
{
resp: &http.Response{
StatusCode: 429,
Header: headers,
},
body: errorBody(429, "xyz"),
keepgoing: true,
Expand All @@ -60,6 +67,7 @@ func TestProcessHTTPStatus(t *testing.T) {
{
resp: &http.Response{
StatusCode: 502,
Header: headers,
},
body: errorBody(502, "xyz"),
keepgoing: true,
Expand All @@ -68,6 +76,7 @@ func TestProcessHTTPStatus(t *testing.T) {
{
resp: &http.Response{
StatusCode: 503,
Header: headers,
},
body: errorBody(503, "xyz"),
keepgoing: true,
Expand All @@ -76,6 +85,7 @@ func TestProcessHTTPStatus(t *testing.T) {
{
resp: &http.Response{
StatusCode: 504,
Header: headers,
},
body: errorBody(504, "xyz"),
keepgoing: true,
Expand All @@ -85,18 +95,39 @@ func TestProcessHTTPStatus(t *testing.T) {
{
resp: &http.Response{
StatusCode: 301,
Header: headers,
},
body: errorBody(301, "xyz"),
keepgoing: false,
err: fmt.Errorf("server returned unsupported code 301"),
},
// shouldn't happen in the real world...
{
resp: &http.Response{},
resp: &http.Response{Header: headers},
body: []byte(""),
keepgoing: false,
err: fmt.Errorf("BUG: fell through error checking with status code 0"),
},
// return a decent error for out-of-spec servers that return JSON after a protobuf payload
{
resp: &http.Response{
StatusCode: 200,
Header: http.Header{"Content-Type": []string{"application/json"}},
},
body: []byte(`{"some": "json"}`),
keepgoing: false,
err: fmt.Errorf(`server is out of specification: expected content type application/x-protobuf but got "application/json"`),
},
// spec requires headers so report that as a server problem too
{
resp: &http.Response{
StatusCode: 200,
// no headers!
},
body: []byte(""),
keepgoing: false,
err: fmt.Errorf("server is out of specification: Content-Type header is missing or mangled"),
},
} {
ctx := context.Background()
_, kg, _, err := processHTTPStatus(ctx, tc.resp, tc.body)
Expand Down