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

Add resource to Recordable, make room for InstrumentationLibrary #580

Closed
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,12 @@ class ETWSpanData final : public sdk::trace::Recordable
parent_span_id_ = parent_span_id;
}

void SetResourceRef(
const opentelemetry::sdk::resource::Resource &resource) noexcept override
{
// TODO
}

void SetAttribute(nostd::string_view key,
const opentelemetry::common::AttributeValue &value) noexcept override
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#pragma once

#include "opentelemetry/proto/resource/v1/resource.pb.h"
#include "opentelemetry/proto/trace/v1/trace.pb.h"
#include "opentelemetry/sdk/trace/recordable.h"
#include "opentelemetry/version.h"
Expand All @@ -13,11 +14,16 @@ class Recordable final : public sdk::trace::Recordable
{
public:
const proto::trace::v1::Span &span() const noexcept { return span_; }
/** Dynamically converts the resource of this span into a proto. */
proto::resource::v1::Resource resource() const noexcept;

void SetIds(trace::TraceId trace_id,
trace::SpanId span_id,
trace::SpanId parent_span_id) noexcept override;

void SetResourceRef(
const opentelemetry::sdk::resource::Resource &resource) noexcept override;

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

Expand All @@ -40,6 +46,7 @@ class Recordable final : public sdk::trace::Recordable

private:
proto::trace::v1::Span span_;
opentelemetry::sdk::resource::Resource &resource_{opentelemetry::sdk::resource::Resource::GetEmpty()};
};
} // namespace otlp
} // namespace exporter
Expand Down
9 changes: 8 additions & 1 deletion exporters/otlp/src/otlp_exporter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,17 @@ void PopulateRequest(const nostd::span<std::unique_ptr<sdk::trace::Recordable>>
{
auto resource_span = request->add_resource_spans();
auto instrumentation_lib = resource_span->add_instrumentation_library_spans();

bool has_resource = false;
for (auto &recordable : spans)
{
auto rec = std::unique_ptr<Recordable>(static_cast<Recordable *>(recordable.release()));
// We assume all the spans are for the same resource.
if (!has_resource)
{
// *resource_span->mutable_resource() = std::move(rec->resource());
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: replace this comment with TODO support for moving?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, theoretically it is moved, I should have removed it before :)

*resource_span->mutable_resource() = rec->resource();
has_resource = true;
Copy link
Member

Choose a reason for hiding this comment

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

nit: we can break from loop once resource is found.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we could assert the later found resource equals the first one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't. I'm using the loop that adds the spans to add the resource. IF we break than we'll be dropping spans.

}
*instrumentation_lib->add_spans() = std::move(rec->span());
}
}
Expand Down
118 changes: 117 additions & 1 deletion exporters/otlp/src/recordable.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ namespace exporter
namespace otlp
{

const int kAttributeValueSize = 14;
const int kAttributeValueSize = 14;
const int kOwnedAttributeValueSize = 14;

void Recordable::SetIds(trace::TraceId trace_id,
trace::SpanId span_id,
Expand Down Expand Up @@ -117,6 +118,120 @@ void PopulateAttribute(opentelemetry::proto::common::v1::KeyValue *attribute,
}
}

/** Maps from C++ attribute into OTLP proto attribute. */
void PopulateAttribute(opentelemetry::proto::common::v1::KeyValue *attribute,
nostd::string_view key,
const sdk::common::OwnedAttributeValue &value)
{
// Assert size of variant to ensure that this method gets updated if the variant
// definition changes
static_assert(
nostd::variant_size<opentelemetry::common::AttributeValue>::value == kOwnedAttributeValueSize,
"AttributeValue contains unknown type");

attribute->set_key(key.data(), key.size());

if (nostd::holds_alternative<bool>(value))
{
attribute->mutable_value()->set_bool_value(nostd::get<bool>(value));
}
else if (nostd::holds_alternative<int32_t>(value))
{
attribute->mutable_value()->set_int_value(nostd::get<int32_t>(value));
}
else if (nostd::holds_alternative<int64_t>(value))
{
attribute->mutable_value()->set_int_value(nostd::get<int64_t>(value));
}
else if (nostd::holds_alternative<uint32_t>(value))
{
attribute->mutable_value()->set_int_value(nostd::get<uint32_t>(value));
}
else if (nostd::holds_alternative<uint64_t>(value))
{
attribute->mutable_value()->set_int_value(nostd::get<uint64_t>(value));
}
else if (nostd::holds_alternative<double>(value))
{
attribute->mutable_value()->set_double_value(nostd::get<double>(value));
}
else if (nostd::holds_alternative<std::string>(value))
{
attribute->mutable_value()->set_string_value(nostd::get<std::string>(value));
}
#ifdef HAVE_SPAN_BYTE
else if (nostd::holds_alternative<uint8_t>(value))
{
attribute->mutable_value()->set_int_value(nostd::get<uint8_t>(value));
}
#endif
else if (nostd::holds_alternative<std::vector<bool>>(value))
{
for (const auto &val : nostd::get<std::vector<bool>>(value))
{
attribute->mutable_value()->mutable_array_value()->add_values()->set_bool_value(val);
}
}
else if (nostd::holds_alternative<std::vector<int32_t>>(value))
{
for (const auto &val : nostd::get<std::vector<int32_t>>(value))
{
attribute->mutable_value()->mutable_array_value()->add_values()->set_int_value(val);
}
}
else if (nostd::holds_alternative<std::vector<uint32_t>>(value))
{
for (const auto &val : nostd::get<std::vector<uint32_t>>(value))
{
attribute->mutable_value()->mutable_array_value()->add_values()->set_int_value(val);
}
}
else if (nostd::holds_alternative<std::vector<int64_t>>(value))
{
for (const auto &val : nostd::get<std::vector<int64_t>>(value))
{
attribute->mutable_value()->mutable_array_value()->add_values()->set_int_value(val);
}
}
else if (nostd::holds_alternative<std::vector<uint64_t>>(value))
{
for (const auto &val : nostd::get<std::vector<uint64_t>>(value))
{
attribute->mutable_value()->mutable_array_value()->add_values()->set_int_value(val);
}
}
else if (nostd::holds_alternative<std::vector<double>>(value))
{
for (const auto &val : nostd::get<std::vector<double>>(value))
{
attribute->mutable_value()->mutable_array_value()->add_values()->set_double_value(val);
}
}
else if (nostd::holds_alternative<std::vector<std::string>>(value))
{
for (const auto &val : nostd::get<std::vector<std::string>>(value))
{
attribute->mutable_value()->mutable_array_value()->add_values()->set_string_value(val);
}
}
}

proto::resource::v1::Resource Recordable::resource() const noexcept
{
proto::resource::v1::Resource proto;
for (const auto &kv : resource_.GetAttributes())
{
PopulateAttribute(proto.add_attributes(), kv.first, kv.second);
}
return proto;
}

void Recordable::SetResourceRef(
const opentelemetry::sdk::resource::Resource &resource) noexcept
{
resource_ = resource;
};

void Recordable::SetAttribute(nostd::string_view key,
const opentelemetry::common::AttributeValue &value) noexcept
{
Expand Down Expand Up @@ -217,6 +332,7 @@ void Recordable::SetDuration(std::chrono::nanoseconds duration) noexcept
const uint64_t unix_end_time = span_.start_time_unix_nano() + duration.count();
span_.set_end_time_unix_nano(unix_end_time);
}

} // namespace otlp
} // namespace exporter
OPENTELEMETRY_END_NAMESPACE
41 changes: 28 additions & 13 deletions exporters/otlp/test/otlp_exporter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -65,22 +65,37 @@ TEST_F(OtlpExporterTestPeer, ExportIntegrationTest)
std::unique_ptr<proto::collector::trace::v1::TraceService::StubInterface> stub_interface(
mock_stub);

auto exporter = GetExporter(stub_interface);
proto::collector::trace::v1::ExportTraceServiceRequest request;

auto processor = std::shared_ptr<sdk::trace::SpanProcessor>(
new sdk::trace::SimpleSpanProcessor(std::move(exporter)));
auto provider =
nostd::shared_ptr<trace::TracerProvider>(new sdk::trace::TracerProvider(processor));
auto tracer = provider->GetTracer("test");
// Create a scope where all traces are flushed within it
{
auto exporter = GetExporter(stub_interface);

EXPECT_CALL(*mock_stub, Export(_, _, _))
.Times(AtLeast(1))
.WillRepeatedly(Return(grpc::Status::OK));
auto processor = std::shared_ptr<sdk::trace::SpanProcessor>(
new sdk::trace::SimpleSpanProcessor(std::move(exporter)));
auto provider =
nostd::shared_ptr<trace::TracerProvider>(new sdk::trace::TracerProvider(processor));
auto tracer = provider->GetTracer("test");

auto parent_span = tracer->StartSpan("Test parent span");
auto child_span = tracer->StartSpan("Test child span");
child_span->End();
parent_span->End();
// TODO: Capture the exported proto and inspect it.
EXPECT_CALL(*mock_stub, Export(_, _, _))
.Times(AtLeast(1))
.WillRepeatedly(DoAll(SaveArg<1>(&request), Return(grpc::Status::OK)));

auto parent_span = tracer->StartSpan("Test parent span");
auto child_span = tracer->StartSpan("Test child span");
child_span->End();
parent_span->End();
}
// After flushing, let's check the (last) request has the last span.
EXPECT_EQ(1, request.resource_spans_size());
EXPECT_EQ(1, request.resource_spans(0).instrumentation_library_spans_size());
EXPECT_EQ(1, request.resource_spans(0).instrumentation_library_spans(0).spans_size());
// Expect the default resource to have been used.
EXPECT_EQ(3, request.resource_spans(0).resource().attributes_size());
// And that the span name is the last span ended.
EXPECT_EQ("Test parent span",
request.resource_spans(0).instrumentation_library_spans(0).spans(0).name());
}

