Add interface for a trace recorder#44
Conversation
…try-cpp into recorder-interface
This is part of the SDK, not the API, right? Really what this means is that the default SDK implementation that we provide will choose to do things via the Recorder approach? |
Yes, SDK only
Right, but I wouldn't say the recorder approach imposes many limitations. It really only says that a Span builds up a Recordable object that gets consumed by a Recorder. |
One limitation I see with this Recorder approach is, that there can only be one Recorder per Tracer. However, one Tracer can have multiple processors/exporters (per spec). |
But the Recorder can fan out to many Exporters without a problem, right? |
For sure. You can have both a fanning Recordable that builds up multiple data formats and a fanning Recorder that directs the data formats to multiple span processors. |
| public: | ||
| explicit Tracer(std::unique_ptr<Recorder> &&recorder) noexcept : recorder_{std::move(recorder)} {} | ||
|
|
||
| Recorder &recorder() const noexcept { return *recorder_; } |
There was a problem hiding this comment.
Implies the recorder passed to the constructor can't be nullptr, right? Is this worth documenting?
There was a problem hiding this comment.
Yes, I added a note.
|
@pyohannes - I added a composite example to the description. |
|
@reyang @pyohannes - could you review? |
Sure, on it now! |
| * @param code the status code | ||
| * @param description a description of the status | ||
| */ | ||
| virtual void SetStatus(trace_api::CanonicalCode code, |
There was a problem hiding this comment.
I wonder if we will have similar Recordable class for metrics and logs moving forward.
There was a problem hiding this comment.
I think that would make sense. I find it useful to have interfaces that just capture events and don't mandate an accessor interface or maintaining data in a structured form.
|
|
||
| bool Span::IsRecording() const noexcept | ||
| { | ||
| std::lock_guard<std::mutex> lock_guard{mu_}; |
There was a problem hiding this comment.
What's the point of using a mutex here?
I guess this would only make sense when the caller has knowledge about the mutex, otherwise by the time the caller received the return value, it might not be the current status.
There was a problem hiding this comment.
I suppose the return value might not be accurate if you have other threads simultaneously ending the span. But it would be undefined behavior without it and this will at least prevent TSAN from flagging it as an error.
| * | ||
| * This class is thread-compatible. | ||
| */ | ||
| class Recordable |
There was a problem hiding this comment.
Do you think that we need to represent parenting on this level? Given this interface, I don't see how an exporter can determine a parent span or recordable.
There was a problem hiding this comment.
Not all the methods are filled in (because we don't have all the types and methods in the api yet).
But there will be some kind of SpanContext type with a corresponding, SetSpanContext or something similar as well as AddLink methods. Those would pass the parenting information.
| * Record a span. | ||
| * @param recordable the data representation of the span. | ||
| */ | ||
| virtual void Record(std::unique_ptr<Recordable> &&recordable) noexcept = 0; |
There was a problem hiding this comment.
Is there a reason for using std::unique_ptr instead of nonstd::unique_ptr in this interface?
There was a problem hiding this comment.
I was limiting nostd::unique_ptr to the places where we need a stable ABI that could cross DSOs.
I don't know that are plans to support dynamically loadable Recorders. I feel like we should try to limit the points where we support plug-ability to avoid confusion and we already have two that we've discussed targeting (i.e. Tracer plugins and Exporter plugins)
There was a problem hiding this comment.
I think Recorder should be an implementation detail of the SDK.
reyang
left a comment
There was a problem hiding this comment.
LGTM, I have minor concerns regarding whether we will need it for logs and metrics, but that can be addressed later.
[DOC] Fix typo tace_id -> trace_id in logger.h (open-telemetry#2703)
Sets up the interface for a recorder that receives finished spans.
In this design, spans create a Recordable object when they're started and use it to build up their data representation.
A key goal of the design is to allow for performant implementations that avoid the cost of translating a "Span Data" like object into the exported formats.
A common SpanData implementation is still possible and fits into easly the Recordable concept.
So SpanData-like exporters are still possible, but more efficient implementations can derive from the Recordable class and write to the exported format directly.
Also, this design can support multiple formats and Recorders with composite types.