diff --git a/src/Interpreters/ClientInfo.cpp b/src/Interpreters/ClientInfo.cpp index f021d989158..4dbec8af3a4 100644 --- a/src/Interpreters/ClientInfo.cpp +++ b/src/Interpreters/ClientInfo.cpp @@ -155,72 +155,6 @@ void ClientInfo::setInitialQuery() client_name = (DBMS_NAME " ") + client_name; } -bool OpenTelemetryTraceContext::parseTraceparentHeader(const std::string & traceparent, - std::string & error) -{ - trace_id = 0; - - uint8_t version = -1; - uint64_t trace_id_high = 0; - uint64_t trace_id_low = 0; - - // Version 00, which is the only one we can parse, is fixed width. Use this - // fact for an additional sanity check. - const int expected_length = 2 + 1 + 32 + 1 + 16 + 1 + 2; - if (traceparent.length() != expected_length) - { - error = fmt::format("unexpected length {}, expected {}", - traceparent.length(), expected_length); - return false; - } - - // clang-tidy doesn't like sscanf: - // error: 'sscanf' used to convert a string to an unsigned integer value, - // but function will not report conversion errors; consider using 'strtoul' - // instead [cert-err34-c,-warnings-as-errors] - // There is no other ready solution, and hand-rolling a more complicated - // parser for an HTTP header in C++ sounds like RCE. - // NOLINTNEXTLINE(cert-err34-c) - int result = sscanf(&traceparent[0], - "%2" SCNx8 "-%16" SCNx64 "%16" SCNx64 "-%16" SCNx64 "-%2" SCNx8, - &version, &trace_id_high, &trace_id_low, &span_id, &trace_flags); - - if (result == EOF) - { - error = "EOF"; - return false; - } - - // We read uint128 as two uint64, so 5 parts and not 4. - if (result != 5) - { - error = fmt::format("could only read {} parts instead of the expected 5", - result); - return false; - } - - if (version != 0) - { - error = fmt::format("unexpected version {}, expected 00", version); - return false; - } - - trace_id = static_cast<__uint128_t>(trace_id_high) << 64 - | trace_id_low; - return true; -} - - -std::string OpenTelemetryTraceContext::composeTraceparentHeader() const -{ - // This span is a parent for its children, so we specify this span_id as a - // parent id. - return fmt::format("00-{:032x}-{:016x}-{:02x}", trace_id, - span_id, - // This cast is needed because fmt is being weird and complaining that - // "mixing character types is not allowed". - static_cast(trace_flags)); -} void ClientInfo::fillOSUserHostNameAndVersionInfo() { diff --git a/src/Interpreters/InterpreterFactory.cpp b/src/Interpreters/InterpreterFactory.cpp index 7505c017953..30992905f5d 100644 --- a/src/Interpreters/InterpreterFactory.cpp +++ b/src/Interpreters/InterpreterFactory.cpp @@ -94,7 +94,7 @@ namespace ErrorCodes std::unique_ptr InterpreterFactory::get(ASTPtr & query, Context & context, QueryProcessingStage::Enum stage) { - OpenTelemetrySpanHolder span(__FUNCTION__); + OpenTelemetrySpanHolder span("InterpreterFactory::get()"); ProfileEvents::increment(ProfileEvents::Query); diff --git a/src/Interpreters/InterpreterSelectQuery.cpp b/src/Interpreters/InterpreterSelectQuery.cpp index 2eee269efe1..949d4e7b0d4 100644 --- a/src/Interpreters/InterpreterSelectQuery.cpp +++ b/src/Interpreters/InterpreterSelectQuery.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include @@ -497,6 +498,8 @@ BlockIO InterpreterSelectQuery::execute() Block InterpreterSelectQuery::getSampleBlockImpl() { + OpenTelemetrySpanHolder span(__PRETTY_FUNCTION__); + query_info.query = query_ptr; if (storage && !options.only_analyze) diff --git a/src/Interpreters/OpenTelemetrySpanLog.cpp b/src/Interpreters/OpenTelemetrySpanLog.cpp index 853428cf7b9..6edfb1f8584 100644 --- a/src/Interpreters/OpenTelemetrySpanLog.cpp +++ b/src/Interpreters/OpenTelemetrySpanLog.cpp @@ -28,6 +28,7 @@ Block OpenTelemetrySpanLogElement::createBlock() }; } + void OpenTelemetrySpanLogElement::appendToBlock(MutableColumns & columns) const { size_t i = 0; @@ -52,6 +53,7 @@ void OpenTelemetrySpanLogElement::appendToBlock(MutableColumns & columns) const columns[i++]->insert(string_values); } + OpenTelemetrySpanHolder::OpenTelemetrySpanHolder(const std::string & _operation_name) { trace_id = 0; @@ -78,13 +80,15 @@ OpenTelemetrySpanHolder::OpenTelemetrySpanHolder(const std::string & _operation_ start_time_us = std::chrono::duration_cast( std::chrono::system_clock::now().time_since_epoch()).count(); - // ****** remove this +#ifndef NDEBUG attribute_names.push_back("clickhouse.start.stacktrace"); attribute_values.push_back(StackTrace().toString()); +#endif thread.thread_trace_context.span_id = span_id; } + OpenTelemetrySpanHolder::~OpenTelemetrySpanHolder() { try @@ -116,11 +120,10 @@ OpenTelemetrySpanHolder::~OpenTelemetrySpanHolder() return; } - //******** remove this - attribute_names.push_back("clickhouse.query_id"); - attribute_values.push_back(context->getCurrentQueryId()); +#ifndef NDEBUG attribute_names.push_back("clickhouse.end.stacktrace"); attribute_values.push_back(StackTrace().toString()); +#endif auto log = context->getOpenTelemetrySpanLog(); if (!log) @@ -146,5 +149,74 @@ OpenTelemetrySpanHolder::~OpenTelemetrySpanHolder() } } + +bool OpenTelemetryTraceContext::parseTraceparentHeader(const std::string & traceparent, + std::string & error) +{ + trace_id = 0; + + uint8_t version = -1; + uint64_t trace_id_high = 0; + uint64_t trace_id_low = 0; + + // Version 00, which is the only one we can parse, is fixed width. Use this + // fact for an additional sanity check. + const int expected_length = 2 + 1 + 32 + 1 + 16 + 1 + 2; + if (traceparent.length() != expected_length) + { + error = fmt::format("unexpected length {}, expected {}", + traceparent.length(), expected_length); + return false; + } + + // clang-tidy doesn't like sscanf: + // error: 'sscanf' used to convert a string to an unsigned integer value, + // but function will not report conversion errors; consider using 'strtoul' + // instead [cert-err34-c,-warnings-as-errors] + // There is no other ready solution, and hand-rolling a more complicated + // parser for an HTTP header in C++ sounds like RCE. + // NOLINTNEXTLINE(cert-err34-c) + int result = sscanf(&traceparent[0], + "%2" SCNx8 "-%16" SCNx64 "%16" SCNx64 "-%16" SCNx64 "-%2" SCNx8, + &version, &trace_id_high, &trace_id_low, &span_id, &trace_flags); + + if (result == EOF) + { + error = "EOF"; + return false; + } + + // We read uint128 as two uint64, so 5 parts and not 4. + if (result != 5) + { + error = fmt::format("could only read {} parts instead of the expected 5", + result); + return false; + } + + if (version != 0) + { + error = fmt::format("unexpected version {}, expected 00", version); + return false; + } + + trace_id = static_cast<__uint128_t>(trace_id_high) << 64 + | trace_id_low; + return true; +} + + +std::string OpenTelemetryTraceContext::composeTraceparentHeader() const +{ + // This span is a parent for its children, so we specify this span_id as a + // parent id. + return fmt::format("00-{:032x}-{:016x}-{:02x}", trace_id, + span_id, + // This cast is needed because fmt is being weird and complaining that + // "mixing character types is not allowed". + static_cast(trace_flags)); +} + + } diff --git a/src/Interpreters/ThreadStatusExt.cpp b/src/Interpreters/ThreadStatusExt.cpp index c9742615e03..4e8debe410b 100644 --- a/src/Interpreters/ThreadStatusExt.cpp +++ b/src/Interpreters/ThreadStatusExt.cpp @@ -341,8 +341,11 @@ void ThreadStatus::detachQuery(bool exit_if_already_detached, bool thread_exits) OpenTelemetrySpanLogElement span; span.trace_id = thread_trace_context.trace_id; - // Might be problematic if some span holder isn't finished by the time - // we detach this thread... + // All child span holders should be finished by the time we detach this + // thread, so the current span id should be the thread span id. If not, + // an assertion for a proper parent span in ~OpenTelemetrySpanHolder() + // is going to fail, because we're going to reset it to zero later in + // this function. span.span_id = thread_trace_context.span_id; span.parent_span_id = query_context->query_trace_context.span_id; span.operation_name = getThreadName(); @@ -355,6 +358,11 @@ void ThreadStatus::detachQuery(bool exit_if_already_detached, bool thread_exits) span.attribute_names.push_back("clickhouse.thread_id"); span.attribute_values.push_back(thread_id); +#ifndef NDEBUG + span.attribute_names.push_back("clickhouse.end.stacktrace"); + span.attribute_values.push_back(StackTrace().toString()); +#endif + opentelemetry_span_log->add(span); } diff --git a/src/Interpreters/executeQuery.cpp b/src/Interpreters/executeQuery.cpp index b0ffea59037..610dd9217ff 100644 --- a/src/Interpreters/executeQuery.cpp +++ b/src/Interpreters/executeQuery.cpp @@ -478,7 +478,7 @@ static std::tuple executeQueryImpl( } { - OpenTelemetrySpanHolder span("execute interpreter"); + OpenTelemetrySpanHolder span("IInterpreter::execute()"); res = interpreter->execute(); } diff --git a/src/Processors/Executors/PipelineExecutor.cpp b/src/Processors/Executors/PipelineExecutor.cpp index f940ea148c7..517e07a3ba4 100644 --- a/src/Processors/Executors/PipelineExecutor.cpp +++ b/src/Processors/Executors/PipelineExecutor.cpp @@ -76,7 +76,6 @@ static void executeJob(IProcessor * processor) { try { - OpenTelemetrySpanHolder span(demangle(typeid(*processor).name())); processor->work(); } catch (Exception & exception) @@ -694,6 +693,8 @@ void PipelineExecutor::initializeExecution(size_t num_threads) void PipelineExecutor::executeImpl(size_t num_threads) { + OpenTelemetrySpanHolder span("PipelineExecutor::executeImpl()"); + initializeExecution(num_threads); using ThreadsData = std::vector; diff --git a/src/Server/TCPHandler.cpp b/src/Server/TCPHandler.cpp index 37a0f12bf8d..1f55a3af635 100644 --- a/src/Server/TCPHandler.cpp +++ b/src/Server/TCPHandler.cpp @@ -518,7 +518,7 @@ void TCPHandler::processInsertQuery(const Settings & connection_settings) void TCPHandler::processOrdinaryQuery() { - OpenTelemetrySpanHolder span(__FUNCTION__); + OpenTelemetrySpanHolder span(__PRETTY_FUNCTION__); /// Pull query execution result, if exists, and send it to network. if (state.io.in)