-
Notifications
You must be signed in to change notification settings - Fork 417
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.
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
Add interface for a trace recorder #44
Conversation
…try-cpp into recorder-interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implies the recorder passed to the constructor can't be nullptr, right? Is this worth documenting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we will have similar Recordable
class for metrics and logs moving forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for using std::unique_ptr
instead of nonstd::unique_ptr
in this interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Recorder should be an implementation detail of the SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.