-
Notifications
You must be signed in to change notification settings - Fork 411
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
Support for attributes on spans #117
Changes from all commits
39e26d2
f9d8c7e
3de6cbf
c300da6
f07d893
08d24a2
d684dc6
f4d6661
852be31
81b1f03
9149aa1
da1c599
bfc7ae0
0b952af
d0a25f2
21ef99f
dd8e1b0
8723b37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
#pragma once | ||
|
||
#include "opentelemetry/common/attribute_value.h" | ||
#include "opentelemetry/core/timestamp.h" | ||
#include "opentelemetry/nostd/string_view.h" | ||
#include "opentelemetry/trace/canonical_code.h" | ||
|
@@ -33,6 +34,14 @@ class Recordable | |
opentelemetry::trace::SpanId span_id, | ||
opentelemetry::trace::SpanId parent_span_id) noexcept = 0; | ||
|
||
/** | ||
* Set an attribute of a span. | ||
* @param name the name of the attribute | ||
* @param value the attribute value | ||
*/ | ||
virtual void SetAttribute(nostd::string_view key, | ||
const opentelemetry::common::AttributeValue &&value) noexcept = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the wrong way to do move semantics. It should be either Move semantics require modifying value so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
/** | ||
* Add an event to a span. | ||
* @param name the name of the event | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
#pragma once | ||
|
||
#include <chrono> | ||
#include <unordered_map> | ||
#include "opentelemetry/core/timestamp.h" | ||
#include "opentelemetry/nostd/string_view.h" | ||
#include "opentelemetry/sdk/trace/recordable.h" | ||
|
@@ -67,6 +68,15 @@ class SpanData final : public Recordable | |
*/ | ||
std::chrono::nanoseconds GetDuration() const noexcept { return duration_; } | ||
|
||
/** | ||
* Get the attributes for this span | ||
* @return the attributes for this span | ||
*/ | ||
const std::unordered_map<std::string, common::AttributeValue> &GetAttributes() const noexcept | ||
{ | ||
return attributes_; | ||
} | ||
|
||
void SetIds(opentelemetry::trace::TraceId trace_id, | ||
opentelemetry::trace::SpanId span_id, | ||
opentelemetry::trace::SpanId parent_span_id) noexcept override | ||
|
@@ -76,6 +86,11 @@ class SpanData final : public Recordable | |
parent_span_id_ = parent_span_id; | ||
} | ||
|
||
void SetAttribute(nostd::string_view key, const common::AttributeValue &&value) noexcept override | ||
{ | ||
attributes_[std::string(key)] = value; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to confirm - do we intend to make this thread safe or the caller has to take care of it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More info:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, that answers it:
I'll add some locking around I'll also add some documentation to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can safely ignore my last comment, this is handled on the level of the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would happen if previous attribute already exists and is of a different type than the new attribute being set? SetAttribute("key", <string_view>) then SetAttribute("key", bool) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why can't we use nostd::string_view instead of std::string here for the map? Since we went with string_view in the first place, we were bothered to avoid the unnecessary memcpy. Transforming into string essentially invalidates the optimization done at top-level. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my comment above.
Optimized approaches are possible when using own There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes default SpanData implementation making assumption that the flow is asynchronous. Whereas there could be also synchronous exporter not needing a memcpy. Another approach is for Exporter (even if async) to perform serialization straight on customer data on customer thread. Thus not needing a copy. Can you clarify on what model should be followed for implementing a synchronous exporter? Does it mean that synchronous exporter devs must implement their own SpanData? I'm also worried about the unnecessary copies of events added using AddEvent. If we are to maintain a separate container for the Span attributes, how is it going to be done for Events added via AddEvent -- would SpanData provide yet another container for Events too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, that's an assumption the SpanData implementation is making.
They need not, but they surely can. I expect that the vast majority of real-world exporters will come together with custom |
||
} | ||
|
||
void AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) noexcept override | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do you plan on storing events on SpanData ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we'll need to come up with a proper |
||
{ | ||
(void)name; | ||
|
@@ -106,6 +121,7 @@ class SpanData final : public Recordable | |
std::string name_; | ||
opentelemetry::trace::CanonicalCode status_code_{opentelemetry::trace::CanonicalCode::OK}; | ||
std::string status_desc_; | ||
std::unordered_map<std::string, common::AttributeValue> attributes_; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this have to be a string? We can add order comparators to nostd::string_view implementation, then you do not need to construct std::string object to transform the nostd::string_view key into std::string... Standard Library has comparators on std::string_view , allowing it to be used as key in the maps. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is more of a lifetime issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do have the same issue with the rest of API surface. For example, AddEvent on span, we we use string_view for keys. My concern as follows:
or
string literal keys would permanently live in read-only .data , and copying them is wasteful. Should this be spelled out in some documentation, when / why the memcpy is necessary for the keys? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For those cases implementing a custom
True. But on exporter side, it would be hard to determine which of the strings received live in read-only .data and which are allocated on the heap/stack. It also would be complicated to come up with an optimization that accounts for that. I'm hesitant to make |
||
}; | ||
} // namespace trace | ||
} // namespace 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.
This method is mixing std and nostd classes. Assuming that this is implementation is NOT ABI compatible across different compilers having std::shared_ptr for return value and std::string for error message on signature, is there a practical reason why we require the tracer_config be of nostd type?
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.
Our ABI policy says this:
Factory
is not an abstract class andMakeTracer
is not a virtual method, so there's no need fornostd
types.I think the reason for
tracer_config
being astring_view
is that it makes it more efficient to pass instd::string
(without need to copy) andconst char*
(without need to create astd::string
). But it's not an ABI requirement in this case.