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

Support for attributes on spans #117

Merged
merged 18 commits into from
Jun 23, 2020
9 changes: 5 additions & 4 deletions api/include/opentelemetry/plugin/factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

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?

Copy link
Contributor Author

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.

std::string &error_message) const
noexcept
{
nostd::unique_ptr<char[]> plugin_error_message;
auto tracer_handle = factory_impl_->MakeTracerHandle(tracer_config, plugin_error_message);
Expand All @@ -46,8 +47,8 @@ class Factory final
detail::CopyErrorMessage(plugin_error_message.get(), error_message);
return nullptr;
}
return std::shared_ptr<Tracer>{new (std::nothrow)
Tracer{library_handle_, std::move(tracer_handle)}};
return std::shared_ptr<opentelemetry::trace::Tracer>{
new (std::nothrow) Tracer{library_handle_, std::move(tracer_handle)}};
}

private:
Expand Down
8 changes: 7 additions & 1 deletion api/include/opentelemetry/plugin/tracer.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ class Span final : public trace::Span
{}

// trace::Span
void SetAttribute(nostd::string_view name, const common::AttributeValue &&value) noexcept override
{
span_->SetAttribute(name, std::move(value));
}

void AddEvent(nostd::string_view name) noexcept override { span_->AddEvent(name); }

void AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) noexcept override
Expand Down Expand Up @@ -61,9 +66,10 @@ class Tracer final : public trace::Tracer, public std::enable_shared_from_this<T
// trace::Tracer
nostd::unique_ptr<trace::Span> StartSpan(
nostd::string_view name,
const trace::KeyValueIterable &attributes,
const trace::StartSpanOptions &options = {}) noexcept override
{
auto span = tracer_handle_->tracer().StartSpan(name, options);
auto span = tracer_handle_->tracer().StartSpan(name, attributes, options);
if (span == nullptr)
{
return nullptr;
Expand Down
5 changes: 5 additions & 0 deletions api/include/opentelemetry/trace/noop.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ class NoopSpan final : public Span
public:
explicit NoopSpan(const std::shared_ptr<Tracer> &tracer) noexcept : tracer_{tracer} {}

void SetAttribute(nostd::string_view /*key*/,
const common::AttributeValue && /*value*/) noexcept override
{}

void AddEvent(nostd::string_view /*name*/) noexcept override {}

void AddEvent(nostd::string_view /*name*/, core::SystemTimestamp /*timestamp*/) noexcept override
Expand Down Expand Up @@ -56,6 +60,7 @@ class NoopTracer final : public Tracer, public std::enable_shared_from_this<Noop
public:
// Tracer
nostd::unique_ptr<Span> StartSpan(nostd::string_view /*name*/,
const KeyValueIterable & /*attributes*/,
const StartSpanOptions & /*options*/) noexcept override
{
return nostd::unique_ptr<Span>{new (std::nothrow) NoopSpan{this->shared_from_this()}};
Expand Down
9 changes: 3 additions & 6 deletions api/include/opentelemetry/trace/span.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include <cstdint>

#include "opentelemetry/common/attribute_value.h"
#include "opentelemetry/core/timestamp.h"
#include "opentelemetry/nostd/span.h"
#include "opentelemetry/nostd/string_view.h"
Expand Down Expand Up @@ -40,7 +41,6 @@ struct StartSpanOptions
// Span(Context?) parent;
// SpanContext remote_parent;
// Links
// Attributes
SpanKind kind = SpanKind::kInternal;
};
/**
Expand Down Expand Up @@ -74,13 +74,10 @@ class Span
Span &operator=(const Span &) = delete;
Span &operator=(Span &&) = delete;

// TODO
// Sets an attribute on the Span. If the Span previously contained a mapping for
// the key, the old value is replaced.
//
// If an empty string is used as the value, the attribute will be silently
g-easy marked this conversation as resolved.
Show resolved Hide resolved
// dropped. Note: this behavior could change in the future.
// virtual void SetAttribute(nostd::string_view key, AttributeValue&& value) = 0;
virtual void SetAttribute(nostd::string_view key,
const common::AttributeValue &&value) noexcept = 0;

// Adds an event to the Span.
virtual void AddEvent(nostd::string_view name) noexcept = 0;
Expand Down
31 changes: 31 additions & 0 deletions api/include/opentelemetry/trace/tracer.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,41 @@ class Tracer
virtual ~Tracer() = default;
/**
* Starts a span.
*
* Optionally sets attributes at Span creation from the given key/value pairs.
*
* Attributes will be processed in order, previous attributes with the same
* key will be overwritten.
*/
virtual nostd::unique_ptr<Span> StartSpan(nostd::string_view name,
const KeyValueIterable &attributes,
const StartSpanOptions &options = {}) noexcept = 0;

nostd::unique_ptr<Span> StartSpan(nostd::string_view name,
const StartSpanOptions &options = {}) noexcept
{
return this->StartSpan(name, {}, options);
}

template <class T, nostd::enable_if_t<detail::is_key_value_iterable<T>::value> * = nullptr>
nostd::unique_ptr<Span> StartSpan(nostd::string_view name,
const T &attributes,
const StartSpanOptions &options = {}) noexcept
{
return this->StartSpan(name, KeyValueIterableView<T>(attributes), options);
}

nostd::unique_ptr<Span> StartSpan(
nostd::string_view name,
std::initializer_list<std::pair<nostd::string_view, common::AttributeValue>> attributes,
const StartSpanOptions &options = {}) noexcept
{
return this->StartSpan(name,
nostd::span<const std::pair<nostd::string_view, common::AttributeValue>>{
attributes.begin(), attributes.end()},
options);
}

/**
* Force any buffered spans to flush.
* @param timeout to complete the flush
Expand Down
20 changes: 14 additions & 6 deletions examples/plugin/plugin/tracer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@

#include <iostream>

namespace nostd = opentelemetry::nostd;
namespace core = opentelemetry::core;
namespace trace = opentelemetry::trace;
namespace nostd = opentelemetry::nostd;
namespace common = opentelemetry::common;
namespace core = opentelemetry::core;
namespace trace = opentelemetry::trace;

namespace
{
Expand All @@ -13,6 +14,7 @@ class Span final : public trace::Span
public:
Span(std::shared_ptr<Tracer> &&tracer,
nostd::string_view name,
const opentelemetry::trace::KeyValueIterable & /*attributes*/,
const trace::StartSpanOptions & /*options*/) noexcept
: tracer_{std::move(tracer)}, name_{name}
{
Expand All @@ -22,6 +24,10 @@ class Span final : public trace::Span
~Span() { std::cout << "~Span\n"; }

// opentelemetry::trace::Span
void SetAttribute(nostd::string_view /*name*/,
const common::AttributeValue && /*value*/) noexcept override
{}

void AddEvent(nostd::string_view /*name*/) noexcept override {}

void AddEvent(nostd::string_view /*name*/, core::SystemTimestamp /*timestamp*/) noexcept override
Expand Down Expand Up @@ -52,9 +58,11 @@ class Span final : public trace::Span

Tracer::Tracer(nostd::string_view /*output*/) {}

nostd::unique_ptr<trace::Span> Tracer::StartSpan(nostd::string_view name,
const trace::StartSpanOptions &options) noexcept
nostd::unique_ptr<trace::Span> Tracer::StartSpan(
nostd::string_view name,
const opentelemetry::trace::KeyValueIterable &attributes,
const trace::StartSpanOptions &options) noexcept
{
return nostd::unique_ptr<opentelemetry::trace::Span>{
new (std::nothrow) Span{this->shared_from_this(), name, options}};
new (std::nothrow) Span{this->shared_from_this(), name, attributes, options}};
}
3 changes: 2 additions & 1 deletion examples/plugin/plugin/tracer.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ class Tracer final : public opentelemetry::trace::Tracer,
// opentelemetry::trace::Tracer
opentelemetry::nostd::unique_ptr<opentelemetry::trace::Span> StartSpan(
opentelemetry::nostd::string_view name,
const opentelemetry::trace::StartSpanOptions &options) noexcept override;
const opentelemetry::trace::KeyValueIterable & /*attributes*/,
const opentelemetry::trace::StartSpanOptions & /*options */) noexcept override;

void ForceFlushWithMicroseconds(uint64_t /*timeout*/) noexcept override {}

Expand Down
9 changes: 9 additions & 0 deletions sdk/include/opentelemetry/sdk/trace/recordable.h
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"
Expand Down Expand Up @@ -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;
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