// Test exporter configuration options
Expand Down
16 changes: 16 additions & 0 deletions exporters/otlp/test/recordable_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,22 @@ TEST(Recordable, SetArrayAtrribute)
}
}

TEST(Recordable, SetResourceRef)
{
Recordable rec;
sdk::resource::Resource default_resource = sdk::resource::Resource::GetDefault();
sdk::resource::Resource empty_resource = sdk::resource::Resource::GetEmpty();
rec.SetResourceRef(default_resource);
auto proto = rec.resource();
EXPECT_EQ(3, proto.attributes_size());
// Note: We don't test ALL attribtues, only a few to avoid flaky tests and to
// make sure we ARE getting attributes passed through.
EXPECT_EQ("telemetry.sdk.name", proto.attributes(1).key());
EXPECT_EQ("opentelemetry", proto.attributes(1).value().string_value());
rec.SetResourceRef(empty_resource);
EXPECT_EQ(0, rec.resource().attributes_size());
}

/**
* AttributeValue can contain different int types, such as int, int64_t,
* unsigned int, and uint64_t. To avoid writing test cases for each, we can
Expand Down
6 changes: 6 additions & 0 deletions ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,12 @@ class ThreadsafeSpanData final : public opentelemetry::sdk::trace::Recordable
parent_span_id_ = parent_span_id;
}

void SetResourceRef(
const opentelemetry::sdk::resource::Resource &resource) noexcept override
{
// TODO
}

void SetAttribute(nostd::string_view key, const common::AttributeValue &value) noexcept override
{
std::lock_guard<std::mutex> lock(mutex_);
Expand Down
8 changes: 6 additions & 2 deletions ext/test/zpages/tracez_data_aggregator_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "opentelemetry/sdk/resource/resource.h"
#include "opentelemetry/sdk/trace/recordable.h"
#include "opentelemetry/sdk/trace/tracer.h"
#include "opentelemetry/sdk/trace/tracer_provider.h"

using namespace opentelemetry::sdk::trace;
using namespace opentelemetry::ext::zpages;
Expand Down Expand Up @@ -36,13 +37,16 @@ class TracezDataAggregatorTest : public ::testing::Test
{
std::shared_ptr<TracezSpanProcessor> processor(new TracezSpanProcessor());
auto resource = opentelemetry::sdk::resource::Resource::Create({});
tracer = std::shared_ptr<opentelemetry::trace::Tracer>(new Tracer(processor, resource));
tracer_provider =
std::shared_ptr<TracerProvider>(new TracerProvider(processor, std::move(resource)));
tracer = tracer_provider->GetTracer("test", "1.0");
tracez_data_aggregator = std::unique_ptr<TracezDataAggregator>(
new TracezDataAggregator(processor, milliseconds(10)));
}

std::unique_ptr<TracezDataAggregator> tracez_data_aggregator;
std::shared_ptr<opentelemetry::trace::Tracer> tracer;
std::shared_ptr<TracerProvider> tracer_provider;
opentelemetry::nostd::shared_ptr<opentelemetry::trace::Tracer> tracer;
};

/**
Expand Down
11 changes: 7 additions & 4 deletions ext/test/zpages/tracez_processor_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "opentelemetry/nostd/span.h"
#include "opentelemetry/sdk/resource/resource.h"
#include "opentelemetry/sdk/trace/tracer.h"
#include "opentelemetry/sdk/trace/tracer_provider.h"

using namespace opentelemetry::sdk::trace;
using namespace opentelemetry::ext::zpages;
Expand Down Expand Up @@ -148,7 +149,7 @@ void GetManySnapshots(std::shared_ptr<TracezSpanProcessor> &processor, int i)
*/
void StartManySpans(
std::vector<opentelemetry::nostd::shared_ptr<opentelemetry::trace::Span>> &spans,
std::shared_ptr<opentelemetry::trace::Tracer> tracer,
opentelemetry::nostd::shared_ptr<opentelemetry::trace::Tracer> tracer,
int i)
{
for (; i > 0; i--)
Expand Down Expand Up @@ -177,8 +178,9 @@ class TracezProcessor : public ::testing::Test
{
processor = std::shared_ptr<TracezSpanProcessor>(new TracezSpanProcessor());
auto resource = opentelemetry::sdk::resource::Resource::Create({});

tracer = std::shared_ptr<opentelemetry::trace::Tracer>(new Tracer(processor, resource));
tracer_provider =
std::shared_ptr<TracerProvider>(new TracerProvider(processor, std::move(resource)));
tracer = tracer_provider->GetTracer("test", "1.0");
auto spans = processor->GetSpanSnapshot();
running = spans.running;
completed = std::move(spans.completed);
Expand All @@ -187,7 +189,8 @@ class TracezProcessor : public ::testing::Test
}

std::shared_ptr<TracezSpanProcessor> processor;
std::shared_ptr<opentelemetry::trace::Tracer> tracer;
std::shared_ptr<opentelemetry::sdk::trace::TracerProvider> tracer_provider;
opentelemetry::nostd::shared_ptr<opentelemetry::trace::Tracer> tracer;

std::vector<std::string> span_names;
std::vector<opentelemetry::nostd::shared_ptr<opentelemetry::trace::Span>> span_vars;
Expand Down
14 changes: 14 additions & 0 deletions sdk/include/opentelemetry/sdk/trace/recordable.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "opentelemetry/core/timestamp.h"
#include "opentelemetry/nostd/string_view.h"
#include "opentelemetry/sdk/common/empty_attributes.h"
#include "opentelemetry/sdk/resource/resource.h"
#include "opentelemetry/trace/canonical_code.h"
#include "opentelemetry/trace/span.h"
#include "opentelemetry/trace/span_context.h"
Expand Down Expand Up @@ -41,6 +42,19 @@ class Recordable
opentelemetry::trace::SpanId span_id,
opentelemetry::trace::SpanId parent_span_id) noexcept = 0;

