From 1d42dfa3fa92597f883940b5eed62cd714769bfa Mon Sep 17 00:00:00 2001 From: lastchiliarch Date: Sat, 17 Apr 2021 18:03:14 +0800 Subject: [PATCH] Add WithStatus option to set Span status at the same time (#1677) Signed-off-by: lastchiliarch --- CHANGELOG.md | 1 + sdk/trace/span.go | 12 ++++++---- sdk/trace/trace_test.go | 53 +++++++++++++++++++++++++++++++++++++++++ trace/config.go | 14 +++++++++++ trace/config_test.go | 14 +++++++++++ 5 files changed, 90 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f08abbc7a66..51ef7cbb4fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -141,6 +141,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Added +- Added `WithStatus` event option to set Span status at the same time when call RecordError. (#1677) - Added `resource.Default()` for use with meter and tracer providers. (#1507) - `AttributePerEventCountLimit` and `AttributePerLinkCountLimit` for `SpanLimits`. (#1535) - Added `Keys()` method to `propagation.TextMapCarrier` and `propagation.HeaderCarrier` to adapt `http.Header` to this interface. (#1544) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 8ca19a896c6..6683a275e9a 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -244,14 +244,18 @@ func (s *span) End(options ...trace.SpanOption) { } } -// RecordError will record err as a span event for this span. An additional call to -// SetStatus is required if the Status of the Span should be set to Error, this method -// does not change the Span status. If this span is not being recorded or err is nil -// than this method does nothing. +// RecordError will record err as a span event for this span. +// this method does not change the Span status in default. +// If you want to change Span status, pass WithStatus as opts. +// this metod does nothing If this span is not being recorded or err is nil. func (s *span) RecordError(err error, opts ...trace.EventOption) { if s == nil || err == nil || !s.IsRecording() { return } + c := trace.NewEventConfig(opts...) + if c.WithStatus { + s.SetStatus(codes.Error, "") + } opts = append(opts, trace.WithAttributes( semconv.ExceptionTypeKey.String(typeStr(err)), diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 014329219fc..9ba05f699cb 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -1164,6 +1164,59 @@ func TestRecordErrorNil(t *testing.T) { } } +func TestRecordErrorWithStatus(t *testing.T) { + scenarios := []struct { + err error + typ string + msg string + }{ + { + err: ottest.NewTestError("test error"), + typ: "go.opentelemetry.io/otel/internal/internaltest.TestError", + msg: "test error", + }, + } + + for _, s := range scenarios { + te := NewTestExporter() + tp := NewTracerProvider(WithSyncer(te), WithResource(resource.Empty())) + span := startSpan(tp, "RecordError") + + errTime := time.Now() + span.RecordError(s.err, trace.WithTimestamp(errTime), trace.WithStatus(true)) + + got, err := endSpan(te, span) + if err != nil { + t.Fatal(err) + } + + want := &SpanSnapshot{ + SpanContext: trace.NewSpanContext(trace.SpanContextConfig{ + TraceID: tid, + TraceFlags: 0x1, + }), + Parent: sc.WithRemote(true), + Name: "span0", + StatusCode: codes.Error, + SpanKind: trace.SpanKindInternal, + MessageEvents: []trace.Event{ + { + Name: semconv.ExceptionEventName, + Time: errTime, + Attributes: []attribute.KeyValue{ + semconv.ExceptionTypeKey.String(s.typ), + semconv.ExceptionMessageKey.String(s.msg), + }, + }, + }, + InstrumentationLibrary: instrumentation.Library{Name: "RecordError"}, + } + if diff := cmpDiff(got, want); diff != "" { + t.Errorf("SpanErrorOptions: -got +want %s", diff) + } + } +} + func TestWithSpanKind(t *testing.T) { te := NewTestExporter() tp := NewTracerProvider(WithSyncer(te), WithSampler(AlwaysSample()), WithResource(resource.Empty())) diff --git a/trace/config.go b/trace/config.go index ea30ee35f15..f434b993612 100644 --- a/trace/config.go +++ b/trace/config.go @@ -60,6 +60,8 @@ type SpanConfig struct { NewRoot bool // SpanKind is the role a Span has in a trace. SpanKind SpanKind + // WithStatus is used to control whether or not set Span status when RecordError + WithStatus bool } // NewSpanConfig applies all the options to a returned SpanConfig. @@ -203,3 +205,15 @@ func (i instrumentationVersionOption) ApplyTracer(config *TracerConfig) { } func (instrumentationVersionOption) private() {} + +type withStatusSpanOption bool + +func (o withStatusSpanOption) ApplySpan(c *SpanConfig) { o.apply(c) } +func (o withStatusSpanOption) ApplyEvent(c *SpanConfig) { o.apply(c) } +func (withStatusSpanOption) private() {} +func (o withStatusSpanOption) apply(c *SpanConfig) { c.WithStatus = bool(o) } + +// WithStatus set the status at the same time when RecordError +func WithStatus(o bool) LifeCycleOption { + return withStatusSpanOption(o) +} diff --git a/trace/config_test.go b/trace/config_test.go index 9375191c40a..703119b278a 100644 --- a/trace/config_test.go +++ b/trace/config_test.go @@ -40,6 +40,10 @@ func TestNewSpanConfig(t *testing.T) { Attributes: []attribute.KeyValue{k1v2, k2v2}, } + withStatusOpt := WithStatus(true) + //just for coverage + withStatusOpt.private() + tests := []struct { options []SpanOption expected *SpanConfig @@ -151,6 +155,14 @@ func TestNewSpanConfig(t *testing.T) { SpanKind: SpanKindConsumer, }, }, + { + []SpanOption{ + withStatusOpt, + }, + &SpanConfig{ + WithStatus: true, + }, + }, { // Everything should work together. []SpanOption{ @@ -159,6 +171,7 @@ func TestNewSpanConfig(t *testing.T) { WithLinks(link1, link2), WithNewRoot(), WithSpanKind(SpanKindConsumer), + WithStatus(true), }, &SpanConfig{ Attributes: []attribute.KeyValue{k1v1}, @@ -166,6 +179,7 @@ func TestNewSpanConfig(t *testing.T) { Links: []Link{link1, link2}, NewRoot: true, SpanKind: SpanKindConsumer, + WithStatus: true, }, }, }