From 8df11939c1d6093d84a52fdc380010b525514d05 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 2 Nov 2022 09:08:35 -0700 Subject: [PATCH 1/3] Do not handle empty partial OTLP successes Fix #3432. The OTLP server will respond with empty partial success responses (i.e. empty messages and 0 count). Treat these as equivalent to it not being set/present like the documentation specifies in the proto: https://github.com/open-telemetry/opentelemetry-proto/blob/724e427879e3d2bae2edc0218fff06e37b9eb46e/opentelemetry/proto/collector/trace/v1/trace_service.proto#L58 --- exporters/otlp/internal/partialsuccess.go | 34 ++++++++----------- .../otlp/otlptrace/otlptracegrpc/client.go | 11 +++--- .../otlp/otlptrace/otlptracehttp/client.go | 11 +++--- 3 files changed, 27 insertions(+), 29 deletions(-) diff --git a/exporters/otlp/internal/partialsuccess.go b/exporters/otlp/internal/partialsuccess.go index 7994706ab51..9ab89b37574 100644 --- a/exporters/otlp/internal/partialsuccess.go +++ b/exporters/otlp/internal/partialsuccess.go @@ -16,19 +16,6 @@ package internal // import "go.opentelemetry.io/otel/exporters/otlp/internal" import "fmt" -// PartialSuccessDropKind indicates the kind of partial success error -// received by an OTLP exporter, which corresponds with the signal -// being exported. -type PartialSuccessDropKind string - -const ( - // TracingPartialSuccess indicates that some spans were rejected. - TracingPartialSuccess PartialSuccessDropKind = "spans" - - // MetricsPartialSuccess indicates that some metric data points were rejected. - MetricsPartialSuccess PartialSuccessDropKind = "metric data points" -) - // PartialSuccess represents the underlying error for all handling // OTLP partial success messages. Use `errors.Is(err, // PartialSuccess{})` to test whether an error passed to the OTel @@ -36,7 +23,7 @@ const ( type PartialSuccess struct { ErrorMessage string RejectedItems int64 - RejectedKind PartialSuccessDropKind + RejectedKind string } var _ error = PartialSuccess{} @@ -56,13 +43,22 @@ func (ps PartialSuccess) Is(err error) bool { return ok } -// PartialSuccessToError produces an error suitable for passing to -// `otel.Handle()` out of the fields in a partial success response, -// independent of which signal produced the outcome. -func PartialSuccessToError(kind PartialSuccessDropKind, itemsRejected int64, errorMessage string) error { +// TracePartialSuccessError returns an error describing a partial success +// response for the trace signal. +func TracePartialSuccessError(itemsRejected int64, errorMessage string) error { + return PartialSuccess{ + ErrorMessage: errorMessage, + RejectedItems: itemsRejected, + RejectedKind: "spans", + } +} + +// MetricPartialSuccessError returns an error describing a partial success +// response for the metric signal. +func MetricPartialSuccessError(itemsRejected int64, errorMessage string) error { return PartialSuccess{ ErrorMessage: errorMessage, RejectedItems: itemsRejected, - RejectedKind: kind, + RejectedKind: "metric data points", } } diff --git a/exporters/otlp/otlptrace/otlptracegrpc/client.go b/exporters/otlp/otlptrace/otlptracegrpc/client.go index 9d6e1898b14..fe23f8e3766 100644 --- a/exporters/otlp/otlptrace/otlptracegrpc/client.go +++ b/exporters/otlp/otlptrace/otlptracegrpc/client.go @@ -202,11 +202,12 @@ func (c *client) UploadTraces(ctx context.Context, protoSpans []*tracepb.Resourc ResourceSpans: protoSpans, }) if resp != nil && resp.PartialSuccess != nil { - otel.Handle(internal.PartialSuccessToError( - internal.TracingPartialSuccess, - resp.PartialSuccess.RejectedSpans, - resp.PartialSuccess.ErrorMessage, - )) + msg := resp.PartialSuccess.GetErrorMessage() + n := resp.PartialSuccess.GetRejectedSpans() + if n != 0 || msg != "" { + err := internal.TracePartialSuccessError(n, msg) + otel.Handle(err) + } } // nil is converted to OK. if status.Code(err) == codes.OK { diff --git a/exporters/otlp/otlptrace/otlptracehttp/client.go b/exporters/otlp/otlptrace/otlptracehttp/client.go index 8f742dfc1bd..79b8af73286 100644 --- a/exporters/otlp/otlptrace/otlptracehttp/client.go +++ b/exporters/otlp/otlptrace/otlptracehttp/client.go @@ -180,11 +180,12 @@ func (d *client) UploadTraces(ctx context.Context, protoSpans []*tracepb.Resourc } if respProto.PartialSuccess != nil { - otel.Handle(internal.PartialSuccessToError( - internal.TracingPartialSuccess, - respProto.PartialSuccess.RejectedSpans, - respProto.PartialSuccess.ErrorMessage, - )) + msg := respProto.PartialSuccess.GetErrorMessage() + n := respProto.PartialSuccess.GetRejectedSpans() + if n != 0 || msg != "" { + err := internal.TracePartialSuccessError(n, msg) + otel.Handle(err) + } } } return nil From ca6b2244e47a7da6785c8c9be8f2a391111561da Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 2 Nov 2022 09:17:03 -0700 Subject: [PATCH 2/3] Fix tests --- exporters/otlp/internal/partialsuccess_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/exporters/otlp/internal/partialsuccess_test.go b/exporters/otlp/internal/partialsuccess_test.go index 3a7b0f0a6ef..9032f244cc4 100644 --- a/exporters/otlp/internal/partialsuccess_test.go +++ b/exporters/otlp/internal/partialsuccess_test.go @@ -36,9 +36,8 @@ func requireErrorString(t *testing.T, expect string, err error) { } func TestPartialSuccessFormat(t *testing.T) { - requireErrorString(t, "empty message (0 metric data points rejected)", PartialSuccessToError(MetricsPartialSuccess, 0, "")) - requireErrorString(t, "help help (0 metric data points rejected)", PartialSuccessToError(MetricsPartialSuccess, 0, "help help")) - requireErrorString(t, "what happened (10 metric data points rejected)", PartialSuccessToError(MetricsPartialSuccess, 10, "what happened")) - requireErrorString(t, "what happened (15 spans rejected)", PartialSuccessToError(TracingPartialSuccess, 15, "what happened")) - requireErrorString(t, "empty message (7 log records rejected)", PartialSuccessToError("log records", 7, "")) + requireErrorString(t, "empty message (0 metric data points rejected)", MetricPartialSuccessError(0, "")) + requireErrorString(t, "help help (0 metric data points rejected)", MetricPartialSuccessError(0, "help help")) + requireErrorString(t, "what happened (10 metric data points rejected)", MetricPartialSuccessError(10, "what happened")) + requireErrorString(t, "what happened (15 spans rejected)", TracePartialSuccessError(15, "what happened")) } From 54b3fe3f5cbc43eb6f89e623adc312399fceb376 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 2 Nov 2022 09:19:52 -0700 Subject: [PATCH 3/3] Add changes to changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e6682f1cca3..65c829728c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Cumulative metrics from the OpenCensus bridge (`go.opentelemetry.io/otel/bridge/opencensus`) are defined as monotonic sums, instead of non-monotonic. (#3389) - Asynchronous counters (`Counter` and `UpDownCounter`) from the metric SDK now produce delta sums when configured with delta temporality. (#3398) - Exported `Status` codes in the `go.opentelemetry.io/otel/exporters/zipkin` exporter are now exported as all upper case values. (#3340) +- Do not report empty partial-success responses in the `go.opentelemetry.io/otel/exporters/otlp` exporters. (#3438, #3432) ## [1.11.1/0.33.0] 2022-10-19