Skip to content

Link api - to add links to span. #342

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.

Sign up for GitHub

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

Closed
lalitb wants to merge 8 commits into from
Closed

Link api - to add links to span. #342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8dd7be1
first draft
lalitb Sep 28, 2020
375322f
fix format
lalitb Sep 28, 2020
0513e54
fix trace_api
lalitb Sep 28, 2020
6597099
fix coverage
lalitb Sep 28, 2020
7ce23b9
add test
lalitb Sep 28, 2020
fc5b652
review comment
lalitb Sep 29, 2020
92be882
more tests
lalitb Sep 29, 2020
dce3656
add comment for span creation
lalitb Sep 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions api/include/opentelemetry/plugin/tracer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "opentelemetry/plugin/detail/dynamic_library_handle.h"
#include "opentelemetry/plugin/detail/tracer_handle.h"
#include "opentelemetry/trace/link.h"
#include "opentelemetry/trace/tracer.h"
#include "opentelemetry/version.h"

Expand Down Expand Up @@ -37,6 +38,18 @@ class Span final : public trace::Span
span_->AddEvent(name, timestamp, attributes);
}

void AddLink(const trace::Link &link) noexcept override { span_->AddLink(link); }
void AddLink(const trace::SpanContext &span_context,
const trace::KeyValueIterable &attributes) noexcept override
{
span_->AddLink(span_context, attributes);
}

void AddLink(const trace::SpanContext &span_context) noexcept override
{
span_->AddLink(span_context);
}

void SetStatus(trace::CanonicalCode code, nostd::string_view description) noexcept override
{
span_->SetStatus(code, description);
Expand Down
8 changes: 8 additions & 0 deletions api/include/opentelemetry/trace/default_span.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,14 @@ class DefaultSpan : public Span
this->AddEvent(name, std::chrono::system_clock::now(), attributes);
}

void AddLink(const trace::Link &link) noexcept override {}

void AddLink(const trace::SpanContext &span_context,
const trace::KeyValueIterable &attributes) noexcept override
{}

void AddLink(const trace::SpanContext &span_context) noexcept override {}

void SetStatus(CanonicalCode status, nostd::string_view description) noexcept {}

void UpdateName(nostd::string_view name) noexcept {}
Expand Down
20 changes: 20 additions & 0 deletions api/include/opentelemetry/trace/link.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#pragma once

#include "opentelemetry/trace/key_value_iterable.h"
#include "opentelemetry/trace/span_context.h"
#include "opentelemetry/version.h"

OPENTELEMETRY_BEGIN_NAMESPACE
namespace trace
{
class Link
{
public:
// returns the Span Context associated with this Link
virtual const SpanContext &GetContext() const = 0;

// returns the attributes associated with link
virtual const KeyValueIterable &GetAttributes() const = 0;
};
} // namespace trace
OPENTELEMETRY_END_NAMESPACE
8 changes: 8 additions & 0 deletions api/include/opentelemetry/trace/noop.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,14 @@ class NoopSpan final : public Span
const trace::KeyValueIterable & /*attributes*/) noexcept override
{}

void AddLink(const Link &link) noexcept override {}

void AddLink(const trace_api::SpanContext &spanContext,
const trace_api::KeyValueIterable &attributes) noexcept override
{}

void AddLink(const trace_api::SpanContext &span_context) noexcept override {}

void SetStatus(CanonicalCode /*code*/, nostd::string_view /*description*/) noexcept override {}

void UpdateName(nostd::string_view /*name*/) noexcept override {}
Expand Down
32 changes: 32 additions & 0 deletions api/include/opentelemetry/trace/span.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "opentelemetry/nostd/unique_ptr.h"
#include "opentelemetry/trace/canonical_code.h"
#include "opentelemetry/trace/key_value_iterable_view.h"
#include "opentelemetry/trace/link.h"
#include "opentelemetry/trace/span_context.h"
#include "opentelemetry/version.h"

