Support for attributes on spans #117

pyohannes · 2020-06-16T21:53:19Z

This PR adds support for setting attributes on spans, refer to this section of the spec for further details. It extends the Span interface, the Recordable interface as well as the default SpanData implementation.

It also adds support for adding attributes as part of the StartSpanOptions.

reyang · 2020-06-16T22:03:50Z

api/include/opentelemetry/trace/span.h

@@ -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;


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

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.

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.

I don't know if pair is stable across different STLs, but adding a struct for { key, value } might even increase readability.

I added a AttributeKeyValue, which indeed makes this much better.

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.

sdk/include/opentelemetry/sdk/trace/recordable.h

reyang · 2020-06-16T22:10:02Z

sdk/include/opentelemetry/sdk/trace/span_data.h

@@ -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;


Just to confirm - do we intend to make this thread safe or the caller has to take care of it?

FYI - https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/concurrency.md

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 as Span.SetAttribute. Thanks @cijothomas for the hint.

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.

You can safely ignore my last comment, this is handled on the level of the Span in the SDK.

Recordables themselves need not be thread safe, Span ensures appropriate locking around access to the Recordable.

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)

The string_view variant would be overwritten by a bool variant. A similar case is covered by this unit test.

api/include/opentelemetry/trace/span.h

g-easy · 2020-06-18T06:15:48Z

api/include/opentelemetry/trace/span.h

+  //
+  // 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;


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.

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.

I went with the attributes as an additional argument to StartSpan. This should fix any lifetime issues.

api/include/opentelemetry/trace/span.h

sdk/src/trace/span.cc

sdk/test/trace/tracer_test.cc

maxgolov · 2020-06-19T20:03:53Z

sdk/include/opentelemetry/sdk/trace/span_data.h

@@ -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_;


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.

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.

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?

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

maxgolov · 2020-06-19T20:08:32Z

sdk/include/opentelemetry/sdk/trace/span_data.h

@@ -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;


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.

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.

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?

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 · 2020-06-20T04:10:50Z

Codecov Report

Merging #117 into master will decrease coverage by 0.30%.
The diff coverage is 86.95%.

@@            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

Impacted Files	Coverage Δ
api/include/opentelemetry/plugin/factory.h	`0.00% <ø> (ø)`
api/include/opentelemetry/trace/noop.h	`50.00% <0.00%> (-4.17%)`	⬇️
api/include/opentelemetry/trace/span.h	`100.00% <ø> (ø)`
sdk/include/opentelemetry/sdk/trace/recordable.h	`100.00% <ø> (ø)`
sdk/include/opentelemetry/sdk/trace/tracer.h	`100.00% <ø> (ø)`
sdk/src/trace/span.h	`0.00% <ø> (ø)`
sdk/src/trace/span.cc	`58.62% <50.00%> (-1.38%)`	⬇️
api/include/opentelemetry/trace/tracer.h	`100.00% <100.00%> (ø)`
sdk/include/opentelemetry/sdk/trace/span_data.h	`100.00% <100.00%> (ø)`
sdk/src/trace/tracer.cc	`41.66% <100.00%> (+5.30%)`	⬆️
... and 4 more

reyang

LGTM.

maxgolov · 2020-06-22T19:56:52Z

api/include/opentelemetry/plugin/factory.h

@@ -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,


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?

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.

maxgolov · 2020-06-22T19:59:35Z

sdk/include/opentelemetry/sdk/trace/span_data.h

+  {
+    attributes_[std::string(key)] = value;
+  }
+
  void AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) noexcept override


How do you plan on storing events on SpanData ?

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.

maxgolov

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>

pyohannes · 2020-06-23T16:30:49Z

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.

reyang · 2020-06-23T16:40:03Z

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.

rnburn · 2020-06-30T19:55:43Z

sdk/include/opentelemetry/sdk/trace/recordable.h

+   * @param value the attribute value
+   */
+  virtual void SetAttribute(nostd::string_view key,
+                            const opentelemetry::common::AttributeValue &&value) noexcept = 0;


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.

Thanks for chiming in @rnburn. I submitted PR #141 to fix this.

pyohannes requested a review from a team June 16, 2020 21:53

pyohannes force-pushed the span-attributes branch from e7e2f44 to c41715d Compare June 16, 2020 21:54

reyang reviewed Jun 16, 2020

View reviewed changes

sdk/include/opentelemetry/sdk/trace/recordable.h Outdated Show resolved Hide resolved

reyang reviewed Jun 16, 2020

View reviewed changes

api/include/opentelemetry/trace/span.h Outdated Show resolved Hide resolved

g-easy reviewed Jun 18, 2020

View reviewed changes

ziqizh mentioned this pull request Jun 19, 2020

Add sampler header file #118

Merged

maxgolov reviewed Jun 19, 2020

View reviewed changes

ziqizh mentioned this pull request Jun 22, 2020

Always on sampler #122

Merged

reyang approved these changes Jun 22, 2020

View reviewed changes

maxgolov reviewed Jun 22, 2020

View reviewed changes

maxgolov approved these changes Jun 22, 2020

View reviewed changes

Johannes Tax and others added 14 commits June 23, 2020 07:42

Add SetAttribute method

39e26d2

Add to span data

f9d8c7e

Add SDK support for attributes

3de6cbf

Add tests

c300da6

Set attributes on span start

f07d893

Fix documentation

08d24a2

Add test for overwriting attributes

d684dc6

Update api/include/opentelemetry/trace/span.h

f4d6661

Co-authored-by: Reiley Yang <reyang@microsoft.com>

Add lock for setting attributes

852be31

Use a struct for attribute key/value pairs

81b1f03

PR comments

9149aa1

Attribute list

da1c599

Const

bfc7ae0

Change to KeyValueIterable

0b952af

Johannes Tax added 4 commits June 23, 2020 07:42

Format

d0a25f2

Format

21ef99f

Format

dd8e1b0

Remove unused struct

8723b37

pyohannes force-pushed the span-attributes branch from 3a2e8a9 to 8723b37 Compare June 23, 2020 14:42

reyang merged commit 9b624b9 into open-telemetry:master Jun 23, 2020

rnburn reviewed Jun 30, 2020

View reviewed changes

pyohannes mentioned this pull request Jun 30, 2020

Don't move a constant AttributeValue #141

Merged

snehilchopra mentioned this pull request Jun 30, 2020

Added Custom Recordable Implementation and Unit Tests GoogleCloudPlatform/opentelemetry-operations-cpp#2

Merged

Support for attributes on spans #117

Support for attributes on spans #117

Conversation

pyohannes commented Jun 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyang Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxgolov Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 20, 2020 • edited Loading

Codecov Report

reyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxgolov Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxgolov left a comment

Choose a reason for hiding this comment

pyohannes commented Jun 23, 2020 • edited Loading

reyang commented Jun 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyang Jun 16, 2020 •

edited

Loading

maxgolov Jun 22, 2020 •

edited

Loading

codecov bot commented Jun 20, 2020 •

edited

Loading

maxgolov Jun 22, 2020 •

edited

Loading

pyohannes commented Jun 23, 2020 •

edited

Loading