Add WithStatus option to set Span status at the same time (#1677) #1817

lastchiliarch · 2021-04-17T10:09:58Z

Add WithStatus event option to set Span status at the same time, see #1677.
Signed-off-by: lastchiliarch [email protected]

codecov · 2021-04-17T10:12:00Z

Codecov Report

Merging #1817 (68fc2eb) into main (d20e722) will increase coverage by 0.0%.
The diff coverage is 80.0%.

@@          Coverage Diff          @@
##            main   #1817   +/-   ##
=====================================
  Coverage   79.2%   79.2%           
=====================================
  Files        139     139           
  Lines       7459    7476   +17     
=====================================
+ Hits        5908    5926   +18     
  Misses      1304    1304           
+ Partials     247     246    -1

Impacted Files	Coverage Δ
bridge/opentracing/internal/mock.go	`72.9% <0.0%> (-0.7%)`	⬇️
trace/noop.go	`57.1% <0.0%> (ø)`
trace/trace.go	`86.7% <ø> (ø)`
oteltest/span.go	`100.0% <100.0%> (ø)`
sdk/trace/span.go	`89.1% <100.0%> (+0.1%)`	⬆️
trace/config.go	`69.7% <100.0%> (+9.1%)`	⬆️
sdk/trace/batch_span_processor.go	`87.1% <0.0%> (+1.5%)`	⬆️

…etry#1677) Signed-off-by: lastchiliarch <[email protected]>

XSAM

Looks good overall.

sdk/trace/trace_test.go

sdk/trace/span.go

trace/config_test.go

…etry#1677) Signed-off-by: lastchiliarch <[email protected]>

MrAlias · 2021-04-17T18:19:54Z

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)


Suggested change