Expand Down Expand Up @@ -135,6 +136,37 @@ class Span
attributes.begin(), attributes.end()});
}

/** Adds a Link to newly created Span
* @param spanContext the context of the linked span
* @param attributes Link attributes
*
* These methods need to be called immediately after Span
* creation. Specification doesn't allow adding links after
* span creation.
*/

virtual void AddLink(const trace::Link &link) noexcept = 0;
Copy link
Copy Markdown
Contributor

@pyohannes pyohannes Sep 28, 2020

Choose a reason for hiding this comment

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

Could we go without the dedicated trace::Link type altogether? We don't have a type for events, so leaving this out for links would make things more symmetrical.

Copy link
Copy Markdown
Member Author

@lalitb lalitb Sep 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Just thinking about scenarios which makes it different from Events, and having trace::Link would be helpful.

  • Creating a link, and then associating it with multiple spans. It would be helpful to have link as an entity in that case.
  • Link should be associated with the Span during it's creation, and not afterwards. As per the specification
    "Links cannot be added after Span creation." With link type, it's helpful to pass sequence of links in Span constructor. Currently we do provide Span::AddLink() but it needs to be called immediately after the span creation ( probably this needs to be clearly documented).

Copy link
Copy Markdown
Contributor

@pyohannes pyohannes Sep 29, 2020

Choose a reason for hiding this comment

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

Links cannot be added after Span creation.

I see. What do you think about completely dismissing AddLink methods, and just adding links via StartSpanOptions? This would enforce the spec requirement, I think this is a better and less ambiguous way to add links.

trace::Link as it is now is not useful. An API user couldn't even construct a trace::Link, as it's a pure interface.

Copy link
Copy Markdown
Member Author

@lalitb lalitb Sep 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@pyohannes - I agree on dismissing AddLink methods. But sending links via StartSpanOptions won't be feasible, as we need to initialize collection of links. Using std::vector/std::array may break the ABI. Better option would be to have as separate argument in StartSpan :

 nostd::shared_ptr<Span> StartSpan(nostd::string_view /*name*/,
                                    const KeyValueIterable & /*attributes*/,
                                    const StartSpanOptions & /*options*/,
                                    const nostd::span<nostd::shared_ptr<Link>>& /*links*/) noexcept override

I don't have strong opinion in having dedicated Link type, but since link contains attribute pairs , and so the overloading for KeyValueIterable can be done within Link type, instead of adding more overloaded StartSpan methods. Let me know what you think ?

Copy link
Copy Markdown
Contributor

@pyohannes pyohannes Oct 1, 2020

Choose a reason for hiding this comment

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

I like the suggestion of having this as additional argument to the StartSpan function. I'm not yet sure about the best way to realize this, but let's work towards that!

Copy link
Copy Markdown
Member

@reyang reyang Oct 1, 2020

Choose a reason for hiding this comment

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

FYI this is what OpenTelemetry Python has https://github.com/open-telemetry/opentelemetry-python/blob/982b667ab7d32de45ed3b9cd6981f37a9e04975b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py#L701

Copy link
Copy Markdown
Member Author

@lalitb lalitb Oct 5, 2020

Choose a reason for hiding this comment

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

@pyohannes - I have done the changes as separate PR #351 , as this was mostly a rework. Please review that PR. I am closing current PR.


virtual void AddLink(const trace::SpanContext &span_context,
const trace::KeyValueIterable &attributes) noexcept = 0;
Comment thread
lalitb marked this conversation as resolved.

virtual void AddLink(const trace::SpanContext &span_context) noexcept = 0;

template <class T, nostd::enable_if_t<detail::is_key_value_iterable<T>::value> * = nullptr>
void AddLink(const trace::SpanContext &span_context, const T &attributes) noexcept
{
this->AddLink(span_context, KeyValueIterableView<T>{attributes});
}

