fix: WithStackTrace option on AddEvent method #2336
Conversation
Signed-off-by: marc audefroy <marc.audefroy@corp.ovh.com>
marcaudefroy
requested review from
Aneurysm9,
dashpole,
evantorrie,
jmacd,
MadVikingGod,
MrAlias,
paivagustavo,
pellared and
XSAM
as code owners
|
|
|
Hi, this PR is related to this PR: #2163 |
| if c.StackTrace() { | ||
| o = append(o, trace.WithAttributes( | ||
| semconv.ExceptionStacktraceKey.String(recordStackTrace()), | ||
| )) | ||
| } |
There was a problem hiding this comment.
If we're going to allow adding a stack trace to all events then we should just move this into span.addEvent() and avoid calling trace.NewEventConfig() multiple times.
That said, I'm not sure it is correct to do this. The semantic conventions for the attribute key it is using relate to exceptions/errors. If there is no error being recorded then we should either not record a stack trace or we should record it using a different convention. To use a different convention we'd need to work that through the spec SIG to get agreement on the new convention.
There was a problem hiding this comment.
That said, I'm not sure it is correct to do this. The semantic conventions for the attribute key it is using relate to exceptions/errors. If there is no error being recorded then we should either not record a stack trace or we should record it using a different convention. To use a different convention we'd need to work that through the spec SIG to get agreement on the new convention.
That's a good point. Storing this under the exception semantic convention name does sound incorrect.
There was a problem hiding this comment.
I agree with you for the semantic convention.
"To use a different convention we'd need to work that through the spec SIG to get agreement on the new convention." => Or maybe add a stacktrace into an event for a no error/exception is just a bad idea and we must decline this PR. I create it 'cause it was weird to can add a stacktrace config to an event without it being taken into account.
| @@ -328,6 +328,14 @@ func (s *recordingSpan) AddEvent(name string, o ...trace.EventOption) { | |||
| if !s.IsRecording() { | |||
| return | |||
| } | |||
|
|
|||
| c := trace.NewEventConfig(o...) | |||
There was a problem hiding this comment.
Please move this to the wrapped addEvent method instead so we do not parse the EventConfig twice.
There was a problem hiding this comment.
That'll also mean removing the similar logic from RecordError() and End() to avoid building and setting the stack trace twice.
|
I'm going to close this for now given there isn't a clear path forward. Please reopen if this is in error or we resolve this in the future. |
Signed-off-by: marc audefroy marc.audefroy@corp.ovh.com