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.

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

Handle 2xx as success for OTLP HTTP trace and metric exporters #4365

Merged
merged 11 commits into from
Aug 7, 2023
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- OTLP Metrics Exporter now supports the `OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE` environment variable. (#4287)
- Add `WithoutCounterSuffixes` option in `go.opentelemetry.io/otel/exporters/prometheus` to disable addition of `_total` suffixes. (#4306)
- Add info and debug logging to the metric SDK. (#4315)
- Accept 201 to 299 HTTP status as success in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp` and `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp`. (#4365)

### Changed

Expand Down
28 changes: 28 additions & 0 deletions exporters/otlp/otlpmetric/internal/otest/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,34 @@ func RunClientTests(f ClientFactory) func(*testing.T) {
want := fmt.Sprintf("%s (%d metric data points rejected)", msg, n)
assert.ErrorContains(t, errs[0], want)
})

t.Run("Other HTTP success", func(t *testing.T) {
for code := 201; code <= 299; code++ {
t.Run(fmt.Sprintf("status_%d", code), func(t *testing.T) {
rCh := make(chan ExportResult, 1)
rCh <- ExportResult{
ResponseStatus: code,
}

ctx := context.Background()
client, _ := f(rCh)
defer func() {
assert.NoError(t, client.Shutdown(ctx))
}()

defer func(orig otel.ErrorHandler) {
otel.SetErrorHandler(orig)
}(otel.GetErrorHandler())

errs := []error{}
eh := otel.ErrorHandlerFunc(func(e error) { errs = append(errs, e) })
otel.SetErrorHandler(eh)

assert.NoError(t, client.UploadMetrics(ctx, nil))
assert.Equal(t, 0, len(errs))
})
}
})
}
}

Expand Down
11 changes: 8 additions & 3 deletions exporters/otlp/otlpmetric/internal/otest/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,9 @@
}

type ExportResult struct {
Response *collpb.ExportMetricsServiceResponse
Err error
Response *collpb.ExportMetricsServiceResponse
ResponseStatus int
Err error
}

// Storage stores uploaded OTLP metric data in their proto form.
Expand Down Expand Up @@ -376,7 +377,11 @@
}

w.Header().Set("Content-Type", "application/x-protobuf")
w.WriteHeader(http.StatusOK)
if resp.ResponseStatus != 0 {
w.WriteHeader(resp.ResponseStatus)
} else {
w.WriteHeader(http.StatusOK)
}

Check warning on line 384 in exporters/otlp/otlpmetric/internal/otest/collector.go

View check run for this annotation

Codecov / codecov/patch

exporters/otlp/otlpmetric/internal/otest/collector.go#L380-L384

Added lines #L380 - L384 were not covered by tests
if resp.Response == nil {
_, _ = w.Write(emptyExportMetricsServiceResponse)
} else {
Expand Down
7 changes: 3 additions & 4 deletions exporters/otlp/otlpmetric/otlpmetrichttp/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,8 @@ func (c *client) UploadMetrics(ctx context.Context, protoMetrics *metricpb.Resou
}

var rErr error
switch resp.StatusCode {
case http.StatusOK:
switch sc := resp.StatusCode; {
case sc >= 200 && sc <= 299:
// Success, do not retry.

// Read the partial success message, if any.
Expand All @@ -202,8 +202,7 @@ func (c *client) UploadMetrics(ctx context.Context, protoMetrics *metricpb.Resou
}
}
return nil
case http.StatusTooManyRequests,
http.StatusServiceUnavailable:
case sc == http.StatusTooManyRequests, sc == http.StatusServiceUnavailable:
// Retry-able failure.
rErr = newResponseError(resp.Header)

Expand Down
6 changes: 3 additions & 3 deletions exporters/otlp/otlptrace/otlptracehttp/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,8 @@ func (d *client) UploadTraces(ctx context.Context, protoSpans []*tracepb.Resourc
}()
}

switch resp.StatusCode {
case http.StatusOK:
switch sc := resp.StatusCode; {
case sc >= 200 && sc <= 299:
// Success, do not retry.
// Read the partial success message, if any.
var respData bytes.Buffer
Expand All @@ -191,7 +191,7 @@ func (d *client) UploadTraces(ctx context.Context, protoSpans []*tracepb.Resourc
}
return nil

case http.StatusTooManyRequests, http.StatusServiceUnavailable:
case sc == http.StatusTooManyRequests, sc == http.StatusServiceUnavailable:
// Retry-able failures. Drain the body to reuse the connection.
if _, err := io.Copy(io.Discard, resp.Body); err != nil {
otel.Handle(err)
Expand Down
31 changes: 31 additions & 0 deletions exporters/otlp/otlptrace/otlptracehttp/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -401,3 +401,34 @@ func TestPartialSuccess(t *testing.T) {
require.Contains(t, errs[0].Error(), "partially successful")
require.Contains(t, errs[0].Error(), "2 spans rejected")
}

func TestOtherHTTPSuccess(t *testing.T) {
for code := 201; code <= 299; code++ {
t.Run(fmt.Sprintf("status_%d", code), func(t *testing.T) {
mcCfg := mockCollectorConfig{
InjectHTTPStatus: []int{code},
}
mc := runMockCollector(t, mcCfg)
defer mc.MustStop(t)
driver := otlptracehttp.NewClient(
otlptracehttp.WithEndpoint(mc.Endpoint()),
otlptracehttp.WithInsecure(),
)
ctx := context.Background()
exporter, err := otlptrace.New(ctx, driver)
require.NoError(t, err)
defer func() {
assert.NoError(t, exporter.Shutdown(context.Background()))
}()

errs := []error{}
otel.SetErrorHandler(otel.ErrorHandlerFunc(func(err error) {
errs = append(errs, err)
}))
err = exporter.ExportSpans(ctx, otlptracetest.SingleReadOnlySpan())
assert.NoError(t, err)

assert.Equal(t, 0, len(errs))
})
}
}