void AddLink(const trace::SpanContext &span_context,
std::initializer_list<std::pair<nostd::string_view, common::AttributeValue>>
attributes) noexcept
{
this->AddLink(span_context,
nostd::span<const std::pair<nostd::string_view, common::AttributeValue>>{
attributes.begin(), attributes.end()});
}

// Sets the status of the span. The default status is OK. Only the value of
// the last call will be
// recorded, and implementations are free to ignore previous calls.
Expand Down
9 changes: 9 additions & 0 deletions api/test/trace/noop_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "opentelemetry/trace/noop.h"
#include "opentelemetry/core/timestamp.h"
#include "opentelemetry/trace/key_value_iterable_view.h"

#include <map>
#include <memory>
Expand Down Expand Up @@ -31,6 +32,14 @@ TEST(NoopTest, UseNoopTracers)
s1->SetAttribute("abc", 4);

s1->AddEvent("abc"); // add Empty
SpanContext sp(false, false);
s1->AddLink(sp);
using M = std::map<std::string, std::string>;
M m1 = {{"abc", "123"}, {"xyz", "456"}};
opentelemetry::trace::KeyValueIterableView<M> iterable{m1};

s1->AddLink(sp, iterable);
Comment thread
lalitb marked this conversation as resolved.
s1->AddLink(sp, {{"abc", "123"}, {"xyz", "456"}});

EXPECT_EQ(s1->IsRecording(), false);

Expand Down
8 changes: 8 additions & 0 deletions examples/plugin/plugin/tracer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,14 @@ class Span final : public trace::Span
const trace::KeyValueIterable & /*attributes*/) noexcept override
{}

void AddLink(const trace::Link &link) noexcept override {}

void AddLink(const trace::SpanContext &span_context,
const trace::KeyValueIterable &attributes) noexcept override
{}

void AddLink(const trace::SpanContext &span_context) noexcept override {}

void SetStatus(trace::CanonicalCode /*code*/,
nostd::string_view /*description*/) noexcept override
{}
Expand Down
2 changes: 1 addition & 1 deletion ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ class ThreadsafeSpanData final : public opentelemetry::sdk::trace::Recordable
}

