-
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
Conversation
e7e2f44
to
c41715d
Compare
@@ -36,11 +37,13 @@ struct StartSpanOptions | |||
core::SystemTimestamp start_system_time; | |||
core::SteadyTimestamp start_steady_time; | |||
|
|||
// Optionally set attributes at Span creation. | |||
nostd::span<std::pair<nostd::string_view, common::AttributeValue>> attributes; |
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 we de-dupe the names? If yes, might need to clarify, it could be one of the following:
- the last one will win (overwrite the previous ones).
- the span creation would fail (end up having invalid span).
- the first one will win (subsequent attributes with the same name will be ignored).
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 went with (1), which in my reading is most in line with the spec:
Setting an attribute with the same key as an existing attribute SHOULD overwrite the existing attribute's value.
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.
Another note on this: it is my understanding that std::pair
is ABI stable, but please anybody correct me if I'm wrong with that.
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 don't know if pair is stable across different STLs, but adding a struct for { key, value } might even increase readability.
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 added a AttributeKeyValue
, which indeed makes this much better.
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.
After re-evaluating this over the weekend, I switched to using a KeyValueIterable
as function argument, as we do for AddEvent
. This should take care of any lifetime issues.
On the flipside, we might end up with some more StartSpan
signatures when we finally support links.
@@ -76,6 +86,11 @@ class SpanData final : public Recordable | |||
parent_span_id_ = parent_span_id; | |||
} | |||
|
|||
void SetAttribute(nostd::string_view key, common::AttributeValue &&value) noexcept override | |||
{ | |||
attributes_[std::string(key)] = value; |
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.
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
More info:
- In Python (to be specific, CPython) this is inherently thread safe.
- In C# this is done by CAS (compare and swap) (note:
Activity.AddTag
provides the same semantic asSpan.SetAttribute
. Thanks @cijothomas for the hint.
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.
Well, that answers it:
Span - All methods of Span are safe to be called concurrently.
I'll add some locking around attributes_
and I will submit another PR making all other SpanData
operations atomic.
I'll also add some documentation to Recordable
, as this is something everybody implementing their own Recordable
has to take into consideration.
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.
You can safely ignore my last comment, this is handled on the level of the Span
in the SDK.
Recordable
s themselves need not be thread safe, Span
ensures appropriate locking around access to the 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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
The string_view
variant would be overwritten by a bool
variant. A similar case is covered by this unit test.
// | ||
// Attributes will be processed in order, previous attributes with the same | ||
// key will be overwritten. | ||
nostd::span<std::pair<nostd::string_view, common::AttributeValue>> attributes; |
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.
nostd::span is a non-owning reference. (as is string_view). There are non-trivial lifetime issues putting this into StartSpanOptions. I'm not sure what to do here.
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'll think more about his.
One solution would be to make this an argument to StartSpan
(in addition to options). Then there'd be no lifetime issues of storing this in a struct.
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 went with the attributes as an additional argument to StartSpan
. This should fix any lifetime issues.
@@ -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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
This is more of a lifetime issue. string_view
is a non-owning reference, so the value pointed to by a string_view
might be invalidated (freed) while we hold on to it.
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.
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:
-
for flows that require synchronous exporter, such as Windows ETW, or anything that involves "local" high-speed RPC channel, e.g. fluentd - we do not necessarily need to copy the key.
-
also for async flows: if the key is a string view of a constant character literal, such as (pseudocode):
SetAttribute("MyKey1", ...);
SetAttribute("MyKey2", ...);
or
M m2 = {{"key1", "one"}, {"key2", "two"}};
span->AddEvent("MyProduct.MyEvent2", m2);
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 comment
The reason will be displayed to describe this comment to others. Learn more.
- for flows that require synchronous exporter, such as Windows ETW, or anything that involves "local" high-speed RPC channel, e.g. fluentd - we do not necessarily need to copy the key.
For those cases implementing a custom Recordable
makes sense. The exporter implementor has full control over what data is copied, the SDK makes no copies whatsoever.
string literal keys would permanently live in read-only .data , and copying them is wasteful.
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 SpanData
a highly optimized part of the SDK, as vendors with high performance requirements have the possibility to come up with Recordable
s tailored to their needs. With the Recordable
approach, we don't need to come up with one SpanData
that fits all needs (which is impossible anyway).
@@ -76,6 +86,11 @@ class SpanData final : public Recordable | |||
parent_span_id_ = parent_span_id; | |||
} | |||
|
|||
void SetAttribute(nostd::string_view key, common::AttributeValue &&value) noexcept override | |||
{ | |||
attributes_[std::string(key)] = value; |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above.
Transforming into string essentially invalidates the optimization done at top-level.
Optimized approaches are possible when using own Recordable
implementations, where there is no need for an intermediate copy is . SpanData
needs to hold internal copies of values due to lifetime issues.
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 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 comment
The reason will be displayed to describe this comment to others. Learn more.
This makes default SpanData implementation making assumption that the flow is asynchronous.
Yes, that's an assumption the SpanData implementation is making.
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?
They need not, but they surely can. I expect that the vast majority of real-world exporters will come together with custom Recordable
implementations. OTLP and GCP exporters will.
Codecov Report
@@ Coverage Diff @@
## master #117 +/- ##
==========================================
- Coverage 93.60% 93.29% -0.31%
==========================================
Files 66 66
Lines 1564 1656 +92
==========================================
+ Hits 1464 1545 +81
- Misses 100 111 +11
|
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.
@@ -36,8 +36,9 @@ class Factory final | |||
* @param error_message on failure this will contain an error message. | |||
* @return a Tracer on success or nullptr on failure. | |||
*/ | |||
std::shared_ptr<Tracer> MakeTracer(nostd::string_view tracer_config, | |||
std::string &error_message) const noexcept | |||
std::shared_ptr<opentelemetry::trace::Tracer> MakeTracer(nostd::string_view tracer_config, |
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:
Abstract classes in the OpenTelemetry API must use ABI stable types in their virtual function signatures.
Factory
is not an abstract class and MakeTracer
is not a virtual method, so there's no need for nostd
types.
I think the reason for tracer_config
being a string_view
is that it makes it more efficient to pass in std::string
(without need to copy) and const char*
(without need to create a std::string
). But it's not an ABI requirement in this case.
{ | ||
attributes_[std::string(key)] = value; | ||
} | ||
|
||
void AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) noexcept override |
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.
How do you plan on storing events on SpanData ?
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 we'll need to come up with a proper struct
type encapsulating an event, but I didn't spend much time thinking about this yet.
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'll log a separate issue on a need to avoid a memcpy for Attributes and Events for high-perf synchronous exporters.
Co-authored-by: Reiley Yang <reyang@microsoft.com>
3a2e8a9
to
8723b37
Compare
This is rebased, but due to rebasing fails some code coverage criteria. I'm not a fan of rigid code coverage requirements, but I'm willing to conform if others think that's necessary. |
Probably not an immediate goal at this moment. Let's merge it. |
* @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 comment
The 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 const opentelemetry::common::AttributeValue &value
(i.e. doesn't allow move semantics) or opentelemetry::common::AttributeValue && value
(supports move semantics).
Move semantics require modifying value so const opentelemetry::common::AttributeValue &&
makes no sense.
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 PR adds support for setting attributes on spans, refer to this section of the spec for further details. It extends the
Span
interface, theRecordable
interface as well as the defaultSpanData
implementation.It also adds support for adding attributes as part of the
StartSpanOptions
.