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.

Sign up for GitHub

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

Jump to bottom

Introduce a functional option for span.RecordError to request that sets the status if the supplied error is not nil #1677

Open
MrAlias opened this issue Mar 8, 2021 · 6 comments · May be fixed by #5762
Open

Introduce a functional option for span.RecordError to request that sets the status if the supplied error is not nil #1677

MrAlias opened this issue Mar 8, 2021 · 6 comments · May be fixed by #5762
Labels
area:trace Part of OpenTelemetry tracing help wanted Extra attention is needed pkg:API Related to an API package

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Mar 8, 2021

Originally posted by @seh in #1661 (comment)
@MrAlias MrAlias mentioned this issue Mar 8, 2021
Closed
@MrAlias MrAlias added area:trace Part of OpenTelemetry tracing pkg:API Related to an API package release:after-ga labels Mar 8, 2021
@lastchiliarch
Copy link
Contributor

I'd like to take this one.

I'm going to add an IsRecordError filed to SpanConfig and new event option WithRecordError.
If spanConfig.IsRecordError is true when RecordError fucntion is called with options inlucde WithRecordError, I will call the s.SetStatus(codes.Error, "") to set status.

Glad to hear from you, if anything was misunderstood.

@MrAlias
Copy link
Contributor Author

MrAlias commented Apr 16, 2021

I'm going to add an IsRecordError filed to SpanConfig and new event option WithRecordError.
If spanConfig.IsRecordError is true when RecordError fucntion is called with options inlucde WithRecordError, I will call the s.SetStatus(codes.Error, "") to set status.

I'm not sure this is what I or others were thinking originally. Instead, add an EventOption that was named WithStatus. E.g.

func WithStatus() EventOption

This would be passed to the call of RecordError when instrumentation wanted to record an error and set the span status to Error.

lastchiliarch added a commit to lastchiliarch/opentelemetry-go that referenced this issue Apr 17, 2021
@lastchiliarch
Add WithStatus option to set Span status at the same time (open-telem…
74fa4b6 
…etry#1677)

Signed-off-by: lastchiliarch <lastchiliarch@163.com>
@lastchiliarch lastchiliarch mentioned this issue Apr 17, 2021
Closed
lastchiliarch added a commit to lastchiliarch/opentelemetry-go that referenced this issue Apr 17, 2021
@lastchiliarch
Add WithStatus option to set Span status at the same time (open-telem…
5d238e8 
…etry#1677)

Signed-off-by: lastchiliarch <lastchiliarch@163.com>
lastchiliarch added a commit to lastchiliarch/opentelemetry-go that referenced this issue Apr 17, 2021
@lastchiliarch
Add WithStatus option to set Span status at the same time (open-telem…
5ae2893 
…etry#1677)

Signed-off-by: lastchiliarch <lastchiliarch@163.com>
lastchiliarch added a commit to lastchiliarch/opentelemetry-go that referenced this issue Apr 17, 2021
@lastchiliarch
Add WithStatus option to set Span status at the same time (open-telem…
1d42dfa 
…etry#1677)

Signed-off-by: lastchiliarch <lastchiliarch@163.com>
lastchiliarch added a commit to lastchiliarch/opentelemetry-go that referenced this issue Apr 17, 2021
@lastchiliarch
Add WithStatus option to set Span status at the same time (open-telem…
d3b58b6 
…etry#1677)

Signed-off-by: lastchiliarch <lastchiliarch@163.com>
lastchiliarch added a commit to lastchiliarch/opentelemetry-go that referenced this issue Apr 17, 2021
@lastchiliarch
Add WithStatus option to set Span status at the same time (open-telem…
c1573c2 
…etry#1677)

Signed-off-by: lastchiliarch <lastchiliarch@163.com>
lastchiliarch added a commit to lastchiliarch/opentelemetry-go that referenced this issue Apr 17, 2021
@lastchiliarch
Add WithStatus option to set Span status at the same time (open-telem…
b1ee5d9 
…etry#1677)
lastchiliarch added a commit to lastchiliarch/opentelemetry-go that referenced this issue Apr 27, 2021
@lastchiliarch
Changed signature of the Span RecordError, replace EventOption by E…
364ad1e 
…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 <lastchiliarch@163.com>
lastchiliarch added a commit to lastchiliarch/opentelemetry-go that referenced this issue May 5, 2021
@lastchiliarch
Changed signature of the Span RecordError, replace EventOption by E…
92558f9 
…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 <lastchiliarch@163.com>
@treuherz
Copy link

It looks like this got stuck after #1677 fell behind a bunch of refactors. If @lastchiliarch isn't able to take it forward I'd be interested in picking this up in a new PR.

@MrAlias MrAlias added help wanted Extra attention is needed and removed release:after-1.0 labels May 3, 2022
@treuherz treuherz mentioned this issue May 5, 2022
Closed
treuherz added a commit to treuherz/opentelemetry-go that referenced this issue May 5, 2022
@treuherz
Add WithStatus option for use with RecordError
7b605f0 
Fixes open-telemetry#1677 by adding a functional option that sets the Status of a
span when calling RecordError.
treuherz added a commit to treuherz/opentelemetry-go that referenced this issue May 5, 2022
@treuherz
Add WithStatus option for use with RecordError
201ed00 
Fixes open-telemetry#1677 by adding a functional option that sets the Status of a
span when calling RecordError.
@tmc
Copy link

tmc commented Aug 31, 2024

@treuherz this would be nice to see in.

@treuherz
Copy link

treuherz commented Sep 1, 2024

Apologies, I wasn’t able to find a satisfactory backwards-compatible way of making this happen. Maybe inspiration will strike someone else!

@amanakin amanakin linked a pull request Sep 2, 2024 that will close this issue
Open
@amanakin
Copy link
Contributor

amanakin commented Sep 3, 2024

Hey @MrAlias, tried to solve an issue.
Can you take a look at PR, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area:trace Part of OpenTelemetry tracing help wanted Extra attention is needed pkg:API Related to an API package
Projects
None yet
Milestone
No milestone
5 participants
@tmc @treuherz @lastchiliarch @MrAlias @amanakin