void AddLink(
opentelemetry::trace::SpanContext span_context,
const opentelemetry::trace::SpanContext &span_context,
const trace_api::KeyValueIterable &attributes =
trace_api::KeyValueIterableView<std::map<std::string, int>>({})) noexcept override
{
Expand Down
47 changes: 47 additions & 0 deletions sdk/include/opentelemetry/sdk/trace/link.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
#pragma once

#include "opentelemetry/sdk/trace/attribute_utils.h"
#include "opentelemetry/trace/link.h"
#include "opentelemetry/version.h"

#include <memory>

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace trace
{
namespace trace_api = opentelemetry::trace;

class Link final : public trace_api::Link
{
public:
Link(opentelemetry::trace::SpanContext span_context,
const opentelemetry::trace::KeyValueIterableView<
std::unordered_map<nostd::string_view, opentelemetry::common::AttributeValue>>
&attributes) noexcept
: span_context_(span_context), attribute_map_{attributes}
{}

Link(opentelemetry::trace::SpanContext span_context)
: span_context_(span_context),
attribute_map_(
std::unordered_map<nostd::string_view, opentelemetry::common::AttributeValue>())
{}

const trace_api::SpanContext &GetContext() const noexcept override { return span_context_; }

const trace_api::KeyValueIterable &GetAttributes() const noexcept override
{
return attribute_map_;
}

private:
trace_api::SpanContext span_context_;
const trace_api::KeyValueIterableView<
std::unordered_map<nostd::string_view, opentelemetry::common::AttributeValue>>
attribute_map_;
};
} // namespace trace
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
4 changes: 2 additions & 2 deletions sdk/include/opentelemetry/sdk/trace/recordable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,14 +82,14 @@ class Recordable
* @param span_context the span context of the linked span
* @param attributes the attributes associated with the link
*/
virtual void AddLink(opentelemetry::trace::SpanContext span_context,
virtual void AddLink(const opentelemetry::trace::SpanContext &span_context,
const trace_api::KeyValueIterable &attributes) noexcept = 0;

/**
* Add a link to a span with default (empty) attributes.
* @param span_context the span context of the linked span
*/
void AddLink(opentelemetry::trace::SpanContext span_context)
void AddLink(const opentelemetry::trace::SpanContext &span_context)
{
AddLink(span_context, opentelemetry::sdk::GetEmptyAttributes());
}
Expand Down
4 changes: 2 additions & 2 deletions sdk/include/opentelemetry/sdk/trace/span_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class SpanDataEvent
class SpanDataLink
{
public:
SpanDataLink(opentelemetry::trace::SpanContext span_context,
SpanDataLink(const opentelemetry::trace::SpanContext &span_context,
const trace_api::KeyValueIterable &attributes)
: span_context_(span_context), attribute_map_(attributes)
{}
Expand Down Expand Up @@ -180,7 +180,7 @@ class SpanData final : public Recordable
events_.push_back(event);
}

void AddLink(opentelemetry::trace::SpanContext span_context,
void AddLink(const opentelemetry::trace::SpanContext &span_context,
const trace_api::KeyValueIterable &attributes) noexcept override
{
SpanDataLink link(span_context, attributes);
Expand Down
37 changes: 36 additions & 1 deletion sdk/src/trace/span.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,15 @@ Span::Span(std::shared_ptr<Tracer> &&tracer,
nostd::string_view name,
const trace_api::KeyValueIterable &attributes,
const trace_api::StartSpanOptions &options,
const trace_api::SpanContext &parent_span_context) noexcept
const trace_api::SpanContext &parent_span_context,
const nostd::span<std::shared_ptr<trace_api::Link>> &links) noexcept
: tracer_{std::move(tracer)},
processor_{processor},
recordable_{processor_->MakeRecordable()},
start_steady_time{options.start_steady_time},
has_ended_{false}
{
(void)options;
Copy link
Copy Markdown
Contributor

@ThomsonTan ThomsonTan Sep 30, 2020

Choose a reason for hiding this comment

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

Not sure why this is added back, should be removed because it was referenced in line 108 and 109.

Copy link
Copy Markdown
Member Author

@lalitb lalitb Oct 5, 2020

Choose a reason for hiding this comment

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

Yeah my bad . Thanks for pointing. Have raised a separate PR #351 as this was now a rework. Please review that. Will close this one.

if (recordable_ == nullptr)
{
return;
Expand Down Expand Up @@ -98,6 +100,11 @@ Span::Span(std::shared_ptr<Tracer> &&tracer,
return true;
});

for (auto &link : links)
{
recordable_->AddLink(link->GetContext(), link->GetAttributes());
}

recordable_->SetStartTime(NowOr(options.start_system_time));
start_steady_time = NowOr(options.start_steady_time);
processor_->OnStart(*recordable_);
Expand Down Expand Up @@ -136,6 +143,34 @@ void Span::AddEvent(nostd::string_view name,
(void)attributes;
}

void Span::AddLink(const trace_api::Link &link) noexcept
{
Copy link
Copy Markdown
Contributor

@ThomsonTan ThomsonTan Sep 30, 2020

Choose a reason for hiding this comment

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

Declare lock_guard(mf_) to make sure it is thread-safe?

Copy link
Copy Markdown
Member Author

@lalitb lalitb Oct 5, 2020

Choose a reason for hiding this comment

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

Have removed AddLink() as part of separate PR #351 . Please review that. thanks.

if (recordable_ == nullptr)
{
return;
}
recordable_->AddLink(link.GetContext(), link.GetAttributes());
}

void Span::AddLink(const trace_api::SpanContext &span_context,
const trace_api::KeyValueIterable &attributes) noexcept
{
if (recordable_ == nullptr)
{
return;
}
recordable_->AddLink(span_context, attributes);
}

void Span::AddLink(const trace_api::SpanContext &span_context) noexcept
{
if (recordable_ == nullptr)
{
return;
}
recordable_->AddLink(span_context);
}

void Span::SetStatus(trace_api::CanonicalCode code, nostd::string_view description) noexcept
{
std::lock_guard<std::mutex> lock_guard{mu_};
Expand Down
11 changes: 10 additions & 1 deletion sdk/src/trace/span.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include <mutex>

#include "opentelemetry/sdk/trace/link.h"
#include "opentelemetry/sdk/trace/tracer.h"
#include "opentelemetry/version.h"

Expand All @@ -20,7 +21,8 @@ class Span final : public trace_api::Span
nostd::string_view name,
const trace_api::KeyValueIterable &attributes,
const trace_api::StartSpanOptions &options,
const trace_api::SpanContext &parent_span_context) noexcept;
const trace_api::SpanContext &parent_span_context,
const nostd::span<std::shared_ptr<trace_api::Link>> &links) noexcept;

~Span() override;

Expand All @@ -35,6 +37,13 @@ class Span final : public trace_api::Span
core::SystemTimestamp timestamp,
const trace_api::KeyValueIterable &attributes) noexcept override;

void AddLink(const trace_api::Link &link) noexcept override;

void AddLink(const trace_api::SpanContext &span_context,
const trace_api::KeyValueIterable &attributes) noexcept override;

void AddLink(const trace_api::SpanContext &span_context) noexcept override;

void SetStatus(trace_api::CanonicalCode code, nostd::string_view description) noexcept override;

void UpdateName(nostd::string_view name) noexcept override;
Expand Down
10 changes: 7 additions & 3 deletions sdk/src/trace/tracer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,13 @@ nostd::shared_ptr<trace_api::Span> Tracer::StartSpan(
}
else
{
auto span = nostd::shared_ptr<trace_api::Span>{
new (std::nothrow) Span{this->shared_from_this(), processor_.load(), name, attributes,
options, GetCurrentSpanContext()}};
auto span = nostd::shared_ptr<trace_api::Span>{new (std::nothrow) Span{this->shared_from_this(),
processor_.load(),
name,
attributes,
options,
GetCurrentSpanContext(),
{}}};

// if the attributes is not nullptr, add attributes to the span.
if (sampling_result.attributes)
Expand Down
19 changes: 19 additions & 0 deletions sdk/test/trace/tracer_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "opentelemetry/sdk/trace/tracer.h"
#include "opentelemetry/sdk/trace/link.h"
#include "opentelemetry/sdk/trace/samplers/always_off.h"
#include "opentelemetry/sdk/trace/samplers/always_on.h"
#include "opentelemetry/sdk/trace/samplers/parent_or_else.h"
Expand Down Expand Up @@ -341,6 +342,24 @@ TEST(Tracer, SpanSetAttribute)
ASSERT_EQ(3.1, nostd::get<double>(span_data->GetAttributes().at("abc")));
}

TEST(Tracer, SpanAddLink)
{
std::shared_ptr<std::vector<std::unique_ptr<SpanData>>> spans_received(
new std::vector<std::unique_ptr<SpanData>>);
auto tracer = initTracer(spans_received);

auto span = tracer->StartSpan("span 1");

SpanContext sp1(true, true);
span->AddLink(sp1);

span->End();
ASSERT_EQ(1, spans_received->size());
auto &span_data = spans_received->at(0);

ASSERT_EQ(1, span_data->GetLinks().size());
}

TEST(Tracer, TestAlwaysOnSampler)
{
std::shared_ptr<std::vector<std::unique_ptr<SpanData>>> spans_received(
Expand Down