- Added `WithStatus` event option to set Span status at the same time when call RecordError. (#1677)

- Added `WithStatus` event option to set Span status when passed to `RecordError`. (#1817)

MrAlias · 2021-04-19T14:40:35Z

sdk/trace/span.go

+// This method does not change the Span status in default
+// To change Span status, pass `WithStatus` as options.
+// This method does nothing If this span is not being recorded or err is nil.


Suggested change

// This method does not change the Span status in default

// To change Span status, pass `WithStatus` as options.

// This method does nothing If this span is not being recorded or err is nil.

// This method does not change the Span status by default.

// To set the Span status, pass `WithStatus` as an option.

// This method does nothing if this span is not being recorded or err is nil.

MrAlias · 2021-04-19T14:50:14Z

trace/config.go

+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 {


Having this return a LifeCycleOption means that the life-cycle methods of the span will accept this. I'm not sure we do or don't want to do this. IIRC the specification might state that we cannot set the status in these methods, you'll need to double check that. If we do, the life-cycle methods will need to be updated to set the status when they receive this option.

Additionally, having this accept a boolean as an argument doesn't seem that useful. Why not accept a code and message for the status to set?

Additionally, having this accept a boolean as an argument doesn't seem that useful. Why not accept a code and message for the status to set?

I would second that, this at the minimum should accept a code and message as the span status concept has room to grow beyond the current tri-state values.

I'm not sure about this either. WithStatusdoesn't seems to be an EventOption, it does not change or affect an Event. It is only applicable to the RecordError method.

For example, using span.AddEvent("some event", trace.WithStatus()) is syntactically correct but does nothing.

I would prefer if we create another method to record an error with status or create an ErrorOption that wraps an EventOptions, for example:

span.RecordErrorStatus(err, code, "description", EventOptions..)

or

span.RecordError(err, trace.WithErrorStatus(code, "description"), trace.WithEventOpts(EventOptions..))

I like the creation of the ErrorOption approach. I could see there being many more things we want to pass here (we talked about maybe an option that would signal the stacktrace needed to be recorded).

What is dead
May never die

ErrorOption sounds good. I will use ErrorOption to replace WithStatusOption.
Thanks very much.

…rrorOption (open-telemetry#1677) * Change signature of the Span `RecordError, replace EventOption by ErrorOption * Add WithEventOpts, WithErrorStatus * Set status when WithErrorStatus is passed to RecordError Signed-off-by: lastchiliarch <[email protected]>

Aneurysm9

I'd really like to avoid the extra WithEventOptions() wrapper.

Aneurysm9 · 2021-04-29T16:54:25Z

bridge/opentracing/internal/mock.go

@@ -255,11 +255,13 @@ func (s *MockSpan) RecordError(err error, opts ...trace.EventOption) {
 	}

 	s.SetStatus(codes.Error, "")


This probably shouldn't be setting the status here. Looks like maybe it was missed in the removal of that functionality.

Aneurysm9 · 2021-04-29T16:55:30Z

CHANGELOG.md

@@ -212,6 +212,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

 ### Changed

+- Changed signature of the Span `RecordError` , replace EventOption by ErrorOption


Suggested change

- Changed signature of the Span `RecordError` , replace EventOption by ErrorOption

- Changed signature of the Span `RecordError`, replace EventOption with ErrorOption. (#1817)

Aneurysm9 · 2021-04-29T16:58:23Z

sdk/trace/span.go

+// This method does not change the Span status in default
+// To set the Span status, pass `WithErrorStatus` as an option.
+// This method does nothing If this span is not being recorded or err is nil.


Suggested change

// This method does not change the Span status in default

// To set the Span status, pass `WithErrorStatus` as an option.

// This method does nothing If this span is not being recorded or err is nil.

// This method does not change the Span status by default

// To set the Span status pass a `WithErrorStatus(status)` option.

// This method does nothing if this span is not being recorded or if err is nil.

Aneurysm9 · 2021-04-29T17:30:54Z

trace/config.go

+	for _, option := range options {
+		option.ApplyError(c)
+	}


I'd like to avoid the need to use WithEventOpts() when calling RecordError(). My first thought was that maybe something like this would work:

for _, option := range options { if evopt, ok := option.(EventOption); ok { c.EventOpts = append(c.EventOpts, option) } else { option.ApplyError(c) } }

But I don't think it does since the method signature now requires ErrorOptions to be passed instead of EventOptions. So, my next thought is could the relevant EventOption implementations gain an ErrorOption implementation that looks something like this:

func (o someOptionType) ApplyError(c *ErrorConfig) { c.EventOpts = append(c.EventOpts, o) }

I don't like the wrap either, I had tried this way to make it work before the wrap.

But it can't work, beacuse the compile complains about the type mismatch.
Error message:
Cannot use 'trace.WithTimestamp(errTime)' (type LifeCycleOption) as the type ErrorOption Type does not implement
'ErrorOption' as some methods are missing: ApplyError(config *ErrorConfig)

Code looks like this:

config: type timestampSpanOption time.Time func (o timestampSpanOption) ApplySpan(c *SpanConfig) { o.apply(c) } func (o timestampSpanOption) ApplyEvent(c *SpanConfig) { o.apply(c) } func (o timestampSpanOption) ApplyError(c *ErrorConfig) {c.Timestamp = time.Time(o)} func (timestampSpanOption) private() {} func (o timestampSpanOption) apply(c *SpanConfig) { c.Timestamp = time.Time(o) } client code: span.RecordError(s.err, trace.WithTimestamp(errTime)) RecordError: func (s *span) RecordError(err error, options ...trace.ErrorOption) { if s == nil || err == nil || !s.IsRecording() { return } c := trace.NewErrorConfig(options...) if c.Code != codes.Unset { s.SetStatus(c.Code, c.Message) } var eventOpts []trace.EventOption for _, option := range options { if option, ok := option.(trace.EventOption); ok { eventOpts = append(eventOpts, option) } } eventOpts = append(eventOpts, trace.WithAttributes( semconv.ExceptionTypeKey.String(typeStr(err)), semconv.ExceptionMessageKey.String(err.Error()), )) s.addEvent(semconv.ExceptionEventName, eventOpts...) }

I don't think it's a good idea to make LifeCycleOption implements ErrorOption.

If you know any idea, that will be a great help, I'd like to remove the wrap.

Why not just embed the ErrorOption in the EventOption? https://github.com/open-telemetry/opentelemetry-go/blob/main/CONTRIBUTING.md#dealing-with-overlap

Aneurysm9 · 2021-04-29T17:32:01Z

trace/config.go

+func (eventOptsErrorOption) private()                    {}
+
+// WithEventOpts set event options, will be passed to addEvent
+func WithEventOpts(option ...EventOption) ErrorOption {


Suggested change

func WithEventOpts(option ...EventOption) ErrorOption {

func WithEventOptions(option ...EventOption) ErrorOption {

If we can't get rid of this I'd like it to not use an abbreviated name.

Thanks very much.

Aneurysm9 · 2021-04-29T17:33:55Z

trace/trace.go

+	// This method does not change the Span status by default.
+	// To set the Span status, pass `WithErrorStatus` as an option.
+	// This method does nothing if this span is not being recorded or err is nil.


Suggested change

// This method does not change the Span status by default.

// To set the Span status, pass `WithErrorStatus` as an option.

// This method does nothing if this span is not being recorded or err is nil.

// This method does not change the Span status by default

// To set the Span status pass a `WithErrorStatus(status)` option.

// This method does nothing if this span is not being recorded or if err is nil.

…rrorOption (open-telemetry#1677) * Change signature of the Span `RecordError, replace EventOption by ErrorOption * Add WithEventOpts, WithErrorStatus * Set status when WithErrorStatus is passed to RecordError Signed-off-by: lastchiliarch <[email protected]>

Signed-off-by: lastchiliarch <[email protected]>

… into issue1677

Aneurysm9 · 2021-05-06T17:19:07Z

trace/config.go

+
+// ErrorOption applies an option to a ErrorConfig.
+type ErrorOption interface {
+	ApplyError(config *ErrorConfig)


Suggested change

ApplyError(config *ErrorConfig)

applyError(config *errorConfig)

We're working on normalizing configuration and options and part of that will involve making the config type and apply methods unexported. Can we get out ahead of that here?

Sure. I had renamed ApplyError to applyError.
But golangci-lint will fail if I changed ErrorConfig to errorConfig.

exported func NewErrorConfig returns unexported type *go.opentelemetry.io/otel/trace.errorConfig, which can be annoying to us.

newErrorConfig shouldn't need to be exported, either.

Since newErrorConfig will be used in by bridge/opentracing/internal/mock.go , oteltest/span.go and sdk/trace/span.go, it will take a lot of work to make it unexported at this time.

I think it's better to move all the code which implements Span interface to the package trace, so we can easily make the config unexported.

But there is another problem, since sdk/trace/span.go needs some code in sdk/trace/internal, we must change it at the same time.

I'm not sure about this solution, it will cause a lot of files to be changed.

Maybe we can keep NewErrorConfig exported and make a new pr to make it unexported, I'd like to take it.

Signed-off-by: lastchiliarch <[email protected]>

… into issue1677

Aneurysm9 · 2021-05-07T16:42:10Z

trace/config.go

+
+	// A private method to prevent users implementing the
+	// interface and so future additions to it will not
+	// violate compatibility.
+	private()


Suggested change

// A private method to prevent users implementing the

// interface and so future additions to it will not

// violate compatibility.

private()

This is no longer necessary since the applyError() method is unexported.

MrAlias · 2021-05-10T15:37:46Z

sdk/trace/span.go

-// than this method does nothing.
-func (s *span) RecordError(err error, opts ...trace.EventOption) {
+// RecordError will record err as a span event for this span.
+// This method does not change the Span status by default


Suggested change

// This method does not change the Span status by default

// This method does not change the Span status by default.

MrAlias · 2021-05-10T15:39:03Z

trace/config.go

@@ -203,3 +205,56 @@ func (i instrumentationVersionOption) ApplyTracer(config *TracerConfig) {
 }

 func (instrumentationVersionOption) private() {}
+
+// ErrorConfig is a group of options for error.


Suggested change

// ErrorConfig is a group of options for error.

// ErrorConfig is a group of options for error events.

MrAlias · 2021-05-10T15:39:52Z

trace/config.go

+}
+func (statusErrorOption) private() {}
+
+// WithErrorStatus set the error code and message when code is not codes.Unset


Suggested change

// WithErrorStatus set the error code and message when code is not codes.Unset

// WithErrorStatus set the error code and message when code is not codes.Unset.

MrAlias · 2021-05-10T15:40:33Z

trace/trace.go

-	// span is not being recorded or err is nil than this method does nothing.
-	RecordError(err error, options ...EventOption)
+	// RecordError will record err as an exception span event for this span.
+	// This method does not change the Span status by default


Suggested change

// This method does not change the Span status by default

// This method does not change the Span status by default.

MrAlias · 2021-05-10T15:47:51Z

trace/config.go

+	for _, option := range options {
+		option.ApplyError(c)
+	}


Why not just embed the ErrorOption in the EventOption? https://github.com/open-telemetry/opentelemetry-go/blob/main/CONTRIBUTING.md#dealing-with-overlap

MrAlias · 2021-06-08T17:14:13Z

Closing as HEAD has progressed far enough this will likely need to be re-evaluated.

lastchiliarch requested review from Aneurysm9, dashpole, evantorrie, jmacd, MrAlias, paivagustavo and XSAM as code owners April 17, 2021 10:09

lastchiliarch force-pushed the issue1677 branch 2 times, most recently from 5d238e8 to 5ae2893 Compare April 17, 2021 12:49

Add WithStatus option to set Span status at the same time (open-telem…

1d42dfa

…etry#1677) Signed-off-by: lastchiliarch <[email protected]>

lastchiliarch force-pushed the issue1677 branch from 5ae2893 to 1d42dfa Compare April 17, 2021 13:27

XSAM approved these changes Apr 17, 2021

View reviewed changes

sdk/trace/trace_test.go Outdated Show resolved Hide resolved

sdk/trace/span.go Outdated Show resolved Hide resolved

trace/config_test.go Outdated Show resolved Hide resolved

lastchiliarch added 2 commits April 17, 2021 23:18

Add WithStatus option to set Span status at the same time (open-telem…

d3b58b6

…etry#1677) Signed-off-by: lastchiliarch <[email protected]>

Add WithStatus option to set Span status at the same time (open-telem…

c1573c2

…etry#1677) Signed-off-by: lastchiliarch <[email protected]>

MrAlias requested changes Apr 19, 2021

View reviewed changes

MrAlias reviewed Apr 19, 2021

View reviewed changes

lastchiliarch force-pushed the issue1677 branch 2 times, most recently from 05cad56 to ca16c7a Compare April 27, 2021 15:20

lastchiliarch requested review from XSAM and MrAlias April 27, 2021 15:34

Aneurysm9 suggested changes Apr 29, 2021

View reviewed changes

lastchiliarch added 3 commits May 6, 2021 00:08

Merge remote-tracking branch 'upstream/main' into issue1677

1b54a85

Merge remote-tracking branch 'upstream/main' into issue1677

f6357e4

Signed-off-by: lastchiliarch <[email protected]>

lastchiliarch force-pushed the issue1677 branch from ca16c7a to f6357e4 Compare May 5, 2021 16:46

lastchiliarch added 2 commits May 6, 2021 00:53

Merge remote-tracking branch 'upstream/main' into issue1677

fe85947

Signed-off-by: lastchiliarch <[email protected]>

Merge branch 'issue1677' of github.com:lastchiliarch/opentelemetry-go…

339c8d0

… into issue1677

lastchiliarch requested a review from Aneurysm9 May 5, 2021 17:04

Aneurysm9 reviewed May 6, 2021

View reviewed changes

Aneurysm9 and others added 4 commits May 6, 2021 17:16

Merge branch 'main' into issue1677

91ea8e2

Change ErrorConfig and ApplyError to unexported.

62cceed

Signed-off-by: lastchiliarch <[email protected]>

Change ErrorConfig and ApplyError to unexported

135b221

Signed-off-by: lastchiliarch <[email protected]>

Merge branch 'issue1677' of github.com:lastchiliarch/opentelemetry-go…

68fc2eb

… into issue1677

lastchiliarch requested a review from Aneurysm9 May 7, 2021 03:54

Aneurysm9 reviewed May 7, 2021

View reviewed changes

MrAlias reviewed May 10, 2021

View reviewed changes

MrAlias closed this Jun 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WithStatus option to set Span status at the same time (#1677) #1817

Add WithStatus option to set Span status at the same time (#1677) #1817

lastchiliarch commented Apr 17, 2021

codecov bot commented Apr 17, 2021 •

edited

Loading

XSAM left a comment

MrAlias Apr 17, 2021

MrAlias Apr 19, 2021

MrAlias Apr 19, 2021

Aneurysm9 Apr 19, 2021

paivagustavo Apr 19, 2021

MrAlias Apr 20, 2021

Aneurysm9 Apr 20, 2021

lastchiliarch Apr 21, 2021 •

edited

Loading

Aneurysm9 left a comment

Aneurysm9 Apr 29, 2021

Aneurysm9 Apr 29, 2021

Aneurysm9 Apr 29, 2021

Aneurysm9 Apr 29, 2021

lastchiliarch Apr 30, 2021

MrAlias May 10, 2021

Aneurysm9 Apr 29, 2021

lastchiliarch Apr 30, 2021

Aneurysm9 Apr 29, 2021

Aneurysm9 May 6, 2021

lastchiliarch May 7, 2021

Aneurysm9 May 7, 2021

lastchiliarch May 8, 2021

Aneurysm9 May 7, 2021

MrAlias May 10, 2021

MrAlias May 10, 2021

MrAlias May 10, 2021

MrAlias May 10, 2021

MrAlias May 10, 2021

MrAlias commented Jun 8, 2021

		@@ -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)

		@@ -255,11 +255,13 @@ func (s *MockSpan) RecordError(err error, opts ...trace.EventOption) {
		}

		s.SetStatus(codes.Error, "")

		@@ -212,6 +212,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

		### Changed

		- Changed signature of the Span `RecordError` , replace EventOption by ErrorOption

	func WithEventOpts(option ...EventOption) ErrorOption {
	func WithEventOptions(option ...EventOption) ErrorOption {

	ApplyError(config *ErrorConfig)
	applyError(config *errorConfig)

	// This method does not change the Span status by default
	// This method does not change the Span status by default.

	// ErrorConfig is a group of options for error.
	// ErrorConfig is a group of options for error events.

	// WithErrorStatus set the error code and message when code is not codes.Unset
	// WithErrorStatus set the error code and message when code is not codes.Unset.

Add WithStatus option to set Span status at the same time (#1677) #1817

Add WithStatus option to set Span status at the same time (#1677) #1817

Conversation

lastchiliarch commented Apr 17, 2021

codecov bot commented Apr 17, 2021 • edited Loading

Codecov Report

XSAM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lastchiliarch Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

Aneurysm9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrAlias commented Jun 8, 2021

codecov bot commented Apr 17, 2021 •

edited

Loading

lastchiliarch Apr 21, 2021 •

edited

Loading