/**
* Add an event to a span.
* @param name the name of the event
Expand Down
16 changes: 16 additions & 0 deletions sdk/include/opentelemetry/sdk/trace/span_data.h
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"
Expand Down Expand Up @@ -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
Expand All @@ -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;
Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@reyang reyang Jun 16, 2020

Choose a reason for hiding this comment

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

More info:

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

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

Copy link
Contributor

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)

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@maxgolov maxgolov Jun 22, 2020

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?

Copy link
Contributor Author

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.

}

void AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) noexcept override
Copy link
Contributor

@maxgolov maxgolov Jun 22, 2020

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 ?

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

{
(void)name;
Expand Down Expand Up @@ -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_;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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

};
} // namespace trace
} // namespace sdk
Expand Down
1 change: 1 addition & 0 deletions sdk/include/opentelemetry/sdk/trace/tracer.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class Tracer final : public trace_api::Tracer, public std::enable_shared_from_th

nostd::unique_ptr<trace_api::Span> StartSpan(
nostd::string_view name,
const trace_api::KeyValueIterable &attributes,
const trace_api::StartSpanOptions &options = {}) noexcept override;

void ForceFlushWithMicroseconds(uint64_t timeout) noexcept override;
Expand Down
13 changes: 13 additions & 0 deletions sdk/src/trace/span.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ SteadyTimestamp NowOr(const SteadyTimestamp &steady)
Span::Span(std::shared_ptr<Tracer> &&tracer,
std::shared_ptr<SpanProcessor> processor,
nostd::string_view name,
const trace_api::KeyValueIterable &attributes,
const trace_api::StartSpanOptions &options) noexcept
: tracer_{std::move(tracer)},
processor_{processor},
Expand All @@ -55,6 +56,11 @@ Span::Span(std::shared_ptr<Tracer> &&tracer,
processor_->OnStart(*recordable_);
recordable_->SetName(name);

attributes.ForEachKeyValue([&](nostd::string_view key, common::AttributeValue value) noexcept {
recordable_->SetAttribute(key, std::move(value));
return true;
});

recordable_->SetStartTime(NowOr(options.start_system_time));
start_steady_time = NowOr(options.start_steady_time);
}
Expand All @@ -64,6 +70,13 @@ Span::~Span()
End();
}

void Span::SetAttribute(nostd::string_view key, const common::AttributeValue &&value) noexcept
{
std::lock_guard<std::mutex> lock_guard{mu_};

recordable_->SetAttribute(key, std::move(value));
}

void Span::AddEvent(nostd::string_view name) noexcept
{
(void)name;
Expand Down
3 changes: 3 additions & 0 deletions sdk/src/trace/span.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,14 @@ class Span final : public trace_api::Span
explicit Span(std::shared_ptr<Tracer> &&tracer,
std::shared_ptr<SpanProcessor> processor,
nostd::string_view name,
const trace_api::KeyValueIterable &attributes,
const trace_api::StartSpanOptions &options) noexcept;

~Span() override;

// trace_api::Span
void SetAttribute(nostd::string_view key, const common::AttributeValue &&value) noexcept override;

void AddEvent(nostd::string_view name) noexcept override;

void AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) noexcept override;
Expand Down
5 changes: 3 additions & 2 deletions sdk/src/trace/tracer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@ std::shared_ptr<SpanProcessor> Tracer::GetProcessor() const noexcept

nostd::unique_ptr<trace_api::Span> Tracer::StartSpan(
nostd::string_view name,
const trace_api::KeyValueIterable &attributes,
const trace_api::StartSpanOptions &options) noexcept
{
return nostd::unique_ptr<trace_api::Span>{
new (std::nothrow) Span{this->shared_from_this(), processor_.load(), name, options}};
return nostd::unique_ptr<trace_api::Span>{new (std::nothrow) Span{
this->shared_from_this(), processor_.load(), name, attributes, options}};
}

void Tracer::ForceFlushWithMicroseconds(uint64_t timeout) noexcept
Expand Down
4 changes: 4 additions & 0 deletions sdk/test/trace/span_data_test.cc
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "opentelemetry/sdk/trace/span_data.h"
#include "opentelemetry/nostd/variant.h"
#include "opentelemetry/trace/span_id.h"
#include "opentelemetry/trace/trace_id.h"

Expand All @@ -20,6 +21,7 @@ TEST(SpanData, DefaultValues)
ASSERT_EQ(data.GetDescription(), "");
ASSERT_EQ(data.GetStartTime().time_since_epoch(), std::chrono::nanoseconds(0));
ASSERT_EQ(data.GetDuration(), std::chrono::nanoseconds(0));
ASSERT_EQ(data.GetAttributes().size(), 0);
}

TEST(SpanData, Set)
Expand All @@ -35,6 +37,7 @@ TEST(SpanData, Set)
data.SetStatus(opentelemetry::trace::CanonicalCode::UNKNOWN, "description");
data.SetStartTime(now);
data.SetDuration(std::chrono::nanoseconds(1000000));
data.SetAttribute("attr1", 314159);
data.AddEvent("event1", now);

ASSERT_EQ(data.GetTraceId(), trace_id);
Expand All @@ -45,4 +48,5 @@ TEST(SpanData, Set)
ASSERT_EQ(data.GetDescription(), "description");
ASSERT_EQ(data.GetStartTime().time_since_epoch(), now.time_since_epoch());
ASSERT_EQ(data.GetDuration(), std::chrono::nanoseconds(1000000));
ASSERT_EQ(opentelemetry::nostd::get<int>(data.GetAttributes().at("attr1")), 314159);
}
Loading