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 @@ -21,6 +21,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Add info and debug logging to the metric SDK. (#4315)
- The `go.opentelemetry.io/otel/semconv/v1.21.0` package.
The package contains semantic conventions from the `v1.21.0` version of the OpenTelemetry Semantic Conventions. (#4362)
- 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)
- Document the `Temporality` and `Aggregation` methods of the `"go.opentelemetry.io/otel/sdk/metric".Exporter"` need to be concurrent safe. (#4381)
- Expand the set of units supported by the prometheus exporter, and don't add unit suffixes if they are already present in `go.opentelemetry.op/otel/exporters/prometheus` (#4374)

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 @@ -153,8 +153,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 @@ -179,8 +179,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 @@ -164,8 +164,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 @@ -190,7 +190,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))
})
}
}