add Scope.set_span_status #75
Conversation
src/core/opentelemetry.ml
Outdated
| @@ -807,6 +918,7 @@ module Scope : sig | |||
| trace_id: Trace_id.t; | |||
| span_id: Span_id.t; | |||
| mutable items: item_list; | |||
| mutable span_status: Span.status option; | |||
There was a problem hiding this comment.
I had to move the Span module up to access it here. But perhaps a better idea would be to extract the status-related code into a Span_status module?
There was a problem hiding this comment.
Because now Scope is in Trace section
There was a problem hiding this comment.
Span_status could make sense, indeed, since it's its own type!
There was a problem hiding this comment.
would it make sense to put the status in items, for the same reasons we introduced item_list recently?
src/core/opentelemetry.ml
Outdated
| match scope.span_status with | ||
| | Some status -> status | ||
| | None -> | ||
| (match res with | ||
| | Ok () -> default_status ~code:Status_code_ok () | ||
| | Error (e, bt) -> | ||
| (* add backtrace *) | ||
| Scope.record_exception scope e bt; | ||
| default_status ~code:Status_code_error | ||
| ~message:(Printexc.to_string e) ()) |
There was a problem hiding this comment.
I wonder if the order of preference is correct here 🤔 My thought was that I don't want to override what the user has set.
But then it means that if the user has explicitly set an Ok status and an exception is raised, the span will remain in the Ok status.
There was a problem hiding this comment.
looking at what the official sdk does, I found this. It seems like we shouldn't set Ok unless it's explicitly done by the user; and so it makes sense to not override the user's explicit choice (maybe the exception is not an actual error).
There was a problem hiding this comment.
It seems like we shouldn't set Ok unless it's explicitly done by the user
I agree and adapted the code
| end = struct | ||
| type item_list = | ||
| | Nil | ||
| | Ev of Event.t * item_list | ||
| | Attr of key_value * item_list | ||
| | Span_link of Span_link.t * item_list | ||
| | Span_status of Span_status.t * item_list |
There was a problem hiding this comment.
Not sure how it matters since that's internal anyway, but this type doesn't enforce the fact that there can only be one status per span.
There was a problem hiding this comment.
I was thinking maybe we can add this info to the Nil constructor
type item_list =
| Nil of Span_status.t option
| Ev of Event.t * item_list
| Attr of key_value * item_list
| Span_link of Span_link.t * item_listBut I'm not sure how it will scale if we add more elements. We may want to switch to a record if we add more elements but then I don't know if that's any better that my initial implementation?
There was a problem hiding this comment.
I don't think it matters too much that there can be only one :). We can just pick the last one.
No description provided.