Skip to content

Commit

Permalink
CXXCBC-387: Optimising tags for noop_tracer (#461)
Browse files Browse the repository at this point in the history
* CXXCBC-387: Performance: potential micro-optimisation on tags and local/remote sessions

Profiling indicates that non-trivial time is spent assigning
tags to spans, which is wasted effort if noop_tracer is being
used.

Borrowing an optimisation from the JVM SDKs, where we skip this
assignment in this case.

This optimisation, together with another small one that will be
submitted separately, produced around a 15% performance improvement.

* Clang and sanitizer
  • Loading branch information
programmatix authored Oct 22, 2023
1 parent f9cac2f commit 8ee3fbf
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 13 deletions.
15 changes: 10 additions & 5 deletions core/io/http_command.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -74,17 +74,21 @@ struct http_command : public std::enable_shared_from_this<http_command<Request>>
if (span_ == nullptr) {
return;
}
span_->add_tag(tracing::attributes::remote_socket, remote_address);
span_->add_tag(tracing::attributes::local_socket, local_address);
if (span_->uses_tags())
span_->add_tag(tracing::attributes::remote_socket, remote_address);
if (span_->uses_tags())
span_->add_tag(tracing::attributes::local_socket, local_address);
span_->end();
span_ = nullptr;
}

void start(http_command_handler&& handler)
{
span_ = tracer_->start_span(tracing::span_name_for_http_service(request.type), parent_span);
span_->add_tag(tracing::attributes::service, tracing::service_name_for_http_service(request.type));
span_->add_tag(tracing::attributes::operation_id, client_context_id_);
if (span_->uses_tags())
span_->add_tag(tracing::attributes::service, tracing::service_name_for_http_service(request.type));
if (span_->uses_tags())
span_->add_tag(tracing::attributes::operation_id, client_context_id_);
handler_ = std::move(handler);
deadline.expires_after(timeout_);
deadline.async_wait([self = this->shared_from_this()](std::error_code ec) {
Expand Down Expand Up @@ -123,7 +127,8 @@ struct http_command : public std::enable_shared_from_this<http_command<Request>>
return;
}
session_ = std::move(session);
span_->add_tag(tracing::attributes::local_id, session_->id());
if (span_->uses_tags())
span_->add_tag(tracing::attributes::local_id, session_->id());
send();
}

Expand Down
24 changes: 16 additions & 8 deletions core/io/mcbp_command.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,10 @@ struct mcbp_command : public std::enable_shared_from_this<mcbp_command<Manager,
void start(mcbp_command_handler&& handler)
{
span_ = manager_->tracer()->start_span(tracing::span_name_for_mcbp_command(encoded_request_type::body_type::opcode), parent_span);
span_->add_tag(tracing::attributes::service, tracing::service::key_value);
span_->add_tag(tracing::attributes::instance, request.id.bucket());
if (span_->uses_tags())
span_->add_tag(tracing::attributes::service, tracing::service::key_value);
if (span_->uses_tags())
span_->add_tag(tracing::attributes::instance, request.id.bucket());

handler_ = std::move(handler);
deadline.expires_after(timeout_);
Expand Down Expand Up @@ -199,7 +201,8 @@ struct mcbp_command : public std::enable_shared_from_this<mcbp_command<Manager,
{
opaque_ = session_->next_opaque();
request.opaque = *opaque_;
span_->add_tag(tracing::attributes::operation_id, fmt::format("0x{:x}", request.opaque));
if (span_->uses_tags())
span_->add_tag(tracing::attributes::operation_id, fmt::format("0x{:x}", request.opaque));
if (request.id.use_collections() && !request.id.is_collection_resolved()) {
if (session_->supports_feature(protocol::hello_feature::collections)) {
auto collection_id = session_->get_collection_uid(request.id.collection_path());
Expand Down Expand Up @@ -249,13 +252,15 @@ struct mcbp_command : public std::enable_shared_from_this<mcbp_command<Manager,

self->retry_backoff.cancel();
if (ec == asio::error::operation_aborted) {
self->span_->add_tag(tracing::attributes::orphan, "aborted");
if (self->span_->uses_tags())
self->span_->add_tag(tracing::attributes::orphan, "aborted");
return self->invoke_handler(make_error_code(self->request.retries.idempotent() ? errc::common::unambiguous_timeout
: errc::common::ambiguous_timeout));
}
if (ec == errc::common::request_canceled) {
if (reason == retry_reason::do_not_retry) {
self->span_->add_tag(tracing::attributes::orphan, "canceled");
if (self->span_->uses_tags())
self->span_->add_tag(tracing::attributes::orphan, "canceled");
return self->invoke_handler(ec);
}
return io::retry_orchestrator::maybe_retry(self->manager_, self, reason, ec);
Expand Down Expand Up @@ -314,9 +319,12 @@ struct mcbp_command : public std::enable_shared_from_this<mcbp_command<Manager,
return;
}
session_ = std::move(session);
span_->add_tag(tracing::attributes::remote_socket, session_->remote_address());
span_->add_tag(tracing::attributes::local_socket, session_->local_address());
span_->add_tag(tracing::attributes::local_id, session_->id());
if (span_->uses_tags())
span_->add_tag(tracing::attributes::remote_socket, session_->remote_address());
if (span_->uses_tags())
span_->add_tag(tracing::attributes::local_socket, session_->local_address());
if (span_->uses_tags())
span_->add_tag(tracing::attributes::local_id, session_->id());
send();
}
};
Expand Down
5 changes: 5 additions & 0 deletions core/tracing/noop_tracer.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ class noop_span : public couchbase::tracing::request_span
{
/* do nothing */
}

bool uses_tags() const override
{
return false;
}
};

class noop_tracer : public couchbase::tracing::request_tracer
Expand Down
5 changes: 5 additions & 0 deletions couchbase/tracing/request_span.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ class request_span
return parent_;
}

virtual bool uses_tags() const
{
return true;
}

private:
std::string name_{};
std::shared_ptr<request_span> parent_{ nullptr };
Expand Down

0 comments on commit 8ee3fbf

Please sign in to comment.