From 8ee3fbf517b521bca64ccf3c66a7e01d19a1e667 Mon Sep 17 00:00:00 2001 From: Graham Pople Date: Sun, 22 Oct 2023 02:32:39 +0100 Subject: [PATCH] CXXCBC-387: Optimising tags for noop_tracer (#461) * 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 --- core/io/http_command.hxx | 15 ++++++++++----- core/io/mcbp_command.hxx | 24 ++++++++++++++++-------- core/tracing/noop_tracer.hxx | 5 +++++ couchbase/tracing/request_span.hxx | 5 +++++ 4 files changed, 36 insertions(+), 13 deletions(-) diff --git a/core/io/http_command.hxx b/core/io/http_command.hxx index 58b3050ce..0a5f0897d 100644 --- a/core/io/http_command.hxx +++ b/core/io/http_command.hxx @@ -74,8 +74,10 @@ struct http_command : public std::enable_shared_from_this> 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; } @@ -83,8 +85,10 @@ struct http_command : public std::enable_shared_from_this> 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) { @@ -123,7 +127,8 @@ struct http_command : public std::enable_shared_from_this> 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(); } diff --git a/core/io/mcbp_command.hxx b/core/io/mcbp_command.hxx index 5e19eca5a..d160d5f6f 100644 --- a/core/io/mcbp_command.hxx +++ b/core/io/mcbp_command.hxx @@ -93,8 +93,10 @@ struct mcbp_command : public std::enable_shared_from_thistracer()->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_); @@ -199,7 +201,8 @@ struct mcbp_command : public std::enable_shared_from_thisnext_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()); @@ -249,13 +252,15 @@ struct mcbp_command : public std::enable_shared_from_thisretry_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); @@ -314,9 +319,12 @@ struct mcbp_command : public std::enable_shared_from_thisadd_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(); } }; diff --git a/core/tracing/noop_tracer.hxx b/core/tracing/noop_tracer.hxx index 6144918e5..45879e0ec 100644 --- a/core/tracing/noop_tracer.hxx +++ b/core/tracing/noop_tracer.hxx @@ -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 diff --git a/couchbase/tracing/request_span.hxx b/couchbase/tracing/request_span.hxx index ec99929e8..63832bf2a 100644 --- a/couchbase/tracing/request_span.hxx +++ b/couchbase/tracing/request_span.hxx @@ -56,6 +56,11 @@ class request_span return parent_; } + virtual bool uses_tags() const + { + return true; + } + private: std::string name_{}; std::shared_ptr parent_{ nullptr };