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.

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

Merged
merged 19 commits into from
Mar 13, 2020

Conversation

rnburn
Copy link
Contributor

@rnburn rnburn commented Mar 4, 2020

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.

class SpanData final : public Recordable {
  // provide accessor interface that can be used for translation
  nostd::string_view name() const noexcept { return name_; }
   // implement recordable methods
 void UpdateName(nostd::string_view name) noexcept override {
     //...
 }
 ...
 private:
   std::string name_;
};

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.

class CompositeRecordable final : public Recordable {
 public:
  void UpdateName(nostd::string_view name) noexcept override {
     for(auto& recordable : recordables_) {
        recordable->UpdateName(name);
     }
  }
 private:
   std::vector<std::unique_ptr<Recordable>> recordables_;
}

@rnburn rnburn changed the title Add an interface for a trace recorder Add interface for a trace recorder Mar 4, 2020
Copy link
Contributor

@g-easy g-easy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

CMakeLists.txt Show resolved Hide resolved
sdk/src/trace/BUILD Outdated Show resolved Hide resolved
sdk/src/trace/BUILD Show resolved Hide resolved
sdk/src/trace/recordable.h Outdated Show resolved Hide resolved
sdk/src/trace/span.h Show resolved Hide resolved
sdk/src/trace/span.h Outdated Show resolved Hide resolved
sdk/src/trace/span.h Show resolved Hide resolved
sdk/src/trace/span.h Outdated Show resolved Hide resolved
@g-easy
Copy link
Contributor

g-easy commented Mar 4, 2020

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.

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?

@rnburn
Copy link
Contributor Author

rnburn commented Mar 4, 2020

This is part of the SDK, not the API, right?

Yes, SDK only

Really what this means is that the default SDK implementation that we provide will choose to do things via the Recorder approach?

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.

@pyohannes
Copy link
Contributor

pyohannes commented Mar 4, 2020

While SpanData-like exporters are still possible, more efficient implementations can derive from the Recordable class and write to the exported format directly.

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).

@g-easy
Copy link
Contributor

g-easy commented Mar 4, 2020

one Recorder per Tracer

But the Recorder can fan out to many Exporters without a problem, right?

@rnburn
Copy link
Contributor Author

rnburn commented Mar 4, 2020

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.

sdk/src/trace/recordable.h Outdated Show resolved Hide resolved
sdk/src/trace/span.cc Outdated Show resolved Hide resolved
public:
explicit Tracer(std::unique_ptr<Recorder> &&recorder) noexcept : recorder_{std::move(recorder)} {}

Recorder &recorder() const noexcept { return *recorder_; }
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@rnburn
Copy link
Contributor Author

rnburn commented Mar 12, 2020

@pyohannes - I added a composite example to the description.

@rnburn
Copy link
Contributor Author

rnburn commented Mar 12, 2020

@reyang @pyohannes - could you review?

@reyang
Copy link
Member

reyang commented Mar 12, 2020

@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,
Copy link
Member

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.

Copy link
Contributor Author

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_};
Copy link
Member

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.

Copy link
Contributor Author

@rnburn rnburn Mar 12, 2020

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
Copy link
Contributor

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.

Copy link
Contributor Author

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;
Copy link
Contributor

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?

Copy link
Contributor Author

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)

Copy link
Contributor

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.

Copy link
Member

@reyang reyang left a 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.

@rnburn rnburn merged commit 2f228b0 into open-telemetry:master Mar 13, 2020
GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Jun 21, 2024
[DOC] Fix typo tace_id -> trace_id in logger.h (open-telemetry#2703)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants