Add span events and status #794
Conversation
|
Now that works! |
…lnik/userver into span_events_and_status
…into span_events_and_status
| @@ -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); | |||
There was a problem hiding this comment.
How do you plan to add attributes to event class?
Maybe it is better to pass Span::Event in this method?
| if (!json_value.IsObject()) { | ||
| throw std::runtime_error("Expected JSON object in \"value\""); | ||
| } | ||
|
|
There was a problem hiding this comment.
you can add reserve here with json_value.GetSize()
| @@ -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()) { | |||
There was a problem hiding this comment.
use constants for otel_status_code and otel_status_description please
also, probably, you should add this to component header documentation
| @@ -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; | |||
There was a problem hiding this comment.
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();
}| 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"); | ||
| } |
There was a problem hiding this comment.
place the implementation to anonymous namespace please
| @@ -33,6 +33,34 @@ class Span final { | |||
| public: | |||
| class Impl; | |||
|
|
|||
| /// @brief Operation status code. | |||
There was a problem hiding this comment.
/// @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
| kError, // The operation contains an error. | ||
| }; | ||
|
|
||
| /// @brief Span event. |
There was a problem hiding this comment.
/// @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.
| void AddEvent(std::string_view event_name); | ||
|
|
||
| /// Set span status. | ||
| void SetStatus(StatusCode status, const std::string_view description); |
There was a problem hiding this comment.
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?
| WriteEventsFromValue(span, value); | ||
| return true; | ||
| } | ||
| if (key == "error") { |
There was a problem hiding this comment.
so, if error key is a special keys, maybe add assert that it is not in attributes mapping?
In this PR, I added events and statuses to the Spans
Note: by creating a PR or an issue you automatically agree to the CLA. See CONTRIBUTING.md. Feel free to remove this note, the agreement holds.