/**
* Sets the resource associated with a span.
*
* <p> The resource is guaranteed to have a lifetime longer than this recordable. It is
Copy link
Contributor

Choose a reason for hiding this comment

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

The resource is guaranteed to have a lifetime longer than this recordable.

I'm not sure if we can make this statement as is. I'm thinking about a scenario where the user frees the TracerProvider, but the exporter is still working on exporting spans (recordables), maybe in another thread.

If we go with plain pointers for resources (instead of shared_ptr), we'd need to be very careful, and either explicitly document all implicit assumptions we make, or take additional precautions to avoid problematic scenarios (e. g. making sure that in the TracerProvider destructor Shutdown is called for all exporters).

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'm planning to change TracerProvider to (a) ensure shutdown and (b) require resource cleanup after shutdown.

Would you be amenable to that change in correlation with this? I think it's needed to make this safe, and I'm happy to go update the docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm definitely amenable to you making those changes.

Some other question: did you for performance reasons decide not to use a managed pointer (std::shared_ptr) for holding on to the resource in the recordable? And rather have implicit ownership via trace provider/recordable lifetimes?

So far we tried to make all ownership explicit to be on the (memory) safe side.

* owned by the `TracerProvider` which (indirectly) owns the `Exporter` that generates a
* `Recordable`.
*
* @param resource the Resource for this span.
* Note: this reference will remain stable for the life of the Recordable.
*/
virtual void SetResourceRef(
const opentelemetry::sdk::resource::Resource &resource) noexcept = 0;

/**
* Set an attribute of a span.
* @param name the name of the attribute
Expand Down
Loading