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 span events and status #794

Open
wants to merge 56 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
5900cf2
Add events
nepridumalnik Dec 19, 2024
056e86a
Intermediate commit
nepridumalnik Dec 20, 2024
1b9d466
Remove attributes
nepridumalnik Dec 20, 2024
d01412d
Serialization refactoring
nepridumalnik Dec 20, 2024
b2227f5
Brief
nepridumalnik Dec 20, 2024
9dd35c2
Add status
nepridumalnik Dec 20, 2024
beca706
Add mutable variable
nepridumalnik Dec 20, 2024
24e8e36
Return method
nepridumalnik Dec 20, 2024
7004622
Remove
nepridumalnik Dec 20, 2024
57ead82
Build fix
nepridumalnik Dec 20, 2024
aea058c
Fix sizes
nepridumalnik Dec 20, 2024
c92d19c
Add events
nepridumalnik Dec 19, 2024
84a6697
Intermediate commit
nepridumalnik Dec 20, 2024
cd47c49
Remove attributes
nepridumalnik Dec 20, 2024
13b1fcf
Serialization refactoring
nepridumalnik Dec 20, 2024
d72491d
Brief
nepridumalnik Dec 20, 2024
e27246b
Add status
nepridumalnik Dec 20, 2024
b6704d5
Add mutable variable
nepridumalnik Dec 20, 2024
f2d0fd8
Return method
nepridumalnik Dec 20, 2024
146372d
Remove
nepridumalnik Dec 20, 2024
5472f64
Build fix
nepridumalnik Dec 20, 2024
ef410e3
Fix sizes
nepridumalnik Dec 20, 2024
1e1c73d
Merge branch 'span_events_and_status' of https://github.com/nepriduma…
nepridumalnik Dec 20, 2024
a143bb5
Update
nepridumalnik Dec 20, 2024
1f61f80
Merge branch 'develop' of https://github.com/userver-framework/userve…
nepridumalnik Dec 20, 2024
715dc7e
Chore
nepridumalnik Dec 23, 2024
3d7d18a
Review
nepridumalnik Dec 23, 2024
10cf365
Add events
nepridumalnik Dec 19, 2024
8d796e5
Intermediate commit
nepridumalnik Dec 20, 2024
0afe674
Remove attributes
nepridumalnik Dec 20, 2024
a679266
Serialization refactoring
nepridumalnik Dec 20, 2024
4a761bd
Brief
nepridumalnik Dec 20, 2024
0959e57
Add status
nepridumalnik Dec 20, 2024
6315ede
Add mutable variable
nepridumalnik Dec 20, 2024
777ea21
Return method
nepridumalnik Dec 20, 2024
27d80e2
Remove
nepridumalnik Dec 20, 2024
ebd5001
Build fix
nepridumalnik Dec 20, 2024
1da9c84
Fix sizes
nepridumalnik Dec 20, 2024
5392321
Add events
nepridumalnik Dec 19, 2024
86a774c
Intermediate commit
nepridumalnik Dec 20, 2024
64386d7
Remove attributes
nepridumalnik Dec 20, 2024
9b00bad
Serialization refactoring
nepridumalnik Dec 20, 2024
84c7ac7
Add status
nepridumalnik Dec 20, 2024
3c9ddfd
Return method
nepridumalnik Dec 20, 2024
7896216
Update
nepridumalnik Dec 20, 2024
cb5bd17
Chore
nepridumalnik Dec 23, 2024
140eac9
Review
nepridumalnik Dec 23, 2024
5114a21
Chore
nepridumalnik Jan 13, 2025
a99e283
Merge branch 'span_events_and_status_2' into span_events_and_status
nepridumalnik Jan 13, 2025
eaa9ef7
Review
nepridumalnik Jan 13, 2025
d70c85a
Small fix
nepridumalnik Jan 13, 2025
814b22f
Review
nepridumalnik Jan 14, 2025
9cee7c0
Fix test
nepridumalnik Jan 14, 2025
5046d79
retrigger checks
nepridumalnik Jan 14, 2025
4885f86
Add specification
nepridumalnik Jan 15, 2025
181bbf2
Add tests
nepridumalnik Jan 15, 2025
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
2 changes: 1 addition & 1 deletion core/include/userver/tracing/in_place_span.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class InPlaceSpan final {

private:
struct Impl;
utils::FastPimpl<Impl, 4224, 8> impl_;
utils::FastPimpl<Impl, 4240, 8> impl_;
};

} // namespace tracing
Expand Down
34 changes: 34 additions & 0 deletions core/include/userver/tracing/span.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,34 @@ class Span final {
public:
class Impl;

/// @brief Operation status code.
Copy link
Member

Choose a reason for hiding this comment

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

/// @brief Operation status code.
/// @see https://opentelemetry.io/docs/concepts/signals/traces/#span-status
/// @see https://github.com/open-telemetry/opentelemetry-proto/blob/v1.3.2/opentelemetry/proto/trace/v1/trace.proto#L312

enum class StatusCode : uint8_t {
kUnset, // Default status.
kOk, // Operation has completed successfully.
kError, // The operation contains an error.
};

/// @brief Span event.
Copy link
Member

Choose a reason for hiding this comment

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

/// @brief Span event -- time-stamped annotation of the span with user-provided text description.
/// @see https://opentelemetry.io/docs/concepts/signals/traces/#span-events
/// @see https://github.com/open-telemetry/opentelemetry-proto/blob/v1.3.2/opentelemetry/proto/trace/v1/trace.proto#L220
/// @todo Implement event attributes.

/// @see
/// https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/trace/v1/trace.proto#L222.
/// @todo Implement attributes.
struct Event final {
/// @brief Constructor.
/// @param name Event name.
explicit Event(std::string_view name);

/// @brief Constructor.
/// @param name Event name.
/// @param time_unix_nano Event timestamp.
Event(std::string_view name, uint64_t time_unix_nano);

/// @brief Event name.
std::string name;

/// @brief Event timestamp.
uint64_t time_unix_nano{};
};

explicit Span(
TracerPtr tracer,
std::string name,
Expand Down Expand Up @@ -157,6 +185,12 @@ class Span final {
/// @overload AddNonInheritableTag
void AddNonInheritableTags(const logging::LogExtra&);

/// Add an event to Span.
void AddEvent(std::string_view event_name);
Copy link
Member

Choose a reason for hiding this comment

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

How do you plan to add attributes to event class?
Maybe it is better to pass Span::Event in this method?


/// Set span status.
void SetStatus(StatusCode status, const std::string_view description);
Copy link
Member

Choose a reason for hiding this comment

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

We already has method that sets error flag: span.AddTag(tracing::kErrorFlag, true);

It is bad to have two methods for making the same result.

I think the right solution is to remove tracing::kErrorFlag and all its occurences: https://github.com/search?q=repo%3Auserver-framework%2Fuserver+AddTag%28tracing%3A%3AkErrorFlag&type=code

and use SetStatus there

Maybe there is another better solution?


/// @brief Sets level for tags logging
void SetLogLevel(logging::Level log_level);

Expand Down
2 changes: 1 addition & 1 deletion core/include/userver/utils/async.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ struct SpanWrapCall {

struct Impl;

static constexpr std::size_t kImplSize = 4264;
static constexpr std::size_t kImplSize = 4280;
static constexpr std::size_t kImplAlign = 8;
utils::FastPimpl<Impl, kImplSize, kImplAlign> pimpl_;
};
Expand Down
29 changes: 29 additions & 0 deletions core/src/tracing/span.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ void Span::Impl::PutIntoLogger(logging::impl::TagWriter writer) && {
}
writer.PutLogExtra(log_extra_inheritable_);

LogEvents(writer);
LogOpenTracing();
}

Expand Down Expand Up @@ -368,6 +369,29 @@ void Span::AddTagFrozen(std::string key, logging::LogExtra::Value value) {
pimpl_->log_extra_inheritable_.Extend(std::move(key), std::move(value), logging::LogExtra::ExtendType::kFrozen);
}

void Span::AddEvent(std::string_view event_name) { pimpl_->events_.emplace_back(event_name); }

// Opentelemetry implementation:
// https://github.com/open-telemetry/opentelemetry-cpp/blob/v1.18.0/exporters/zipkin/src/recordable.cc#L220
void Span::SetStatus(StatusCode status, const std::string_view description) {
if (status == StatusCode::kUnset) {
return;
}

AddTag("otel.status_description", std::string{description});

if (status == StatusCode::kError) {
// Error description is required by OpenTelemetry

// Service data
AddTag("otel.status_code", "ERROR");
AddTag("error", "true");
} else {
AddTag("otel.status_code", "OK");
AddTag("error", "false");
}
Comment on lines +377 to +392
Copy link
Member

Choose a reason for hiding this comment

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

place the implementation to anonymous namespace please

}

void Span::SetLink(std::string link) { AddTagFrozen(kLinkTag, std::move(link)); }

void Span::SetParentLink(std::string parent_link) { AddTagFrozen(kParentLinkTag, std::move(parent_link)); }
Expand Down Expand Up @@ -412,6 +436,11 @@ const Span::Impl* GetParentSpanImpl() {
return !spans_ptr || spans_ptr->empty() ? nullptr : &spans_ptr->back();
}

Span::Event::Event(std::string_view name)
: Span::Event{name, static_cast<uint64_t>(std::chrono::system_clock::now().time_since_epoch().count())} {}

Span::Event::Event(std::string_view name, uint64_t time_unix_nano) : name{name}, time_unix_nano{time_unix_nano} {}

namespace impl {

struct DetachLocalSpansScope::Impl {
Expand Down
3 changes: 3 additions & 0 deletions core/src/tracing/span_impl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ class Span::Impl : public boost::intrusive::list_base_hook<boost::intrusive::lin
void AttachToCoroStack();

private:
void LogEvents(logging::impl::TagWriter& writer) const;
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove the method and do all work in DoLogOpenTracing

Something like in the end of DoLogOpenTracing

if (!events_.empty()) {
        const auto events_tag = jaeger::MakeTagFromEvents(events_);
        writer.PutTag(jaeger::kEvents, events_tag);
    }

where jaeger::MakeTagFromEvents is

std::string MakeTagFromEvents(const std::vector<Span::Event>& events) {
    formats::json::StringBuilder builder;
    {
        const formats::json::StringBuilder::ObjectGuard event_guard(builder);

        for (const auto& event : events) {
            builder.Key(event.name);
            builder.WriteUInt64(event.time_unix_nano);
        }
    }

    return builder.GetString();
}

void LogOpenTracing() const;
void DoLogOpenTracing(logging::impl::TagWriter writer) const;
static void AddOpentracingTags(formats::json::StringBuilder& output, const logging::LogExtra& input);
Expand Down Expand Up @@ -105,6 +106,8 @@ class Span::Impl : public boost::intrusive::list_base_hook<boost::intrusive::lin
const ReferenceType reference_type_;
utils::impl::SourceLocation source_location_;

std::vector<Span::Event> events_;

friend class Span;
friend class SpanBuilder;
friend class TagScope;
Expand Down
19 changes: 19 additions & 0 deletions core/src/tracing/span_opentracing.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ constexpr std::string_view kStartTimeMillis = "start_time_millis";
constexpr std::string_view kDuration = "duration";

} // namespace jaeger

} // namespace

void Span::Impl::LogOpenTracing() const {
Expand Down Expand Up @@ -123,6 +124,24 @@ void Span::Impl::DoLogOpenTracing(logging::impl::TagWriter writer) const {
writer.PutTag("tags", tags.GetStringView());
}

void Span::Impl::LogEvents(logging::impl::TagWriter& writer) const {
if (events_.empty()) {
return;
}

formats::json::StringBuilder events;
{
const formats::json::StringBuilder::ObjectGuard event_guard(events);

for (const auto& event : events_) {
events.Key(event.name);
events.WriteUInt64(event.time_unix_nano);
}
}

writer.PutTag("events", events.GetStringView());
}

void Span::Impl::AddOpentracingTags(formats::json::StringBuilder& output, const logging::LogExtra& input) {
for (const auto& [key, value] : *input.extra_) {
const auto tag_it = jaeger::kGetOpentracingTags.TryFind(key);
Expand Down
48 changes: 48 additions & 0 deletions core/src/tracing/span_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,10 @@ UTEST_F(Span, LogBufferSize) {
}

UTEST_F(Span, SourceLocation) {
// clang-format off
{ tracing::Span span("span_name"); }
// clang-format on

logging::LogFlush();
EXPECT_THAT(GetStreamString(), HasSubstr("module=TestBody ( "));
EXPECT_THAT(GetStreamString(), HasSubstr("userver/core/src/tracing"));
Expand Down Expand Up @@ -204,7 +207,10 @@ UTEST_F(OpentracingSpan, Tags) {

UTEST_F(OpentracingSpan, FromTracerWithServiceName) {
auto tracer = tracing::MakeTracer("test_service", tracing::Tracer::GetTracer()->GetOptionalLogger());
// clang-format off
{ tracing::Span span(tracer, "span_name", nullptr, tracing::ReferenceType::kChild); }
// clang-format on

FlushOpentracing();
const auto log_str = GetOtStreamString();
EXPECT_THAT(log_str, HasSubstr("service_name=test_service"));
Expand Down Expand Up @@ -430,6 +436,7 @@ UTEST_F(Span, NoLogPrefixes) {
};
tracing::Tracer::SetNoLogSpans(std::move(no_logs));

// clang-format off
{ tracing::Span a{kIgnorePrefix0 + "foo"}; }
{ tracing::Span a{kLogSpan0}; }
{ tracing::Span a{kLogSpan1}; }
Expand All @@ -442,6 +449,7 @@ UTEST_F(Span, NoLogPrefixes) {
{ tracing::Span a{kIgnorePrefix1}; }
{ tracing::Span a{kIgnorePrefix2}; }
{ tracing::Span a{kIgnoreSpan}; }
// clang-format on

logging::LogFlush();

Expand Down Expand Up @@ -479,6 +487,7 @@ UTEST_F(Span, NoLogMixed) {
const std::string kIgnorePrefix1 = "skip";
const std::string kIgnorePrefix2 = "do_not_keep";

// clang-format off
{ tracing::Span a{kIgnorePrefix0 + "oops"}; }
{ tracing::Span a{kLogSpan0}; }
{ tracing::Span a{kLogSpan1}; }
Expand All @@ -492,6 +501,7 @@ UTEST_F(Span, NoLogMixed) {
{ tracing::Span a{kIgnorePrefix1}; }
{ tracing::Span a{kIgnorePrefix2}; }
{ tracing::Span a{kIgnoreSpan}; }
// clang-format on

logging::LogFlush();

Expand Down Expand Up @@ -689,4 +699,42 @@ UTEST_F(Span, MakeSpanWithParentIdTraceIdLinkWithExisting) {
}
}

UTEST_F(Span, MakeSpanEvent) {
{
tracing::Span root_span("root_span");

auto error_span = root_span.CreateChild("error_span");
error_span.AddEvent("error_event");
}

logging::LogFlush();

const auto logs_raw = GetStreamString();

EXPECT_THAT(logs_raw, HasSubstr("events={\"error_event\":"));
EXPECT_THAT(logs_raw, HasSubstr("root_span"));
EXPECT_THAT(logs_raw, HasSubstr("error_span"));
}

UTEST_F(Span, MakeSpanSetStatus) {
{
tracing::Span error_span("error_span");
error_span.SetStatus(tracing::Span::StatusCode::kError, "Error description");
tracing::Span ok_span("ok_span");
ok_span.SetStatus(tracing::Span::StatusCode::kOk, "Ok description");
}

logging::LogFlush();

const auto logs_raw = GetStreamString();

EXPECT_THAT(logs_raw, HasSubstr("error_span"));
EXPECT_THAT(logs_raw, HasSubstr("otel_status_description=Error description"));
EXPECT_THAT(logs_raw, HasSubstr("otel_status_code=ERROR"));

EXPECT_THAT(logs_raw, HasSubstr("ok_span"));
EXPECT_THAT(logs_raw, HasSubstr("otel_status_description=Ok description"));
EXPECT_THAT(logs_raw, HasSubstr("otel_status_code=OK"));
}

USERVER_NAMESPACE_END
9 changes: 9 additions & 0 deletions otlp/src/otlp/logs/component.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,15 @@ LoggerComponent::LoggerComponent(const components::ComponentConfig& config, cons
logger_config.extra_attributes = config["extra-attributes"].As<std::unordered_map<std::string, std::string>>({});
logger_config.attributes_mapping =
config["attributes-mapping"].As<std::unordered_map<std::string, std::string>>({});

// Define error mapping
if (logger_config.attributes_mapping.find("otel_status_code") == logger_config.attributes_mapping.end()) {
Copy link
Member

Choose a reason for hiding this comment

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

use constants for otel_status_code and otel_status_description please
also, probably, you should add this to component header documentation

logger_config.attributes_mapping["otel_status_code"] = "otel.status_code";
}
if (logger_config.attributes_mapping.find("otel_status_description") == logger_config.attributes_mapping.end()) {
logger_config.attributes_mapping["otel_status_description"] = "otel.status_description";
}

logger_config.logs_sink = config["sinks"]["logs"].As<SinkType>(SinkType::kOtlp);
logger_config.tracing_sink = config["sinks"]["tracing"].As<SinkType>(SinkType::kOtlp);

Expand Down
60 changes: 53 additions & 7 deletions otlp/src/otlp/logs/logger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <iostream>

#include <userver/engine/async.hpp>
#include <userver/formats/json.hpp>
#include <userver/formats/parse/common_containers.hpp>
#include <userver/formats/parse/to.hpp>
#include <userver/logging/impl/tag_writer.hpp>
Expand All @@ -21,11 +22,40 @@ USERVER_NAMESPACE_BEGIN
namespace otlp {

namespace {

constexpr std::string_view kTelemetrySdkLanguage = "telemetry.sdk.language";
constexpr std::string_view kTelemetrySdkName = "telemetry.sdk.name";
constexpr std::string_view kServiceName = "service.name";

const std::string kTimestampFormat = "%Y-%m-%dT%H:%M:%E*S";

std::vector<tracing::Span::Event> GetEventsFromValue(const std::string_view value) {
std::vector<tracing::Span::Event> events;

const auto json_value = formats::json::FromString(value);

if (!json_value.IsObject()) {
throw std::runtime_error("Expected JSON object in \"value\"");
}

Copy link
Member

Choose a reason for hiding this comment

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

you can add reserve here with json_value.GetSize()

for (const auto& [key, value] : formats::common::Items(json_value)) {
events.emplace_back(key, value.As<uint64_t>());
}

return events;
}

void WriteEventsFromValue(::opentelemetry::proto::trace::v1::Span& span, std::string_view value) {
const std::vector<tracing::Span::Event> events = GetEventsFromValue(value);
span.mutable_events()->Reserve(events.size());

fdr400 marked this conversation as resolved.
Show resolved Hide resolved
for (const auto& event : events) {
auto* event_proto = span.add_events();
event_proto->set_name(event.name);
event_proto->set_time_unix_nano(event.time_unix_nano);
}
}

} // namespace

SinkType Parse(const yaml_config::YamlConfig& value, formats::parse::To<SinkType>) {
Expand Down Expand Up @@ -93,8 +123,9 @@ void Logger::Log(logging::Level level, std::string_view msg) {

++stats_.by_level[static_cast<int>(level)];

[[maybe_unused]] auto parse_ok =
utils::encoding::TskvReadRecord(parser, [&](std::string_view key, std::string_view value) {
[[maybe_unused]] auto parse_ok = utils::encoding::TskvReadRecord(
parser,
[this, &log_record, &timestamp](std::string_view key, std::string_view value) {
if (key == "text") {
log_record.mutable_body()->set_string_value(grpc::string(std::string{value}));
return true;
Expand All @@ -120,7 +151,8 @@ void Logger::Log(logging::Level level, std::string_view msg) {
attributes->set_key(std::string{MapAttribute(key)});
attributes->mutable_value()->set_string_value(std::string{value});
return true;
});
}
);

auto nanoseconds = std::chrono::duration_cast<std::chrono::nanoseconds>(timestamp.time_since_epoch());
log_record.set_time_unix_nano(nanoseconds.count());
Expand All @@ -147,8 +179,9 @@ void Logger::Trace(logging::Level level, std::string_view msg) {
std::string start_timestamp;
std::string total_time;

[[maybe_unused]] auto parse_ok =
utils::encoding::TskvReadRecord(parser, [&](std::string_view key, std::string_view value) {
[[maybe_unused]] auto parse_ok = utils::encoding::TskvReadRecord(
nepridumalnik marked this conversation as resolved.
Show resolved Hide resolved
parser,
[this, &span, &start_timestamp, &total_time](std::string_view key, std::string_view value) {
if (key == "trace_id") {
span.set_trace_id(utils::encoding::FromHex(value));
return true;
Expand Down Expand Up @@ -176,12 +209,24 @@ void Logger::Trace(logging::Level level, std::string_view msg) {
if (key == "timestamp" || key == "text") {
return true;
}
if (key == "events") {
WriteEventsFromValue(span, value);
return true;
}
if (key == "error") {
Copy link
Member

Choose a reason for hiding this comment

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

so, if error key is a special keys, maybe add assert that it is not in attributes mapping?

auto attributes = span.add_attributes();
attributes->set_key(std::string{key});
attributes->mutable_value()->set_bool_value(value == "true");

return true;
}

auto attributes = span.add_attributes();
attributes->set_key(std::string{MapAttribute(key)});
attributes->mutable_value()->set_string_value(std::string{value});
return true;
});
}
);

auto start_timestamp_double = std::stod(start_timestamp);
span.set_start_time_unix_nano(start_timestamp_double * 1'000'000'000);
Expand Down Expand Up @@ -226,7 +271,8 @@ void Logger::SendingLoop(Queue::Consumer& consumer, LogClient& log_client, Trace
[&scope_logs](const opentelemetry::proto::logs::v1::LogRecord& action) {
auto log_records = scope_logs->add_log_records();
*log_records = action;
}},
}
},
action
);
} while (consumer.Pop(action, deadline));
Expand Down
Loading