From 05ad9b9fffe1a49cf91eaf3e5d0df89e42d55124 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Thu, 20 Aug 2020 23:59:40 +0300 Subject: [PATCH 001/145] opentelemetry wip --- base/daemon/BaseDaemon.cpp | 2 +- programs/server/config.xml | 10 ++++ src/Common/Exception.h | 7 ++- src/Interpreters/ClientInfo.h | 5 ++ src/Interpreters/Context.cpp | 53 +++++++++++------- src/Interpreters/Context.h | 2 + src/Interpreters/InterpreterSystemQuery.cpp | 4 +- src/Interpreters/OpenTelemetryLog.cpp | 46 ++++++++++++++++ src/Interpreters/OpenTelemetryLog.h | 57 ++++++++++++++++++++ src/Interpreters/SystemLog.cpp | 6 +++ src/Interpreters/SystemLog.h | 3 ++ src/Interpreters/executeQuery.cpp | 4 ++ src/Storages/StorageFile.cpp | 59 ++++++++++++++++++++- 13 files changed, 233 insertions(+), 25 deletions(-) create mode 100644 src/Interpreters/OpenTelemetryLog.cpp create mode 100644 src/Interpreters/OpenTelemetryLog.h diff --git a/base/daemon/BaseDaemon.cpp b/base/daemon/BaseDaemon.cpp index 78801e71a6f..cc5a765a52d 100644 --- a/base/daemon/BaseDaemon.cpp +++ b/base/daemon/BaseDaemon.cpp @@ -153,7 +153,7 @@ static void signalHandler(int sig, siginfo_t * info, void * context) if (sig != SIGTSTP) /// This signal is used for debugging. { /// The time that is usually enough for separate thread to print info into log. - sleepForSeconds(10); + sleepForSeconds(1); call_default_signal_handler(sig); } diff --git a/programs/server/config.xml b/programs/server/config.xml index af01e880dc2..7e88150b95f 100644 --- a/programs/server/config.xml +++ b/programs/server/config.xml @@ -563,6 +563,16 @@ 60000 + + + system + opentelemetry_log
+ 7500 + + + diff --git a/src/Common/Exception.h b/src/Common/Exception.h index 763b90048bb..d0de8d6a3f2 100644 --- a/src/Common/Exception.h +++ b/src/Common/Exception.h @@ -39,8 +39,11 @@ public: const char * name() const throw() override { return "DB::Exception"; } const char * what() const throw() override { return message().data(); } - /// Add something to the existing message. - void addMessage(const std::string & arg) { extendedMessage(arg); } + template + void addMessage(Fmt&&... fmt) + { + extendedMessage(fmt::format(std::forward(fmt)...)); + } std::string getStackTraceString() const; diff --git a/src/Interpreters/ClientInfo.h b/src/Interpreters/ClientInfo.h index f3a99112170..575d1521b6f 100644 --- a/src/Interpreters/ClientInfo.h +++ b/src/Interpreters/ClientInfo.h @@ -57,6 +57,11 @@ public: String initial_user; String initial_query_id; Poco::Net::SocketAddress initial_address; + + + UInt128 trace_id; + UInt64 span_id; + UInt64 parent_span_id; /// All below are parameters related to initial query. diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 70cf41a679c..93e8ac49114 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -1075,35 +1075,37 @@ void Context::setCurrentQueryId(const String & query_id) if (!client_info.current_query_id.empty()) throw Exception("Logical error: attempt to set query_id twice", ErrorCodes::LOGICAL_ERROR); - String query_id_to_set = query_id; + /// Generate random UUID, but using lower quality RNG, + /// because Poco::UUIDGenerator::generateRandom method is using /dev/random, that is very expensive. + /// NOTE: Actually we don't need to use UUIDs for query identifiers. + /// We could use any suitable string instead. + union + { + char bytes[16]; + struct + { + UInt64 a; + UInt64 b; + } words; + UInt128 uuid; + } random; + random.words.a = thread_local_rng(); //-V656 + random.words.b = thread_local_rng(); //-V656 + + trace_id = random.uuid; + + + String query_id_to_set = query_id; if (query_id_to_set.empty()) /// If the user did not submit his query_id, then we generate it ourselves. { - /// Generate random UUID, but using lower quality RNG, - /// because Poco::UUIDGenerator::generateRandom method is using /dev/random, that is very expensive. - /// NOTE: Actually we don't need to use UUIDs for query identifiers. - /// We could use any suitable string instead. - - union - { - char bytes[16]; - struct - { - UInt64 a; - UInt64 b; - } words; - } random; - - random.words.a = thread_local_rng(); //-V656 - random.words.b = thread_local_rng(); //-V656 - /// Use protected constructor. struct QueryUUID : Poco::UUID { QueryUUID(const char * bytes, Poco::UUID::Version version) : Poco::UUID(bytes, version) {} }; - + query_id_to_set = QueryUUID(random.bytes, Poco::UUID::UUID_RANDOM).toString(); } @@ -1705,6 +1707,17 @@ std::shared_ptr Context::getAsynchronousMetricLog() } +std::shared_ptr Context::getOpenTelemetryLog() +{ + auto lock = getLock(); + + if (!shared->system_logs) + return {}; + + return shared->system_logs->opentelemetry_log; +} + + CompressionCodecPtr Context::chooseCompressionCodec(size_t part_size, double part_size_ratio) const { auto lock = getLock(); diff --git a/src/Interpreters/Context.h b/src/Interpreters/Context.h index c8d13baa9ae..6cabac3db0d 100644 --- a/src/Interpreters/Context.h +++ b/src/Interpreters/Context.h @@ -81,6 +81,7 @@ class TextLog; class TraceLog; class MetricLog; class AsynchronousMetricLog; +class OpenTelemetrySpanLog; struct MergeTreeSettings; class StorageS3Settings; class IDatabase; @@ -531,6 +532,7 @@ public: std::shared_ptr getTextLog(); std::shared_ptr getMetricLog(); std::shared_ptr getAsynchronousMetricLog(); + std::shared_ptr getOpenTelemetryLog(); /// Returns an object used to log operations with parts if it possible. /// Provide table name to make required checks. diff --git a/src/Interpreters/InterpreterSystemQuery.cpp b/src/Interpreters/InterpreterSystemQuery.cpp index 4bfa84090c2..24fb0637b3d 100644 --- a/src/Interpreters/InterpreterSystemQuery.cpp +++ b/src/Interpreters/InterpreterSystemQuery.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -321,7 +322,8 @@ BlockIO InterpreterSystemQuery::execute() [&] () { if (auto trace_log = context.getTraceLog()) trace_log->flush(true); }, [&] () { if (auto text_log = context.getTextLog()) text_log->flush(true); }, [&] () { if (auto metric_log = context.getMetricLog()) metric_log->flush(true); }, - [&] () { if (auto asynchronous_metric_log = context.getAsynchronousMetricLog()) asynchronous_metric_log->flush(true); } + [&] () { if (auto asynchronous_metric_log = context.getAsynchronousMetricLog()) asynchronous_metric_log->flush(true); }, + [&] () { if (auto opentelemetry_log = context.getOpenTelemetryLog()) opentelemetry_log->flush(true); } ); break; case Type::STOP_LISTEN_QUERIES: diff --git a/src/Interpreters/OpenTelemetryLog.cpp b/src/Interpreters/OpenTelemetryLog.cpp new file mode 100644 index 00000000000..511c820797a --- /dev/null +++ b/src/Interpreters/OpenTelemetryLog.cpp @@ -0,0 +1,46 @@ +#include "OpenTelemetryLog.h" + +#include +#include +#include +#include +#include +#include + +namespace DB +{ + +Block OpenTelemetrySpanLogElement::createBlock() +{ + return { + {std::make_shared(), "trace_id"}, + {std::make_shared(), "span_id"}, + {std::make_shared(), "parent_span_id"}, + {std::make_shared(), "operation_name"}, + {std::make_shared(), "start_date"}, + {std::make_shared(), "start_time"}, + {std::make_shared(), "finish_time"}, + {std::make_shared(std::make_shared()), + "attribute.names"}, + {std::make_shared(std::make_shared()), + "attribute.values"} + }; +} + +void OpenTelemetrySpanLogElement::appendToBlock(MutableColumns & columns) const +{ + size_t i = 0; + + columns[i++]->insert(trace_id); + columns[i++]->insert(span_id); + columns[i++]->insert(parent_span_id); + columns[i++]->insert(operation_name); + columns[i++]->insert(DateLUT::instance().toDayNum(start_time)); + columns[i++]->insert(start_time); + columns[i++]->insert(finish_time); + columns[i++]->insert(attribute_names); + columns[i++]->insert(attribute_values); +} + +} + diff --git a/src/Interpreters/OpenTelemetryLog.h b/src/Interpreters/OpenTelemetryLog.h new file mode 100644 index 00000000000..723a4201783 --- /dev/null +++ b/src/Interpreters/OpenTelemetryLog.h @@ -0,0 +1,57 @@ +#pragma once + +#include + +namespace DB +{ + +/* +struct OpenTelemetrySpanContext +{ + UInt128 trace_id; + UInt64 span_id; + UInt8 trace_flags; + String trace_state; +}; +*/ + +// TODO figure out precisely which part of this is run time, and which part we +// must log. +struct OpenTelemetrySpan +{ + UInt128 trace_id; + UInt64 span_id; + UInt64 parent_span_id; + std::string operation_name; + time_t start_time{}; + time_t finish_time{}; + Array attribute_names; + Array attribute_values; + // I don't understand how Links work, namely, which direction should they + // point to, and how they are related with parent_span_id, so no Links for + // now. + + // The following fields look like something that is runtime only and doesn't + // require logging. + UInt8 trace_flags; + // Vendor-specific info, key-value pairs. Keep it as a string as described + // here: https://w3c.github.io/trace-context/#tracestate-header + String trace_state; +}; + +struct OpenTelemetrySpanLogElement : public OpenTelemetrySpan +{ + static std::string name() { return "OpenTelemetrySpanLog"; } + static Block createBlock(); + void appendToBlock(MutableColumns & columns) const; +}; + +// OpenTelemetry standartizes some Log data as well, so it's not just +// OpenTelemetryLog to avoid confusion. +class OpenTelemetrySpanLog : public SystemLog +{ +public: + using SystemLog::SystemLog; +}; + +} diff --git a/src/Interpreters/SystemLog.cpp b/src/Interpreters/SystemLog.cpp index e64444edd67..27236c99a23 100644 --- a/src/Interpreters/SystemLog.cpp +++ b/src/Interpreters/SystemLog.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -88,6 +89,9 @@ SystemLogs::SystemLogs(Context & global_context, const Poco::Util::AbstractConfi asynchronous_metric_log = createSystemLog( global_context, "system", "asynchronous_metric_log", config, "asynchronous_metric_log"); + opentelemetry_log = createSystemLog( + global_context, "system", "opentelemetry_log", config, + "opentelemetry_log"); if (query_log) logs.emplace_back(query_log.get()); @@ -105,6 +109,8 @@ SystemLogs::SystemLogs(Context & global_context, const Poco::Util::AbstractConfi logs.emplace_back(metric_log.get()); if (asynchronous_metric_log) logs.emplace_back(asynchronous_metric_log.get()); + if (opentelemetry_log) + logs.emplace_back(opentelemetry_log.get()); try { diff --git a/src/Interpreters/SystemLog.h b/src/Interpreters/SystemLog.h index a2e97747d12..15ebf04c6ce 100644 --- a/src/Interpreters/SystemLog.h +++ b/src/Interpreters/SystemLog.h @@ -71,6 +71,7 @@ class TraceLog; class CrashLog; class MetricLog; class AsynchronousMetricLog; +class OpenTelemetrySpanLog; class ISystemLog @@ -105,6 +106,8 @@ struct SystemLogs std::shared_ptr metric_log; /// Used to log all metrics. /// Metrics from system.asynchronous_metrics. std::shared_ptr asynchronous_metric_log; + /// OpenTelemetry trace spans + std::shared_ptr opentelemetry_log; std::vector logs; }; diff --git a/src/Interpreters/executeQuery.cpp b/src/Interpreters/executeQuery.cpp index fa5b3e0bcc2..27ac5fda56f 100644 --- a/src/Interpreters/executeQuery.cpp +++ b/src/Interpreters/executeQuery.cpp @@ -584,6 +584,10 @@ static std::tuple executeQueryImpl( if (auto query_log = context.getQueryLog()) query_log->add(elem); } + + if (auto opentelemetry_log = context.getOpenTelemetryLog()) + { + } }; auto exception_callback = [elem, &context, ast, log_queries, log_queries_min_type = settings.log_queries_min_type, quota(quota), diff --git a/src/Storages/StorageFile.cpp b/src/Storages/StorageFile.cpp index 558216a6216..d22a0625a67 100644 --- a/src/Storages/StorageFile.cpp +++ b/src/Storages/StorageFile.cpp @@ -199,6 +199,45 @@ StorageFile::StorageFile(CommonArguments args) setInMemoryMetadata(storage_metadata); } +struct ScopedExceptionHint +{ + std::string hint; + + // Format message with fmt::format + template + ScopedExceptionHint(Fmt&&... fmt) + : hint(fmt::format(std::forward(fmt)...)) + { + fprintf(stderr, "hint %s created\n", hint.c_str()); + } + + ~ScopedExceptionHint() + { + fprintf(stderr, "hint %s is being destroyed\n", hint.c_str()); + std::exception_ptr exception = std::current_exception(); + if (exception) + { + try { + std::rethrow_exception(exception); + } + catch (Exception & e) + { + e.addMessage(hint); + fprintf(stderr, "added hint %s\n", hint.c_str()); + } + catch (...) + { + fprintf(stderr, "unknown exception\n"); + // pass? + } + } + else + { + fprintf(stderr, "no exception\n"); + } + } +}; + class StorageFileSource : public SourceWithProgress { public: @@ -317,10 +356,28 @@ public: if (!column_defaults.empty()) reader = std::make_shared(reader, column_defaults, context); + ScopedExceptionHint("Reading prefix of file '{}'", current_path); reader->readPrefix(); } - if (auto res = reader->read()) + ScopedExceptionHint("Reader->read() {}", reader->getName()); + + Block res; + try + { + res = reader->read(); + } + catch (Exception & e) + { + e.addMessage("Error while reading from '{}'", current_path); + throw; + } + catch (...) + { + throw; + } + + if (res) { Columns columns = res.getColumns(); UInt64 num_rows = res.rows(); From 54b3047d19d5aed63c59fe46215c70138052386d Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Thu, 27 Aug 2020 21:44:20 +0300 Subject: [PATCH 002/145] fixup --- src/Interpreters/ClientInfo.h | 3 +- src/Interpreters/Context.cpp | 7 ++-- src/Interpreters/OpenTelemetryLog.cpp | 13 +++++--- src/Interpreters/OpenTelemetryLog.h | 2 +- src/Interpreters/executeQuery.cpp | 48 +++++++++++++++++++++++++++ 5 files changed, 63 insertions(+), 10 deletions(-) diff --git a/src/Interpreters/ClientInfo.h b/src/Interpreters/ClientInfo.h index 575d1521b6f..bc6afec36f4 100644 --- a/src/Interpreters/ClientInfo.h +++ b/src/Interpreters/ClientInfo.h @@ -2,6 +2,7 @@ #include #include +#include namespace DB @@ -59,7 +60,7 @@ public: Poco::Net::SocketAddress initial_address; - UInt128 trace_id; + __uint128_t trace_id; UInt64 span_id; UInt64 parent_span_id; diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 93e8ac49114..c225c332248 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -1087,14 +1087,15 @@ void Context::setCurrentQueryId(const String & query_id) UInt64 a; UInt64 b; } words; - UInt128 uuid; + __uint128_t uuid; } random; random.words.a = thread_local_rng(); //-V656 random.words.b = thread_local_rng(); //-V656 - trace_id = random.uuid; - + client_info.trace_id = random.uuid; + client_info.span_id = 1; + client_info.parent_span_id = 0; String query_id_to_set = query_id; if (query_id_to_set.empty()) /// If the user did not submit his query_id, then we generate it ourselves. diff --git a/src/Interpreters/OpenTelemetryLog.cpp b/src/Interpreters/OpenTelemetryLog.cpp index 511c820797a..f8d7d684478 100644 --- a/src/Interpreters/OpenTelemetryLog.cpp +++ b/src/Interpreters/OpenTelemetryLog.cpp @@ -13,12 +13,15 @@ namespace DB Block OpenTelemetrySpanLogElement::createBlock() { return { + // event_date is the date part of event_time, used for indexing. + {std::make_shared(), "event_date"}, + // event_time is the span start time, named so to be compatible with + // the standard ClickHouse system log column names. + {std::make_shared(), "event_time"}, {std::make_shared(), "trace_id"}, {std::make_shared(), "span_id"}, {std::make_shared(), "parent_span_id"}, {std::make_shared(), "operation_name"}, - {std::make_shared(), "start_date"}, - {std::make_shared(), "start_time"}, {std::make_shared(), "finish_time"}, {std::make_shared(std::make_shared()), "attribute.names"}, @@ -31,12 +34,12 @@ void OpenTelemetrySpanLogElement::appendToBlock(MutableColumns & columns) const { size_t i = 0; - columns[i++]->insert(trace_id); + columns[i++]->insert(DateLUT::instance().toDayNum(start_time)); + columns[i++]->insert(start_time); + columns[i++]->insert(UInt128(Int128(trace_id))); columns[i++]->insert(span_id); columns[i++]->insert(parent_span_id); columns[i++]->insert(operation_name); - columns[i++]->insert(DateLUT::instance().toDayNum(start_time)); - columns[i++]->insert(start_time); columns[i++]->insert(finish_time); columns[i++]->insert(attribute_names); columns[i++]->insert(attribute_values); diff --git a/src/Interpreters/OpenTelemetryLog.h b/src/Interpreters/OpenTelemetryLog.h index 723a4201783..73ad5382c95 100644 --- a/src/Interpreters/OpenTelemetryLog.h +++ b/src/Interpreters/OpenTelemetryLog.h @@ -19,7 +19,7 @@ struct OpenTelemetrySpanContext // must log. struct OpenTelemetrySpan { - UInt128 trace_id; + __uint128_t trace_id; UInt64 span_id; UInt64 parent_span_id; std::string operation_name; diff --git a/src/Interpreters/executeQuery.cpp b/src/Interpreters/executeQuery.cpp index 27ac5fda56f..e74b24c4aa7 100644 --- a/src/Interpreters/executeQuery.cpp +++ b/src/Interpreters/executeQuery.cpp @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -146,6 +147,11 @@ static void logQuery(const String & query, const Context & context, bool interna (current_user != "default" ? ", user: " + context.getClientInfo().current_user : ""), (!initial_query_id.empty() && current_query_id != initial_query_id ? ", initial_query_id: " + initial_query_id : std::string()), joinLines(query)); + + LOG_TRACE(&Poco::Logger::get("executeQuery"), + "OpenTelemetry trace id {:x}, span id {:x}, parent span id {:x}", + context.getClientInfo().trace_id, context.getClientInfo().span_id, + context.getClientInfo().parent_span_id); } } @@ -216,6 +222,29 @@ static void onExceptionBeforeStart(const String & query_for_logging, Context & c if (auto query_log = context.getQueryLog()) query_log->add(elem); + if (auto opentelemetry_log = context.getOpenTelemetryLog()) + { + OpenTelemetrySpanLogElement span; + span.trace_id = context.getClientInfo().trace_id; + span.span_id = context.getClientInfo().span_id; + span.parent_span_id = context.getClientInfo().parent_span_id; + span.operation_name = "query"; + span.start_time = current_time; + span.finish_time = current_time; + + // keep values synchonized to type enum in QueryLogElement::createBlock + span.attribute_names.push_back("status"); + span.attribute_values.push_back("ExceptionBeforeStart"); + + span.attribute_names.push_back("query"); + span.attribute_values.push_back(elem.query); + + span.attribute_names.push_back("query_id"); + span.attribute_values.push_back(elem.client_info.current_query_id); + + opentelemetry_log->add(span); + } + ProfileEvents::increment(ProfileEvents::FailedQuery); if (ast) @@ -587,6 +616,25 @@ static std::tuple executeQueryImpl( if (auto opentelemetry_log = context.getOpenTelemetryLog()) { + OpenTelemetrySpanLogElement span; + span.trace_id = context.getClientInfo().trace_id; + span.span_id = context.getClientInfo().span_id; + span.parent_span_id = context.getClientInfo().parent_span_id; + span.operation_name = "query"; + span.start_time = elem.query_start_time; + span.finish_time = time(nullptr); // current time + + // keep values synchonized to type enum in QueryLogElement::createBlock + span.attribute_names.push_back("status"); + span.attribute_values.push_back("QueryFinish"); + + span.attribute_names.push_back("query"); + span.attribute_values.push_back(elem.query); + + span.attribute_names.push_back("query_id"); + span.attribute_values.push_back(elem.client_info.current_query_id); + + opentelemetry_log->add(span); } }; From 2bef406200c9e097cfc42910b9b055e1f350b840 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Thu, 27 Aug 2020 21:49:06 +0300 Subject: [PATCH 003/145] remove accidental changes --- base/daemon/BaseDaemon.cpp | 2 +- src/Interpreters/ClientInfo.h | 1 - src/Storages/StorageFile.cpp | 59 +---------------------------------- 3 files changed, 2 insertions(+), 60 deletions(-) diff --git a/base/daemon/BaseDaemon.cpp b/base/daemon/BaseDaemon.cpp index cc5a765a52d..78801e71a6f 100644 --- a/base/daemon/BaseDaemon.cpp +++ b/base/daemon/BaseDaemon.cpp @@ -153,7 +153,7 @@ static void signalHandler(int sig, siginfo_t * info, void * context) if (sig != SIGTSTP) /// This signal is used for debugging. { /// The time that is usually enough for separate thread to print info into log. - sleepForSeconds(1); + sleepForSeconds(10); call_default_signal_handler(sig); } diff --git a/src/Interpreters/ClientInfo.h b/src/Interpreters/ClientInfo.h index bc6afec36f4..52391d6cf73 100644 --- a/src/Interpreters/ClientInfo.h +++ b/src/Interpreters/ClientInfo.h @@ -59,7 +59,6 @@ public: String initial_query_id; Poco::Net::SocketAddress initial_address; - __uint128_t trace_id; UInt64 span_id; UInt64 parent_span_id; diff --git a/src/Storages/StorageFile.cpp b/src/Storages/StorageFile.cpp index d22a0625a67..558216a6216 100644 --- a/src/Storages/StorageFile.cpp +++ b/src/Storages/StorageFile.cpp @@ -199,45 +199,6 @@ StorageFile::StorageFile(CommonArguments args) setInMemoryMetadata(storage_metadata); } -struct ScopedExceptionHint -{ - std::string hint; - - // Format message with fmt::format - template - ScopedExceptionHint(Fmt&&... fmt) - : hint(fmt::format(std::forward(fmt)...)) - { - fprintf(stderr, "hint %s created\n", hint.c_str()); - } - - ~ScopedExceptionHint() - { - fprintf(stderr, "hint %s is being destroyed\n", hint.c_str()); - std::exception_ptr exception = std::current_exception(); - if (exception) - { - try { - std::rethrow_exception(exception); - } - catch (Exception & e) - { - e.addMessage(hint); - fprintf(stderr, "added hint %s\n", hint.c_str()); - } - catch (...) - { - fprintf(stderr, "unknown exception\n"); - // pass? - } - } - else - { - fprintf(stderr, "no exception\n"); - } - } -}; - class StorageFileSource : public SourceWithProgress { public: @@ -356,28 +317,10 @@ public: if (!column_defaults.empty()) reader = std::make_shared(reader, column_defaults, context); - ScopedExceptionHint("Reading prefix of file '{}'", current_path); reader->readPrefix(); } - ScopedExceptionHint("Reader->read() {}", reader->getName()); - - Block res; - try - { - res = reader->read(); - } - catch (Exception & e) - { - e.addMessage("Error while reading from '{}'", current_path); - throw; - } - catch (...) - { - throw; - } - - if (res) + if (auto res = reader->read()) { Columns columns = res.getColumns(); UInt64 num_rows = res.rows(); From 46127946788d0ae89aa8c895b8d31fbb453e04c1 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Fri, 28 Aug 2020 04:21:08 +0300 Subject: [PATCH 004/145] opentelemetry context propagation --- src/Core/Defines.h | 5 +- src/Interpreters/ClientInfo.cpp | 140 ++++++++++++++++++++++++++++++ src/Interpreters/ClientInfo.h | 17 +++- src/Interpreters/Context.cpp | 20 ++++- src/Interpreters/executeQuery.cpp | 33 +++++-- src/Interpreters/ya.make | 1 + src/Server/HTTPHandler.cpp | 23 ++++- src/Server/TCPHandler.cpp | 8 +- src/Storages/StorageURL.cpp | 18 +++- 9 files changed, 244 insertions(+), 21 deletions(-) diff --git a/src/Core/Defines.h b/src/Core/Defines.h index e244581c339..d19513d1434 100644 --- a/src/Core/Defines.h +++ b/src/Core/Defines.h @@ -67,8 +67,11 @@ /// Minimum revision supporting SettingsBinaryFormat::STRINGS. #define DBMS_MIN_REVISION_WITH_SETTINGS_SERIALIZED_AS_STRINGS 54429 +/// Minimum revision supporting OpenTelemetry +#define DBMS_MIN_REVISION_WITH_OPENTELEMETRY 54227 + /// Version of ClickHouse TCP protocol. Set to git tag with latest protocol change. -#define DBMS_TCP_PROTOCOL_VERSION 54226 +#define DBMS_TCP_PROTOCOL_VERSION 54227 /// The boundary on which the blocks for asynchronous file operations should be aligned. #define DEFAULT_AIO_FILE_BLOCK_SIZE 4096 diff --git a/src/Interpreters/ClientInfo.cpp b/src/Interpreters/ClientInfo.cpp index 378375dcc18..9e501ca5c11 100644 --- a/src/Interpreters/ClientInfo.cpp +++ b/src/Interpreters/ClientInfo.cpp @@ -60,6 +60,18 @@ void ClientInfo::write(WriteBuffer & out, const UInt64 server_protocol_revision) if (server_protocol_revision >= DBMS_MIN_REVISION_WITH_VERSION_PATCH) writeVarUInt(client_version_patch, out); } + + if (server_protocol_revision >= DBMS_MIN_REVISION_WITH_OPENTELEMETRY) + { + // No point writing these numbers with variable length, because they + // are random and will probably require the full length anyway. + writeBinary(opentelemetry_trace_id, out); + writeBinary(opentelemetry_span_id, out); + writeBinary(opentelemetry_parent_span_id, out); + writeBinary(opentelemetry_tracestate, out); + writeBinary(opentelemetry_trace_flags, out); + std::cerr << fmt::format("wrote {:x}, {}, {}\n", opentelemetry_trace_id, opentelemetry_span_id, opentelemetry_parent_span_id) << StackTrace().toString() << std::endl; + } } @@ -113,6 +125,17 @@ void ClientInfo::read(ReadBuffer & in, const UInt64 client_protocol_revision) else client_version_patch = client_revision; } + + if (client_protocol_revision >= DBMS_MIN_REVISION_WITH_OPENTELEMETRY) + { + readBinary(opentelemetry_trace_id, in); + readBinary(opentelemetry_span_id, in); + readBinary(opentelemetry_parent_span_id, in); + readBinary(opentelemetry_tracestate, in); + readBinary(opentelemetry_trace_flags, in); + + std::cerr << fmt::format("read {:x}, {}, {}\n", opentelemetry_trace_id, opentelemetry_span_id, opentelemetry_parent_span_id) << StackTrace().toString() << std::endl; + } } @@ -123,6 +146,123 @@ void ClientInfo::setInitialQuery() client_name = (DBMS_NAME " ") + client_name; } +template +bool readLowercaseHexDigits(const char *& begin, const char * end, T & dest_value, std::string & error) +{ + char * dest_begin = reinterpret_cast(&dest_value); + char * dest_end = dest_begin + sizeof(dest_value); + bool odd_character = true; + for (;;) + { + if (begin == end) + { + if (dest_begin == dest_end) + { + return true; + } + error = fmt::format("Not enough charaters in the input, got {}, need {} more", end - begin, dest_end - dest_begin); + return false; + } + + if (dest_begin == dest_end) + { + return true; + } + + int cur = 0; + if (*begin >= '0' && *begin <= '9') + { + cur = *begin - '0'; + } + else if (*begin >= 'a' && *begin <= 'f') + { + cur = 10 + *begin - 'a'; + } + else + { + error = fmt::format("Encountered '{}' which is not a lowercase hexadecimal digit", *begin); + return false; + } + + // Two characters per byte, little-endian. + if (odd_character) + { + *(dest_end - 1) = cur; + } + else + { + *(dest_end - 1) = *(dest_end - 1) << 8 | cur; + --dest_end; + } + + begin++; + odd_character = !odd_character; + } +} + +bool ClientInfo::setOpenTelemetryTraceparent(const std::string & traceparent, + std::string & error) +{ + uint8_t version = -1; + __uint128_t trace_id = 0; + uint64_t trace_parent = 0; + uint8_t trace_flags = 0; + + const char * begin = &traceparent[0]; + const char * end = begin + traceparent.length(); + +#define CHECK_CONDITION(condition, ...) \ + ((condition) || (error = fmt::format(__VA_ARGS__), false)) + +#define CHECK_DELIMITER \ + (begin >= end \ + ? (error = fmt::format( \ + "Expected '-' delimiter, got EOL at position {}", \ + begin - &traceparent[0]), \ + false) \ + : *begin != '-' \ + ? (error = fmt::format( \ + "Expected '-' delimiter, got '{}' at position {}", \ + *begin, begin - &traceparent[0]), \ + false) \ + : (++begin, true)) + + bool result = readLowercaseHexDigits(begin, end, version, error) + && CHECK_CONDITION(version == 0, "Expected version 00, got {}", version) + && CHECK_DELIMITER + && readLowercaseHexDigits(begin, end, trace_id, error) + && CHECK_DELIMITER + && readLowercaseHexDigits(begin, end, trace_parent, error) + && CHECK_DELIMITER + && readLowercaseHexDigits(begin, end, trace_flags, error) + && CHECK_CONDITION(begin == end, + "Expected end of string, got {} at position {}", *begin, end - begin); + +#undef CHECK +#undef CHECK_DELIMITER + + if (!result) + { + return false; + } + + opentelemetry_trace_id = trace_id; + opentelemetry_parent_span_id = trace_parent; + opentelemetry_trace_flags = trace_flags; + return true; +} + + +std::string ClientInfo::getOpenTelemetryTraceparentForChild() const +{ + // This span is a parent for its children (so deep...), so we specify + // this span_id as a parent id. + return fmt::format("00-{:032x}-{:016x}-{:02x}", opentelemetry_trace_id, + opentelemetry_span_id, + // This cast is because fmt is being weird and complaining that + // "mixing character types is not allowed". + static_cast(opentelemetry_trace_flags)); +} void ClientInfo::fillOSUserHostNameAndVersionInfo() { diff --git a/src/Interpreters/ClientInfo.h b/src/Interpreters/ClientInfo.h index 52391d6cf73..413e1c42bf7 100644 --- a/src/Interpreters/ClientInfo.h +++ b/src/Interpreters/ClientInfo.h @@ -59,9 +59,17 @@ public: String initial_query_id; Poco::Net::SocketAddress initial_address; - __uint128_t trace_id; - UInt64 span_id; - UInt64 parent_span_id; + // OpenTelemetry things + __uint128_t opentelemetry_trace_id = 0; + // Span ID is not strictly the client info, but convenient to keep here. + // The span id we get the in the incoming client info becomes our parent span + // id, and the span id we send becomes downstream parent span id. + UInt64 opentelemetry_span_id = 0; + UInt64 opentelemetry_parent_span_id = 0; + // the incoming tracestate header, we just pass it downstream. + // https://www.w3.org/TR/trace-context/ + String opentelemetry_tracestate; + UInt8 opentelemetry_trace_flags; /// All below are parameters related to initial query. @@ -95,6 +103,9 @@ public: /// Initialize parameters on client initiating query. void setInitialQuery(); + bool setOpenTelemetryTraceparent(const std::string & traceparent, std::string & error); + std::string getOpenTelemetryTraceparentForChild() const; + private: void fillOSUserHostNameAndVersionInfo(); }; diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index c225c332248..c5fd2d585e1 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -1093,9 +1093,23 @@ void Context::setCurrentQueryId(const String & query_id) random.words.a = thread_local_rng(); //-V656 random.words.b = thread_local_rng(); //-V656 - client_info.trace_id = random.uuid; - client_info.span_id = 1; - client_info.parent_span_id = 0; + fmt::print(stderr, "traceid {}, ==0 {}\n", client_info.opentelemetry_trace_id, client_info.opentelemetry_trace_id == 0); + if (client_info.opentelemetry_trace_id == 0) + { + // If trace_id is not initialized, it means that this is an initial query + // without any parent OpenTelemetry trace. Use the randomly generated + // default query id as the new trace id. + client_info.opentelemetry_trace_id = random.uuid; + client_info.opentelemetry_parent_span_id = 0; + client_info.opentelemetry_span_id = thread_local_rng(); + } + else + { + // The incoming span id becomes our parent span id. + client_info.opentelemetry_parent_span_id = client_info.opentelemetry_span_id; + client_info.opentelemetry_span_id = thread_local_rng(); + } + fmt::print(stderr, "traceid {}, ==0 {}\n{}\n", client_info.opentelemetry_trace_id, client_info.opentelemetry_trace_id == 0, StackTrace().toString()); String query_id_to_set = query_id; if (query_id_to_set.empty()) /// If the user did not submit his query_id, then we generate it ourselves. diff --git a/src/Interpreters/executeQuery.cpp b/src/Interpreters/executeQuery.cpp index e74b24c4aa7..2a35bc205fa 100644 --- a/src/Interpreters/executeQuery.cpp +++ b/src/Interpreters/executeQuery.cpp @@ -149,9 +149,11 @@ static void logQuery(const String & query, const Context & context, bool interna joinLines(query)); LOG_TRACE(&Poco::Logger::get("executeQuery"), - "OpenTelemetry trace id {:x}, span id {:x}, parent span id {:x}", - context.getClientInfo().trace_id, context.getClientInfo().span_id, - context.getClientInfo().parent_span_id); + "OpenTelemetry trace id {:x}, span id {}, parent span id {}", + context.getClientInfo().opentelemetry_trace_id, context.getClientInfo().opentelemetry_span_id, + context.getClientInfo().opentelemetry_parent_span_id); + + std::cerr << StackTrace().toString() << std::endl; } } @@ -225,9 +227,9 @@ static void onExceptionBeforeStart(const String & query_for_logging, Context & c if (auto opentelemetry_log = context.getOpenTelemetryLog()) { OpenTelemetrySpanLogElement span; - span.trace_id = context.getClientInfo().trace_id; - span.span_id = context.getClientInfo().span_id; - span.parent_span_id = context.getClientInfo().parent_span_id; + span.trace_id = context.getClientInfo().opentelemetry_trace_id; + span.span_id = context.getClientInfo().opentelemetry_span_id; + span.parent_span_id = context.getClientInfo().opentelemetry_parent_span_id; span.operation_name = "query"; span.start_time = current_time; span.finish_time = current_time; @@ -242,6 +244,13 @@ static void onExceptionBeforeStart(const String & query_for_logging, Context & c span.attribute_names.push_back("query_id"); span.attribute_values.push_back(elem.client_info.current_query_id); + if (!context.getClientInfo().opentelemetry_tracestate.empty()) + { + span.attribute_names.push_back("tracestate"); + span.attribute_values.push_back( + context.getClientInfo().opentelemetry_tracestate); + } + opentelemetry_log->add(span); } @@ -617,9 +626,9 @@ static std::tuple executeQueryImpl( if (auto opentelemetry_log = context.getOpenTelemetryLog()) { OpenTelemetrySpanLogElement span; - span.trace_id = context.getClientInfo().trace_id; - span.span_id = context.getClientInfo().span_id; - span.parent_span_id = context.getClientInfo().parent_span_id; + span.trace_id = context.getClientInfo().opentelemetry_trace_id; + span.span_id = context.getClientInfo().opentelemetry_span_id; + span.parent_span_id = context.getClientInfo().opentelemetry_parent_span_id; span.operation_name = "query"; span.start_time = elem.query_start_time; span.finish_time = time(nullptr); // current time @@ -633,6 +642,12 @@ static std::tuple executeQueryImpl( span.attribute_names.push_back("query_id"); span.attribute_values.push_back(elem.client_info.current_query_id); + if (!context.getClientInfo().opentelemetry_tracestate.empty()) + { + span.attribute_names.push_back("tracestate"); + span.attribute_values.push_back( + context.getClientInfo().opentelemetry_tracestate); + } opentelemetry_log->add(span); } diff --git a/src/Interpreters/ya.make b/src/Interpreters/ya.make index 23cde61a744..8c4086722c8 100644 --- a/src/Interpreters/ya.make +++ b/src/Interpreters/ya.make @@ -113,6 +113,7 @@ SRCS( MutationsInterpreter.cpp MySQL/InterpretersMySQLDDLQuery.cpp NullableUtils.cpp + OpenTelemetryLog.cpp OptimizeIfChains.cpp OptimizeIfWithConstantConditionVisitor.cpp PartLog.cpp diff --git a/src/Server/HTTPHandler.cpp b/src/Server/HTTPHandler.cpp index 95f56b715b8..fb630010198 100644 --- a/src/Server/HTTPHandler.cpp +++ b/src/Server/HTTPHandler.cpp @@ -96,6 +96,7 @@ namespace ErrorCodes extern const int WRONG_PASSWORD; extern const int REQUIRED_PASSWORD; + extern const int BAD_REQUEST_PARAMETER; extern const int INVALID_SESSION_TIMEOUT; extern const int HTTP_LENGTH_REQUIRED; } @@ -279,9 +280,7 @@ void HTTPHandler::processQuery( } } - std::string query_id = params.get("query_id", ""); context.setUser(user, password, request.clientAddress()); - context.setCurrentQueryId(query_id); if (!quota_key.empty()) context.setQuotaKey(quota_key); @@ -311,6 +310,26 @@ void HTTPHandler::processQuery( session->release(); }); + std::string query_id = params.get("query_id", ""); + context.setCurrentQueryId(query_id); + + if (request.has("traceparent")) + { + std::string opentelemetry_traceparent = request.get("traceparent"); + std::string error; + if (!context.getClientInfo().setOpenTelemetryTraceparent( + opentelemetry_traceparent, error)) + { + throw Exception(ErrorCodes::BAD_REQUEST_PARAMETER, + "Failed to parse OpenTelemetry traceparent header '{}': {}", + opentelemetry_traceparent, error); + } + + context.getClientInfo().opentelemetry_tracestate = request.get("tracestate", ""); + + + } + /// The client can pass a HTTP header indicating supported compression method (gzip or deflate). String http_response_compression_methods = request.get("Accept-Encoding", ""); CompressionMethod http_response_compression_method = CompressionMethod::None; diff --git a/src/Server/TCPHandler.cpp b/src/Server/TCPHandler.cpp index ab4ce820666..e83bbb02cad 100644 --- a/src/Server/TCPHandler.cpp +++ b/src/Server/TCPHandler.cpp @@ -845,13 +845,17 @@ void TCPHandler::receiveQuery() state.is_empty = false; readStringBinary(state.query_id, *in); - query_context->setCurrentQueryId(state.query_id); - /// Client info ClientInfo & client_info = query_context->getClientInfo(); if (client_revision >= DBMS_MIN_REVISION_WITH_CLIENT_INFO) client_info.read(*in, client_revision); + // It is convenient to generate default OpenTelemetry trace id and default + // query id together. ClientInfo might contain upstream trace id, so we + // decide whether to use the default ids after we have received the ClientInfo. + // We also set up the parent span id while we're at it. + query_context->setCurrentQueryId(state.query_id); + /// For better support of old clients, that does not send ClientInfo. if (client_info.query_kind == ClientInfo::QueryKind::NO_QUERY) { diff --git a/src/Storages/StorageURL.cpp b/src/Storages/StorageURL.cpp index c2f7bfd18d2..d5e86c08a8b 100644 --- a/src/Storages/StorageURL.cpp +++ b/src/Storages/StorageURL.cpp @@ -67,6 +67,22 @@ namespace const CompressionMethod compression_method) : SourceWithProgress(sample_block), name(std::move(name_)) { + ReadWriteBufferFromHTTP::HTTPHeaderEntries header; + + // Propagate OpenTelemetry trace context, if any, downstream. + auto & client_info = context.getClientInfo(); + if (client_info.opentelemetry_trace_id) + { + header.emplace_back("traceparent", + client_info.getOpenTelemetryTraceparentForChild()); + + if (!client_info.opentelemetry_tracestate.empty()) + { + header.emplace_back("tracestate", + client_info.opentelemetry_tracestate); + } + } + read_buf = wrapReadBufferWithCompressionMethod( std::make_unique( uri, @@ -76,7 +92,7 @@ namespace context.getSettingsRef().max_http_get_redirects, Poco::Net::HTTPBasicCredentials{}, DBMS_DEFAULT_BUFFER_SIZE, - ReadWriteBufferFromHTTP::HTTPHeaderEntries{}, + header, context.getRemoteHostFilter()), compression_method); From f87db0fbe3a9a32fdb2041ecb822cf304edbcf5e Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Fri, 28 Aug 2020 04:34:06 +0300 Subject: [PATCH 005/145] less crazy parsing --- src/Interpreters/ClientInfo.cpp | 113 +++++++------------------------- 1 file changed, 24 insertions(+), 89 deletions(-) diff --git a/src/Interpreters/ClientInfo.cpp b/src/Interpreters/ClientInfo.cpp index 9e501ca5c11..0e8da221ca9 100644 --- a/src/Interpreters/ClientInfo.cpp +++ b/src/Interpreters/ClientInfo.cpp @@ -146,107 +146,42 @@ void ClientInfo::setInitialQuery() client_name = (DBMS_NAME " ") + client_name; } -template -bool readLowercaseHexDigits(const char *& begin, const char * end, T & dest_value, std::string & error) -{ - char * dest_begin = reinterpret_cast(&dest_value); - char * dest_end = dest_begin + sizeof(dest_value); - bool odd_character = true; - for (;;) - { - if (begin == end) - { - if (dest_begin == dest_end) - { - return true; - } - error = fmt::format("Not enough charaters in the input, got {}, need {} more", end - begin, dest_end - dest_begin); - return false; - } - - if (dest_begin == dest_end) - { - return true; - } - - int cur = 0; - if (*begin >= '0' && *begin <= '9') - { - cur = *begin - '0'; - } - else if (*begin >= 'a' && *begin <= 'f') - { - cur = 10 + *begin - 'a'; - } - else - { - error = fmt::format("Encountered '{}' which is not a lowercase hexadecimal digit", *begin); - return false; - } - - // Two characters per byte, little-endian. - if (odd_character) - { - *(dest_end - 1) = cur; - } - else - { - *(dest_end - 1) = *(dest_end - 1) << 8 | cur; - --dest_end; - } - - begin++; - odd_character = !odd_character; - } -} - bool ClientInfo::setOpenTelemetryTraceparent(const std::string & traceparent, std::string & error) { uint8_t version = -1; - __uint128_t trace_id = 0; + __uint64_t trace_id_high = 0; + __uint64_t trace_id_low = 0; uint64_t trace_parent = 0; uint8_t trace_flags = 0; - const char * begin = &traceparent[0]; - const char * end = begin + traceparent.length(); + int result = sscanf(&traceparent[0], + "%2" SCNx8 "-%16" SCNx64 "%16" SCNx64 "-%16" SCNx64 "-%2" SCNx8, + &version, &trace_id_high, &trace_id_low, &trace_parent, &trace_flags); -#define CHECK_CONDITION(condition, ...) \ - ((condition) || (error = fmt::format(__VA_ARGS__), false)) - -#define CHECK_DELIMITER \ - (begin >= end \ - ? (error = fmt::format( \ - "Expected '-' delimiter, got EOL at position {}", \ - begin - &traceparent[0]), \ - false) \ - : *begin != '-' \ - ? (error = fmt::format( \ - "Expected '-' delimiter, got '{}' at position {}", \ - *begin, begin - &traceparent[0]), \ - false) \ - : (++begin, true)) - - bool result = readLowercaseHexDigits(begin, end, version, error) - && CHECK_CONDITION(version == 0, "Expected version 00, got {}", version) - && CHECK_DELIMITER - && readLowercaseHexDigits(begin, end, trace_id, error) - && CHECK_DELIMITER - && readLowercaseHexDigits(begin, end, trace_parent, error) - && CHECK_DELIMITER - && readLowercaseHexDigits(begin, end, trace_flags, error) - && CHECK_CONDITION(begin == end, - "Expected end of string, got {} at position {}", *begin, end - begin); - -#undef CHECK -#undef CHECK_DELIMITER - - if (!result) + if (result == EOF) { + error = "Failed to parse traceparent header (EOF)"; return false; } - opentelemetry_trace_id = trace_id; + if (result != 5) + { + error = fmt::format("Failed to parse traceparent header" + "(could only read {} parts instead of the expected 5)", + result); + return false; + } + + if (version != 0) + { + error = fmt::format("Unexpected version {} of traceparent header:" + "expected 00", version); + return false; + } + + opentelemetry_trace_id = static_cast<__uint128_t>(trace_id_high) << 64 + | trace_id_low; opentelemetry_parent_span_id = trace_parent; opentelemetry_trace_flags = trace_flags; return true; From 063cc386a3508cb8ec857373bfe10ca83f2a4c58 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Fri, 28 Aug 2020 22:02:50 +0300 Subject: [PATCH 006/145] cosmetic fixes --- src/Interpreters/ClientInfo.cpp | 27 +++++++++++++++++++++------ src/Interpreters/ClientInfo.h | 2 +- src/Interpreters/Context.cpp | 4 ++-- src/Interpreters/executeQuery.cpp | 2 +- 4 files changed, 25 insertions(+), 10 deletions(-) diff --git a/src/Interpreters/ClientInfo.cpp b/src/Interpreters/ClientInfo.cpp index 0e8da221ca9..f7ed8eafa46 100644 --- a/src/Interpreters/ClientInfo.cpp +++ b/src/Interpreters/ClientInfo.cpp @@ -70,7 +70,6 @@ void ClientInfo::write(WriteBuffer & out, const UInt64 server_protocol_revision) writeBinary(opentelemetry_parent_span_id, out); writeBinary(opentelemetry_tracestate, out); writeBinary(opentelemetry_trace_flags, out); - std::cerr << fmt::format("wrote {:x}, {}, {}\n", opentelemetry_trace_id, opentelemetry_span_id, opentelemetry_parent_span_id) << StackTrace().toString() << std::endl; } } @@ -155,28 +154,44 @@ bool ClientInfo::setOpenTelemetryTraceparent(const std::string & traceparent, uint64_t trace_parent = 0; uint8_t trace_flags = 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, &trace_parent, &trace_flags); if (result == EOF) { - error = "Failed to parse traceparent header (EOF)"; + error = "EOF"; return false; } + // We read uint128 as two uint64, so 5 parts and not 4. if (result != 5) { - error = fmt::format("Failed to parse traceparent header" - "(could only read {} parts instead of the expected 5)", + error = fmt::format("could only read {} parts instead of the expected 5", result); return false; } if (version != 0) { - error = fmt::format("Unexpected version {} of traceparent header:" - "expected 00", version); + error = fmt::format("unexpected version {}, expected 00", version); return false; } diff --git a/src/Interpreters/ClientInfo.h b/src/Interpreters/ClientInfo.h index 413e1c42bf7..604a9771a52 100644 --- a/src/Interpreters/ClientInfo.h +++ b/src/Interpreters/ClientInfo.h @@ -58,7 +58,7 @@ public: String initial_user; String initial_query_id; Poco::Net::SocketAddress initial_address; - + // OpenTelemetry things __uint128_t opentelemetry_trace_id = 0; // Span ID is not strictly the client info, but convenient to keep here. diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index c5fd2d585e1..362e762df6d 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -1092,7 +1092,7 @@ void Context::setCurrentQueryId(const String & query_id) random.words.a = thread_local_rng(); //-V656 random.words.b = thread_local_rng(); //-V656 - + fmt::print(stderr, "traceid {}, ==0 {}\n", client_info.opentelemetry_trace_id, client_info.opentelemetry_trace_id == 0); if (client_info.opentelemetry_trace_id == 0) { @@ -1120,7 +1120,7 @@ void Context::setCurrentQueryId(const String & query_id) QueryUUID(const char * bytes, Poco::UUID::Version version) : Poco::UUID(bytes, version) {} }; - + query_id_to_set = QueryUUID(random.bytes, Poco::UUID::UUID_RANDOM).toString(); } diff --git a/src/Interpreters/executeQuery.cpp b/src/Interpreters/executeQuery.cpp index 2a35bc205fa..05a0211adce 100644 --- a/src/Interpreters/executeQuery.cpp +++ b/src/Interpreters/executeQuery.cpp @@ -147,7 +147,7 @@ static void logQuery(const String & query, const Context & context, bool interna (current_user != "default" ? ", user: " + context.getClientInfo().current_user : ""), (!initial_query_id.empty() && current_query_id != initial_query_id ? ", initial_query_id: " + initial_query_id : std::string()), joinLines(query)); - + LOG_TRACE(&Poco::Logger::get("executeQuery"), "OpenTelemetry trace id {:x}, span id {}, parent span id {}", context.getClientInfo().opentelemetry_trace_id, context.getClientInfo().opentelemetry_span_id, From 79e0b184ac16675322f25d1d840c50201a2ab822 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Fri, 28 Aug 2020 23:40:23 +0300 Subject: [PATCH 007/145] A test + reinterpretAsUUID --- src/Functions/reinterpretStringAs.cpp | 8 +++-- src/Storages/StorageURL.cpp | 2 +- .../0_stateless/01455_opentelemetry.reference | 3 ++ .../0_stateless/01455_opentelemetry.sh | 31 +++++++++++++++++++ tests/queries/shell_config.sh | 20 ++++++------ 5 files changed, 52 insertions(+), 12 deletions(-) create mode 100644 tests/queries/0_stateless/01455_opentelemetry.reference create mode 100755 tests/queries/0_stateless/01455_opentelemetry.sh diff --git a/src/Functions/reinterpretStringAs.cpp b/src/Functions/reinterpretStringAs.cpp index bb290b33b6d..34e53f32dd2 100644 --- a/src/Functions/reinterpretStringAs.cpp +++ b/src/Functions/reinterpretStringAs.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -66,7 +67,7 @@ public: size_t offset = 0; for (size_t i = 0; i < size; ++i) { - ToFieldType value = 0; + ToFieldType value{}; memcpy(&value, &data_from[offset], std::min(static_cast(sizeof(ToFieldType)), offsets_from[i] - offset - 1)); vec_res[i] = value; offset = offsets_from[i]; @@ -88,7 +89,7 @@ public: size_t copy_size = std::min(step, sizeof(ToFieldType)); for (size_t i = 0; i < size; ++i) { - ToFieldType value = 0; + ToFieldType value{}; memcpy(&value, &data_from[offset], copy_size); vec_res[i] = value; offset += step; @@ -118,6 +119,7 @@ struct NameReinterpretAsFloat32 { static constexpr auto name = "reinterpretA struct NameReinterpretAsFloat64 { static constexpr auto name = "reinterpretAsFloat64"; }; struct NameReinterpretAsDate { static constexpr auto name = "reinterpretAsDate"; }; struct NameReinterpretAsDateTime { static constexpr auto name = "reinterpretAsDateTime"; }; +struct NameReinterpretAsUUID { static constexpr auto name = "reinterpretAsUUID"; }; using FunctionReinterpretAsUInt8 = FunctionReinterpretStringAs; using FunctionReinterpretAsUInt16 = FunctionReinterpretStringAs; @@ -131,6 +133,7 @@ using FunctionReinterpretAsFloat32 = FunctionReinterpretStringAs; using FunctionReinterpretAsDate = FunctionReinterpretStringAs; using FunctionReinterpretAsDateTime = FunctionReinterpretStringAs; +using FunctionReinterpretAsUUID = FunctionReinterpretStringAs; void registerFunctionsReinterpretStringAs(FunctionFactory & factory) @@ -147,6 +150,7 @@ void registerFunctionsReinterpretStringAs(FunctionFactory & factory) factory.registerFunction(); factory.registerFunction(); factory.registerFunction(); + factory.registerFunction(); } } diff --git a/src/Storages/StorageURL.cpp b/src/Storages/StorageURL.cpp index d5e86c08a8b..cd1b5dd882a 100644 --- a/src/Storages/StorageURL.cpp +++ b/src/Storages/StorageURL.cpp @@ -70,7 +70,7 @@ namespace ReadWriteBufferFromHTTP::HTTPHeaderEntries header; // Propagate OpenTelemetry trace context, if any, downstream. - auto & client_info = context.getClientInfo(); + const auto & client_info = context.getClientInfo(); if (client_info.opentelemetry_trace_id) { header.emplace_back("traceparent", diff --git a/tests/queries/0_stateless/01455_opentelemetry.reference b/tests/queries/0_stateless/01455_opentelemetry.reference new file mode 100644 index 00000000000..b0484d7df0b --- /dev/null +++ b/tests/queries/0_stateless/01455_opentelemetry.reference @@ -0,0 +1,3 @@ +1 +4 +1 diff --git a/tests/queries/0_stateless/01455_opentelemetry.sh b/tests/queries/0_stateless/01455_opentelemetry.sh new file mode 100755 index 00000000000..f537377dfe6 --- /dev/null +++ b/tests/queries/0_stateless/01455_opentelemetry.sh @@ -0,0 +1,31 @@ +#!/usr/bin/env bash +set -ue + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +. "$CURDIR"/../shell_config.sh + +# Generate some random trace id so that the prevous runs of the test do not interfere. +trace_id=$(${CLICKHOUSE_CLIENT} -q "select lower(hex(reverse(reinterpretAsString(generateUUIDv4()))))") + +${CLICKHOUSE_CURL} --header "traceparent: 00-$trace_id-0000000000000010-01" --header "tracestate: some custom state" "http://localhost:8123/" --get --data-urlencode "query=select 1 from remote('127.0.0.2', system, one)" + +${CLICKHOUSE_CLIENT} -q "system flush logs" + +# Check that the HTTP traceparent was read, and then passed to `remote` instance. +# We expect 4 queries, because there are two DESC TABLE queries for the shard. +# This is bug-ish, see https://github.com/ClickHouse/ClickHouse/issues/14228 +${CLICKHOUSE_CLIENT} -q "select count(*) from system.opentelemetry_log where trace_id = reinterpretAsUUID(reverse(unhex('$trace_id')))" + +# Check that the tracestate header was read and passed. Must have +# exactly the same value for all "query" spans in this trace. +${CLICKHOUSE_CLIENT} -q " + select count(distinct attribute.values) + from system.opentelemetry_log + array join attribute.names, attribute.values + where + trace_id = reinterpretAsUUID(reverse(unhex('$trace_id'))) + and operation_name = 'query' + and attribute.names = 'tracestate' +" + + diff --git a/tests/queries/shell_config.sh b/tests/queries/shell_config.sh index 8c66d79b5b1..cd4fd418c81 100644 --- a/tests/queries/shell_config.sh +++ b/tests/queries/shell_config.sh @@ -1,16 +1,18 @@ +#!/usr/bin/env bash + export CLICKHOUSE_DATABASE=${CLICKHOUSE_DATABASE:="test"} export CLICKHOUSE_CLIENT_SERVER_LOGS_LEVEL=${CLICKHOUSE_CLIENT_SERVER_LOGS_LEVEL:="warning"} -[ -n "$CLICKHOUSE_CONFIG_CLIENT" ] && CLICKHOUSE_CLIENT_OPT0+=" --config-file=${CLICKHOUSE_CONFIG_CLIENT} " -[ -n "${CLICKHOUSE_HOST}" ] && CLICKHOUSE_CLIENT_OPT0+=" --host=${CLICKHOUSE_HOST} " -[ -n "${CLICKHOUSE_PORT_TCP}" ] && CLICKHOUSE_CLIENT_OPT0+=" --port=${CLICKHOUSE_PORT_TCP} " -[ -n "${CLICKHOUSE_CLIENT_SERVER_LOGS_LEVEL}" ] && CLICKHOUSE_CLIENT_OPT0+=" --send_logs_level=${CLICKHOUSE_CLIENT_SERVER_LOGS_LEVEL} " -[ -n "${CLICKHOUSE_DATABASE}" ] && CLICKHOUSE_CLIENT_OPT0+=" --database=${CLICKHOUSE_DATABASE} " +[ -v CLICKHOUSE_CONFIG_CLIENT ] && CLICKHOUSE_CLIENT_OPT0+=" --config-file=${CLICKHOUSE_CONFIG_CLIENT} " +[ -v CLICKHOUSE_HOST ] && CLICKHOUSE_CLIENT_OPT0+=" --host=${CLICKHOUSE_HOST} " +[ -v CLICKHOUSE_PORT_TCP ] && CLICKHOUSE_CLIENT_OPT0+=" --port=${CLICKHOUSE_PORT_TCP} " +[ -v CLICKHOUSE_CLIENT_SERVER_LOGS_LEVEL ] && CLICKHOUSE_CLIENT_OPT0+=" --send_logs_level=${CLICKHOUSE_CLIENT_SERVER_LOGS_LEVEL} " +[ -v CLICKHOUSE_DATABASE ] && CLICKHOUSE_CLIENT_OPT0+=" --database=${CLICKHOUSE_DATABASE} " export CLICKHOUSE_BINARY=${CLICKHOUSE_BINARY:="clickhouse"} [ -x "$CLICKHOUSE_BINARY-client" ] && CLICKHOUSE_CLIENT_BINARY=${CLICKHOUSE_CLIENT_BINARY:=$CLICKHOUSE_BINARY-client} [ -x "$CLICKHOUSE_BINARY" ] && CLICKHOUSE_CLIENT_BINARY=${CLICKHOUSE_CLIENT_BINARY:=$CLICKHOUSE_BINARY client} export CLICKHOUSE_CLIENT_BINARY=${CLICKHOUSE_CLIENT_BINARY:=$CLICKHOUSE_BINARY-client} -export CLICKHOUSE_CLIENT=${CLICKHOUSE_CLIENT:="$CLICKHOUSE_CLIENT_BINARY ${CLICKHOUSE_CLIENT_OPT0} ${CLICKHOUSE_CLIENT_OPT}"} +export CLICKHOUSE_CLIENT=${CLICKHOUSE_CLIENT:="$CLICKHOUSE_CLIENT_BINARY ${CLICKHOUSE_CLIENT_OPT0:-} ${CLICKHOUSE_CLIENT_OPT:-}"} [ -x "${CLICKHOUSE_BINARY}-local" ] && CLICKHOUSE_LOCAL=${CLICKHOUSE_LOCAL:="${CLICKHOUSE_BINARY}-local"} [ -x "${CLICKHOUSE_BINARY}" ] && CLICKHOUSE_LOCAL=${CLICKHOUSE_LOCAL:="${CLICKHOUSE_BINARY} local"} export CLICKHOUSE_LOCAL=${CLICKHOUSE_LOCAL:="${CLICKHOUSE_BINARY}-local"} @@ -42,7 +44,7 @@ export CLICKHOUSE_PORT_HTTPS=${CLICKHOUSE_PORT_HTTPS:="8443"} export CLICKHOUSE_PORT_HTTP_PROTO=${CLICKHOUSE_PORT_HTTP_PROTO:="http"} # Add database to url params -if [ -n "${CLICKHOUSE_URL_PARAMS}" ] +if [ -v CLICKHOUSE_URL_PARAMS ] then export CLICKHOUSE_URL_PARAMS="${CLICKHOUSE_URL_PARAMS}&database=${CLICKHOUSE_DATABASE}" else @@ -53,7 +55,7 @@ export CLICKHOUSE_URL=${CLICKHOUSE_URL:="${CLICKHOUSE_PORT_HTTP_PROTO}://${CLICK export CLICKHOUSE_URL_HTTPS=${CLICKHOUSE_URL_HTTPS:="https://${CLICKHOUSE_HOST}:${CLICKHOUSE_PORT_HTTPS}/"} # Add url params to url -if [ -n "${CLICKHOUSE_URL_PARAMS}" ] +if [ -v CLICKHOUSE_URL_PARAMS ] then export CLICKHOUSE_URL="${CLICKHOUSE_URL}?${CLICKHOUSE_URL_PARAMS}" export CLICKHOUSE_URL_HTTPS="${CLICKHOUSE_URL_HTTPS}?${CLICKHOUSE_URL_PARAMS}" @@ -65,7 +67,7 @@ export CLICKHOUSE_URL_INTERSERVER=${CLICKHOUSE_URL_INTERSERVER:="${CLICKHOUSE_PO export CLICKHOUSE_CURL_COMMAND=${CLICKHOUSE_CURL_COMMAND:="curl"} export CLICKHOUSE_CURL_TIMEOUT=${CLICKHOUSE_CURL_TIMEOUT:="10"} -export CLICKHOUSE_CURL=${CLICKHOUSE_CURL:="${CLICKHOUSE_CURL_COMMAND} -q --max-time ${CLICKHOUSE_CURL_TIMEOUT}"} +export CLICKHOUSE_CURL=${CLICKHOUSE_CURL:="${CLICKHOUSE_CURL_COMMAND} -q -s --max-time ${CLICKHOUSE_CURL_TIMEOUT}"} export CLICKHOUSE_TMP=${CLICKHOUSE_TMP:="."} mkdir -p ${CLICKHOUSE_TMP} From c02f9e1dd1ca825a6dba653b2fb6cf3722ea195d Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Sat, 29 Aug 2020 01:48:48 +0300 Subject: [PATCH 008/145] remove some debug output to pass the tests --- src/Interpreters/Context.cpp | 2 -- src/Interpreters/executeQuery.cpp | 2 -- 2 files changed, 4 deletions(-) diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 362e762df6d..4bc66173a9c 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -1093,7 +1093,6 @@ void Context::setCurrentQueryId(const String & query_id) random.words.a = thread_local_rng(); //-V656 random.words.b = thread_local_rng(); //-V656 - fmt::print(stderr, "traceid {}, ==0 {}\n", client_info.opentelemetry_trace_id, client_info.opentelemetry_trace_id == 0); if (client_info.opentelemetry_trace_id == 0) { // If trace_id is not initialized, it means that this is an initial query @@ -1109,7 +1108,6 @@ void Context::setCurrentQueryId(const String & query_id) client_info.opentelemetry_parent_span_id = client_info.opentelemetry_span_id; client_info.opentelemetry_span_id = thread_local_rng(); } - fmt::print(stderr, "traceid {}, ==0 {}\n{}\n", client_info.opentelemetry_trace_id, client_info.opentelemetry_trace_id == 0, StackTrace().toString()); String query_id_to_set = query_id; if (query_id_to_set.empty()) /// If the user did not submit his query_id, then we generate it ourselves. diff --git a/src/Interpreters/executeQuery.cpp b/src/Interpreters/executeQuery.cpp index 05a0211adce..1da12d993ea 100644 --- a/src/Interpreters/executeQuery.cpp +++ b/src/Interpreters/executeQuery.cpp @@ -152,8 +152,6 @@ static void logQuery(const String & query, const Context & context, bool interna "OpenTelemetry trace id {:x}, span id {}, parent span id {}", context.getClientInfo().opentelemetry_trace_id, context.getClientInfo().opentelemetry_span_id, context.getClientInfo().opentelemetry_parent_span_id); - - std::cerr << StackTrace().toString() << std::endl; } } From fa8eebed780de4b79c000613341da25d4efe6f4a Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Sat, 29 Aug 2020 02:25:30 +0300 Subject: [PATCH 009/145] more tests + clickhouse-client options --- programs/client/Client.cpp | 21 ++++++++ .../0_stateless/01455_opentelemetry.sh | 31 ----------- ...01455_opentelemetry_distributed.reference} | 3 ++ .../01455_opentelemetry_distributed.sh | 52 +++++++++++++++++++ 4 files changed, 76 insertions(+), 31 deletions(-) delete mode 100755 tests/queries/0_stateless/01455_opentelemetry.sh rename tests/queries/0_stateless/{01455_opentelemetry.reference => 01455_opentelemetry_distributed.reference} (50%) create mode 100755 tests/queries/0_stateless/01455_opentelemetry_distributed.sh diff --git a/programs/client/Client.cpp b/programs/client/Client.cpp index e3a9b68dc47..fc2adff337e 100644 --- a/programs/client/Client.cpp +++ b/programs/client/Client.cpp @@ -2180,6 +2180,8 @@ public: ("log-level", po::value(), "client log level") ("server_logs_file", po::value(), "put server logs into specified file") ("query-fuzzer-runs", po::value()->default_value(0), "query fuzzer runs") + ("opentelemetry-traceparent", po::value(), "OpenTelemetry traceparent header as described by W3C Trace Context recommendation") + ("opentelemetry-tracestate", po::value(), "OpenTelemetry tracestate header as described by W3C Trace Context recommendation") ; Settings cmd_settings; @@ -2348,6 +2350,25 @@ public: ignore_error = true; } + if (options.count("opentelemetry-traceparent")) + { + std::string traceparent = options["opentelemetry-traceparent"].as(); + std::string error; + if (!context.getClientInfo().setOpenTelemetryTraceparent( + traceparent, error)) + { + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "Cannot parse OpenTelemetry traceparent '{}': {}", + traceparent, error); + } + } + + if (options.count("opentelemetry-tracestate")) + { + context.getClientInfo().opentelemetry_tracestate = + options["opentelemetry-tracestate"].as(); + } + argsToConfig(common_arguments, config(), 100); clearPasswordFromCommandLine(argc, argv); diff --git a/tests/queries/0_stateless/01455_opentelemetry.sh b/tests/queries/0_stateless/01455_opentelemetry.sh deleted file mode 100755 index f537377dfe6..00000000000 --- a/tests/queries/0_stateless/01455_opentelemetry.sh +++ /dev/null @@ -1,31 +0,0 @@ -#!/usr/bin/env bash -set -ue - -CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) -. "$CURDIR"/../shell_config.sh - -# Generate some random trace id so that the prevous runs of the test do not interfere. -trace_id=$(${CLICKHOUSE_CLIENT} -q "select lower(hex(reverse(reinterpretAsString(generateUUIDv4()))))") - -${CLICKHOUSE_CURL} --header "traceparent: 00-$trace_id-0000000000000010-01" --header "tracestate: some custom state" "http://localhost:8123/" --get --data-urlencode "query=select 1 from remote('127.0.0.2', system, one)" - -${CLICKHOUSE_CLIENT} -q "system flush logs" - -# Check that the HTTP traceparent was read, and then passed to `remote` instance. -# We expect 4 queries, because there are two DESC TABLE queries for the shard. -# This is bug-ish, see https://github.com/ClickHouse/ClickHouse/issues/14228 -${CLICKHOUSE_CLIENT} -q "select count(*) from system.opentelemetry_log where trace_id = reinterpretAsUUID(reverse(unhex('$trace_id')))" - -# Check that the tracestate header was read and passed. Must have -# exactly the same value for all "query" spans in this trace. -${CLICKHOUSE_CLIENT} -q " - select count(distinct attribute.values) - from system.opentelemetry_log - array join attribute.names, attribute.values - where - trace_id = reinterpretAsUUID(reverse(unhex('$trace_id'))) - and operation_name = 'query' - and attribute.names = 'tracestate' -" - - diff --git a/tests/queries/0_stateless/01455_opentelemetry.reference b/tests/queries/0_stateless/01455_opentelemetry_distributed.reference similarity index 50% rename from tests/queries/0_stateless/01455_opentelemetry.reference rename to tests/queries/0_stateless/01455_opentelemetry_distributed.reference index b0484d7df0b..5993b628ad4 100644 --- a/tests/queries/0_stateless/01455_opentelemetry.reference +++ b/tests/queries/0_stateless/01455_opentelemetry_distributed.reference @@ -1,3 +1,6 @@ 1 4 1 +1 +2 +1 diff --git a/tests/queries/0_stateless/01455_opentelemetry_distributed.sh b/tests/queries/0_stateless/01455_opentelemetry_distributed.sh new file mode 100755 index 00000000000..df5c194b2be --- /dev/null +++ b/tests/queries/0_stateless/01455_opentelemetry_distributed.sh @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +set -ue + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +. "$CURDIR"/../shell_config.sh + +function check_log +{ +${CLICKHOUSE_CLIENT} -nq " +system flush logs; + +-- Check the number of spans with given trace id, to verify it was propagated. +select count(*) + from system.opentelemetry_log + where trace_id = reinterpretAsUUID(reverse(unhex('$trace_id'))) + and operation_name = 'query' + ; + +-- Check that the tracestate header was propagated. It must have exactly the +-- same non-empty value for all 'query' spans in this trace. +select count(distinct value) + from system.opentelemetry_log + array join attribute.names as name, attribute.values as value + where + trace_id = reinterpretAsUUID(reverse(unhex('$trace_id'))) + and operation_name = 'query' + and name = 'tracestate' + and length(value) > 0 + ; +" +} + +# Generate some random trace id so that the prevous runs of the test do not interfere. +trace_id=$(${CLICKHOUSE_CLIENT} -q "select lower(hex(reverse(reinterpretAsString(generateUUIDv4()))))") + +# Check that the HTTP traceparent is read, and then passed through `remote` table function. +# We expect 4 queries, because there are two DESC TABLE queries for the shard. +# This is bug-ish, see https://github.com/ClickHouse/ClickHouse/issues/14228 +${CLICKHOUSE_CURL} --header "traceparent: 00-$trace_id-0000000000000010-01" --header "tracestate: some custom state" "http://localhost:8123/" --get --data-urlencode "query=select 1 from remote('127.0.0.2', system, one)" + +check_log + +# With another trace id, check that clickhouse-client accepts traceparent, and +# that it is passed through URL table function. We expect two query spans, one +# for the initial query, and one for the HTTP query. +trace_id=$(${CLICKHOUSE_CLIENT} -q "select lower(hex(reverse(reinterpretAsString(generateUUIDv4()))))") + +${CLICKHOUSE_CLIENT} --opentelemetry-traceparent "00-$trace_id-0000000000000020-02" --opentelemetry-tracestate "another custom state" --query " + select * from url('http://127.0.0.2:8123/?query=select%201', CSV, 'a int') +" + +check_log From d0a9926e7dba352adfe6e89c3fa8a3bd8f940377 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Tue, 8 Sep 2020 16:19:27 +0300 Subject: [PATCH 010/145] fixes and some docs --- docs/en/operations/opentelemetry-draft.md | 61 +++++++++++++++++++ programs/server/config.xml | 15 +++++ src/Core/Settings.h | 1 + src/Interpreters/ClientInfo.cpp | 49 ++++++++++----- src/Interpreters/ClientInfo.h | 2 +- src/Interpreters/Context.cpp | 26 +++++--- src/Interpreters/OpenTelemetryLog.cpp | 11 ++-- src/Interpreters/OpenTelemetryLog.h | 5 +- src/Interpreters/executeQuery.cpp | 54 +++++++++------- src/Server/TCPHandler.cpp | 26 +++++--- .../01455_opentelemetry_distributed.reference | 4 ++ .../01455_opentelemetry_distributed.sh | 41 +++++++++++-- 12 files changed, 226 insertions(+), 69 deletions(-) create mode 100644 docs/en/operations/opentelemetry-draft.md diff --git a/docs/en/operations/opentelemetry-draft.md b/docs/en/operations/opentelemetry-draft.md new file mode 100644 index 00000000000..3363b37b6d6 --- /dev/null +++ b/docs/en/operations/opentelemetry-draft.md @@ -0,0 +1,61 @@ +# [draft] OpenTelemetry support + +[OpenTelemetry](https://opentelemetry.io/) is an open standard for collecting +traces and metrics from distributed application. ClickHouse has some support +for OpenTelemetry. + + +## Supplying Trace Context to ClickHouse + +ClickHouse accepts trace context HTTP headers, as described by +the [W3C recommendation](https://www.w3.org/TR/trace-context/). +It also accepts trace context over native protocol that is used for +communication between ClickHouse servers or between the client and server. +For manual testing, trace context headers conforming to the Trace Context +recommendation can be supplied to `clickhouse-client` using +`--opentelemetry-traceparent` and `--opentelemetry-tracestate` flags. + +If no parent trace context is supplied, ClickHouse can start a new trace, with +probability controlled by the `opentelemetry_start_trace_probability` setting. + + +## Propagating the Trace Context + +The trace context is propagated to downstream services in the following cases: + +* Queries to remote ClickHouse servers, such as when using `Distributed` table + engine. + +* `URL` table function. Trace context information is sent in HTTP headers. + + +## Tracing the ClickHouse Itself + +ClickHouse creates _trace spans_ for each query and some of the query execution +stages, such as query planning or distributed queries. + +To be useful, the tracing information has to be exported to a monitoring system +that supports OpenTelemetry, such as Jaeger or Prometheus. ClickHouse avoids +a dependency on a particular monitoring system, instead only +providing the tracing data conforming to the standard. A natural way to do so +in an SQL RDBMS is a system table. OpenTelemetry trace span information +[required by the standard](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/overview.md#span) +is stored in the system table called `system.opentelemetry_log`. + +The table must be enabled in the server configuration, see the `opentelemetry_log` +element in the default config file `config.xml`. It is enabled by default. + +The table has the following columns: + +- `trace_id` +- `span_id` +- `parent_span_id` +- `operation_name` +- `start_time` +- `finish_time` +- `finish_date` +- `attribute.name` +- `attribute.values` + +The tags or attributes are saved as two parallel arrays, containing the keys +and values. Use `ARRAY JOIN` to work with them. diff --git a/programs/server/config.xml b/programs/server/config.xml index 7e88150b95f..913f43fc435 100644 --- a/programs/server/config.xml +++ b/programs/server/config.xml @@ -567,6 +567,21 @@ OpenTelemetry log contains OpenTelemetry trace spans. --> + + + engine MergeTree + partition by toYYYYMM(finish_date) + order by (finish_date, finish_time, trace_id) + system opentelemetry_log
7500 diff --git a/src/Core/Settings.h b/src/Core/Settings.h index f7438b356d5..bb3baae6ba4 100644 --- a/src/Core/Settings.h +++ b/src/Core/Settings.h @@ -218,6 +218,7 @@ class IColumn; M(UInt64, query_profiler_cpu_time_period_ns, 1000000000, "Period for CPU clock timer of query profiler (in nanoseconds). Set 0 value to turn off the CPU clock query profiler. Recommended value is at least 10000000 (100 times a second) for single queries or 1000000000 (once a second) for cluster-wide profiling.", 0) \ M(Bool, metrics_perf_events_enabled, false, "If enabled, some of the perf events will be measured throughout queries' execution.", 0) \ M(String, metrics_perf_events_list, "", "Comma separated list of perf metrics that will be measured throughout queries' execution. Empty means all events. See PerfEventInfo in sources for the available events.", 0) \ + M(Float, opentelemetry_start_trace_probability, 0., "Probability to start an OpenTelemetry trace for an incoming query.", 0) \ \ \ /** Limits during query execution are part of the settings. \ diff --git a/src/Interpreters/ClientInfo.cpp b/src/Interpreters/ClientInfo.cpp index f7ed8eafa46..da9f14ec879 100644 --- a/src/Interpreters/ClientInfo.cpp +++ b/src/Interpreters/ClientInfo.cpp @@ -65,11 +65,19 @@ void ClientInfo::write(WriteBuffer & out, const UInt64 server_protocol_revision) { // No point writing these numbers with variable length, because they // are random and will probably require the full length anyway. - writeBinary(opentelemetry_trace_id, out); - writeBinary(opentelemetry_span_id, out); - writeBinary(opentelemetry_parent_span_id, out); - writeBinary(opentelemetry_tracestate, out); - writeBinary(opentelemetry_trace_flags, out); + if (opentelemetry_trace_id) + { + writeBinary(uint8_t(1), out); + writeBinary(opentelemetry_trace_id, out); + writeBinary(opentelemetry_span_id, out); + writeBinary(opentelemetry_parent_span_id, out); + writeBinary(opentelemetry_tracestate, out); + writeBinary(opentelemetry_trace_flags, out); + } + else + { + writeBinary(uint8_t(0), out); + } } } @@ -125,15 +133,24 @@ void ClientInfo::read(ReadBuffer & in, const UInt64 client_protocol_revision) client_version_patch = client_revision; } + // TODO what does it even mean to read this structure over HTTP? I thought + // this was for native protocol? See interface == Interface::HTTP. if (client_protocol_revision >= DBMS_MIN_REVISION_WITH_OPENTELEMETRY) { - readBinary(opentelemetry_trace_id, in); - readBinary(opentelemetry_span_id, in); - readBinary(opentelemetry_parent_span_id, in); - readBinary(opentelemetry_tracestate, in); - readBinary(opentelemetry_trace_flags, in); + uint8_t have_trace_id = 0; + readBinary(have_trace_id, in); + if (have_trace_id) + { + readBinary(opentelemetry_trace_id, in); + readBinary(opentelemetry_span_id, in); + readBinary(opentelemetry_parent_span_id, in); + readBinary(opentelemetry_tracestate, in); + readBinary(opentelemetry_trace_flags, in); - std::cerr << fmt::format("read {:x}, {}, {}\n", opentelemetry_trace_id, opentelemetry_span_id, opentelemetry_parent_span_id) << StackTrace().toString() << std::endl; + fmt::print(stderr, "read {:x}, {}, {} at\n{}\n", + opentelemetry_trace_id, opentelemetry_span_id, + opentelemetry_parent_span_id, StackTrace().toString()); + } } } @@ -149,8 +166,8 @@ bool ClientInfo::setOpenTelemetryTraceparent(const std::string & traceparent, std::string & error) { uint8_t version = -1; - __uint64_t trace_id_high = 0; - __uint64_t trace_id_low = 0; + uint64_t trace_id_high = 0; + uint64_t trace_id_low = 0; uint64_t trace_parent = 0; uint8_t trace_flags = 0; @@ -205,11 +222,11 @@ bool ClientInfo::setOpenTelemetryTraceparent(const std::string & traceparent, std::string ClientInfo::getOpenTelemetryTraceparentForChild() const { - // This span is a parent for its children (so deep...), so we specify - // this span_id as a parent id. + // 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}", opentelemetry_trace_id, opentelemetry_span_id, - // This cast is because fmt is being weird and complaining that + // This cast is needed because fmt is being weird and complaining that // "mixing character types is not allowed". static_cast(opentelemetry_trace_flags)); } diff --git a/src/Interpreters/ClientInfo.h b/src/Interpreters/ClientInfo.h index 604a9771a52..f57d1853b76 100644 --- a/src/Interpreters/ClientInfo.h +++ b/src/Interpreters/ClientInfo.h @@ -69,7 +69,7 @@ public: // the incoming tracestate header, we just pass it downstream. // https://www.w3.org/TR/trace-context/ String opentelemetry_tracestate; - UInt8 opentelemetry_trace_flags; + UInt8 opentelemetry_trace_flags = 0; /// All below are parameters related to initial query. diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 4bc66173a9c..e12747a7f95 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -1093,18 +1093,28 @@ void Context::setCurrentQueryId(const String & query_id) random.words.a = thread_local_rng(); //-V656 random.words.b = thread_local_rng(); //-V656 - if (client_info.opentelemetry_trace_id == 0) + if (client_info.query_kind == ClientInfo::QueryKind::INITIAL_QUERY + && client_info.opentelemetry_trace_id == 0) { - // If trace_id is not initialized, it means that this is an initial query - // without any parent OpenTelemetry trace. Use the randomly generated - // default query id as the new trace id. - client_info.opentelemetry_trace_id = random.uuid; - client_info.opentelemetry_parent_span_id = 0; - client_info.opentelemetry_span_id = thread_local_rng(); + // If this is an initial query without any parent OpenTelemetry trace, we + // might start the trace ourselves, with some configurable probability. + std::bernoulli_distribution should_start_trace{ + settings.opentelemetry_start_trace_probability}; + + if (should_start_trace(thread_local_rng)) + { + // Use the randomly generated default query id as the new trace id. + client_info.opentelemetry_trace_id = random.uuid; + client_info.opentelemetry_parent_span_id = 0; + client_info.opentelemetry_span_id = thread_local_rng(); + // Mark this trace as sampled in the flags. + client_info.opentelemetry_trace_flags = 1; + } } else { - // The incoming span id becomes our parent span id. + // The incoming request has an OpenTelemtry trace context. Its span id + // becomes our parent span id. client_info.opentelemetry_parent_span_id = client_info.opentelemetry_span_id; client_info.opentelemetry_span_id = thread_local_rng(); } diff --git a/src/Interpreters/OpenTelemetryLog.cpp b/src/Interpreters/OpenTelemetryLog.cpp index f8d7d684478..161af01107f 100644 --- a/src/Interpreters/OpenTelemetryLog.cpp +++ b/src/Interpreters/OpenTelemetryLog.cpp @@ -13,16 +13,13 @@ namespace DB Block OpenTelemetrySpanLogElement::createBlock() { return { - // event_date is the date part of event_time, used for indexing. - {std::make_shared(), "event_date"}, - // event_time is the span start time, named so to be compatible with - // the standard ClickHouse system log column names. - {std::make_shared(), "event_time"}, {std::make_shared(), "trace_id"}, {std::make_shared(), "span_id"}, {std::make_shared(), "parent_span_id"}, {std::make_shared(), "operation_name"}, + {std::make_shared(), "start_time"}, {std::make_shared(), "finish_time"}, + {std::make_shared(), "finish_date"}, {std::make_shared(std::make_shared()), "attribute.names"}, {std::make_shared(std::make_shared()), @@ -34,13 +31,13 @@ void OpenTelemetrySpanLogElement::appendToBlock(MutableColumns & columns) const { size_t i = 0; - columns[i++]->insert(DateLUT::instance().toDayNum(start_time)); - columns[i++]->insert(start_time); columns[i++]->insert(UInt128(Int128(trace_id))); columns[i++]->insert(span_id); columns[i++]->insert(parent_span_id); columns[i++]->insert(operation_name); + columns[i++]->insert(start_time); columns[i++]->insert(finish_time); + columns[i++]->insert(DateLUT::instance().toDayNum(finish_time)); columns[i++]->insert(attribute_names); columns[i++]->insert(attribute_values); } diff --git a/src/Interpreters/OpenTelemetryLog.h b/src/Interpreters/OpenTelemetryLog.h index 73ad5382c95..e2906a99a02 100644 --- a/src/Interpreters/OpenTelemetryLog.h +++ b/src/Interpreters/OpenTelemetryLog.h @@ -23,8 +23,9 @@ struct OpenTelemetrySpan UInt64 span_id; UInt64 parent_span_id; std::string operation_name; - time_t start_time{}; - time_t finish_time{}; + time_t start_time; + time_t finish_time; + UInt64 duration_ns; Array attribute_names; Array attribute_values; // I don't understand how Links work, namely, which direction should they diff --git a/src/Interpreters/executeQuery.cpp b/src/Interpreters/executeQuery.cpp index 1da12d993ea..141a9ade57f 100644 --- a/src/Interpreters/executeQuery.cpp +++ b/src/Interpreters/executeQuery.cpp @@ -138,20 +138,26 @@ static void logQuery(const String & query, const Context & context, bool interna } else { - const auto & current_query_id = context.getClientInfo().current_query_id; - const auto & initial_query_id = context.getClientInfo().initial_query_id; - const auto & current_user = context.getClientInfo().current_user; + const auto & client_info = context.getClientInfo(); + + const auto & current_query_id = client_info.current_query_id; + const auto & initial_query_id = client_info.initial_query_id; + const auto & current_user = client_info.current_user; LOG_DEBUG(&Poco::Logger::get("executeQuery"), "(from {}{}{}) {}", - context.getClientInfo().current_address.toString(), - (current_user != "default" ? ", user: " + context.getClientInfo().current_user : ""), + client_info.current_address.toString(), + (current_user != "default" ? ", user: " + current_user : ""), (!initial_query_id.empty() && current_query_id != initial_query_id ? ", initial_query_id: " + initial_query_id : std::string()), joinLines(query)); - LOG_TRACE(&Poco::Logger::get("executeQuery"), - "OpenTelemetry trace id {:x}, span id {}, parent span id {}", - context.getClientInfo().opentelemetry_trace_id, context.getClientInfo().opentelemetry_span_id, - context.getClientInfo().opentelemetry_parent_span_id); + if (client_info.opentelemetry_trace_id) + { + LOG_TRACE(&Poco::Logger::get("executeQuery"), + "OpenTelemetry trace id {:x}, span id {}, parent span id {}", + client_info.opentelemetry_trace_id, + client_info.opentelemetry_span_id, + client_info.opentelemetry_parent_span_id); + } } } @@ -222,7 +228,9 @@ static void onExceptionBeforeStart(const String & query_for_logging, Context & c if (auto query_log = context.getQueryLog()) query_log->add(elem); - if (auto opentelemetry_log = context.getOpenTelemetryLog()) + if (auto opentelemetry_log = context.getOpenTelemetryLog(); + context.getClientInfo().opentelemetry_trace_id + && opentelemetry_log) { OpenTelemetrySpanLogElement span; span.trace_id = context.getClientInfo().opentelemetry_trace_id; @@ -231,20 +239,21 @@ static void onExceptionBeforeStart(const String & query_for_logging, Context & c span.operation_name = "query"; span.start_time = current_time; span.finish_time = current_time; + span.duration_ns = 0; // keep values synchonized to type enum in QueryLogElement::createBlock - span.attribute_names.push_back("status"); + span.attribute_names.push_back("clickhouse.query_status"); span.attribute_values.push_back("ExceptionBeforeStart"); - span.attribute_names.push_back("query"); + span.attribute_names.push_back("db.statement"); span.attribute_values.push_back(elem.query); - span.attribute_names.push_back("query_id"); + span.attribute_names.push_back("clickhouse.query_id"); span.attribute_values.push_back(elem.client_info.current_query_id); if (!context.getClientInfo().opentelemetry_tracestate.empty()) { - span.attribute_names.push_back("tracestate"); + span.attribute_names.push_back("clickhouse.tracestate"); span.attribute_values.push_back( context.getClientInfo().opentelemetry_tracestate); } @@ -285,7 +294,7 @@ static std::tuple executeQueryImpl( bool has_query_tail, ReadBuffer * istr) { - time_t current_time = time(nullptr); + const time_t current_time = time(nullptr); /// If we already executing query and it requires to execute internal query, than /// don't replace thread context with given (it can be temporary). Otherwise, attach context to thread. @@ -621,7 +630,9 @@ static std::tuple executeQueryImpl( query_log->add(elem); } - if (auto opentelemetry_log = context.getOpenTelemetryLog()) + if (auto opentelemetry_log = context.getOpenTelemetryLog(); + context.getClientInfo().opentelemetry_trace_id + && opentelemetry_log) { OpenTelemetrySpanLogElement span; span.trace_id = context.getClientInfo().opentelemetry_trace_id; @@ -629,20 +640,21 @@ static std::tuple executeQueryImpl( span.parent_span_id = context.getClientInfo().opentelemetry_parent_span_id; span.operation_name = "query"; span.start_time = elem.query_start_time; - span.finish_time = time(nullptr); // current time + span.finish_time = elem.event_time; + span.duration_ns = elapsed_seconds * 1000000000; // keep values synchonized to type enum in QueryLogElement::createBlock - span.attribute_names.push_back("status"); + span.attribute_names.push_back("clickhouse.query_status"); span.attribute_values.push_back("QueryFinish"); - span.attribute_names.push_back("query"); + span.attribute_names.push_back("db.statement"); span.attribute_values.push_back(elem.query); - span.attribute_names.push_back("query_id"); + span.attribute_names.push_back("clickhouse.query_id"); span.attribute_values.push_back(elem.client_info.current_query_id); if (!context.getClientInfo().opentelemetry_tracestate.empty()) { - span.attribute_names.push_back("tracestate"); + span.attribute_names.push_back("clickhouse.tracestate"); span.attribute_values.push_back( context.getClientInfo().opentelemetry_tracestate); } diff --git a/src/Server/TCPHandler.cpp b/src/Server/TCPHandler.cpp index e83bbb02cad..49f737c9550 100644 --- a/src/Server/TCPHandler.cpp +++ b/src/Server/TCPHandler.cpp @@ -850,12 +850,6 @@ void TCPHandler::receiveQuery() if (client_revision >= DBMS_MIN_REVISION_WITH_CLIENT_INFO) client_info.read(*in, client_revision); - // It is convenient to generate default OpenTelemetry trace id and default - // query id together. ClientInfo might contain upstream trace id, so we - // decide whether to use the default ids after we have received the ClientInfo. - // We also set up the parent span id while we're at it. - query_context->setCurrentQueryId(state.query_id); - /// For better support of old clients, that does not send ClientInfo. if (client_info.query_kind == ClientInfo::QueryKind::NO_QUERY) { @@ -884,8 +878,11 @@ void TCPHandler::receiveQuery() /// Per query settings are also passed via TCP. /// We need to check them before applying due to they can violate the settings constraints. - auto settings_format = (client_revision >= DBMS_MIN_REVISION_WITH_SETTINGS_SERIALIZED_AS_STRINGS) ? SettingsWriteFormat::STRINGS_WITH_FLAGS - : SettingsWriteFormat::BINARY; + auto settings_format = + (client_revision >= DBMS_MIN_REVISION_WITH_SETTINGS_SERIALIZED_AS_STRINGS) + ? SettingsWriteFormat::STRINGS_WITH_FLAGS + : SettingsWriteFormat::BINARY; + Settings passed_settings; passed_settings.read(*in, settings_format); auto settings_changes = passed_settings.changes(); @@ -900,12 +897,23 @@ void TCPHandler::receiveQuery() query_context->clampToSettingsConstraints(settings_changes); } query_context->applySettingsChanges(settings_changes); - const Settings & settings = query_context->getSettingsRef(); + + // Use the received query id, or generate a random default. It is convenient + // to also generate the default OpenTelemetry trace id at the same time, and + // and and set the trace parent. + // Why is this done here and not earlier: + // 1) ClientInfo might contain upstream trace id, so we decide whether to use + // the default ids after we have received the ClientInfo. + // 2) There is the opentelemetry_start_trace_probability setting that + // controls when we start a new trace. It can be changed via Native protocol, + // so we have to apply the changes first. + query_context->setCurrentQueryId(state.query_id); /// Sync timeouts on client and server during current query to avoid dangling queries on server /// NOTE: We use settings.send_timeout for the receive timeout and vice versa (change arguments ordering in TimeoutSetter), /// because settings.send_timeout is client-side setting which has opposite meaning on the server side. /// NOTE: these settings are applied only for current connection (not for distributed tables' connections) + const Settings & settings = query_context->getSettingsRef(); state.timeout_setter = std::make_unique(socket(), settings.receive_timeout, settings.send_timeout); readVarUInt(stage, *in); diff --git a/tests/queries/0_stateless/01455_opentelemetry_distributed.reference b/tests/queries/0_stateless/01455_opentelemetry_distributed.reference index 5993b628ad4..176626befa7 100644 --- a/tests/queries/0_stateless/01455_opentelemetry_distributed.reference +++ b/tests/queries/0_stateless/01455_opentelemetry_distributed.reference @@ -1,6 +1,10 @@ +===http=== 1 4 1 +===native=== 1 2 1 +===sampled=== +1 1 diff --git a/tests/queries/0_stateless/01455_opentelemetry_distributed.sh b/tests/queries/0_stateless/01455_opentelemetry_distributed.sh index df5c194b2be..1cb65113179 100755 --- a/tests/queries/0_stateless/01455_opentelemetry_distributed.sh +++ b/tests/queries/0_stateless/01455_opentelemetry_distributed.sh @@ -24,29 +24,60 @@ select count(distinct value) where trace_id = reinterpretAsUUID(reverse(unhex('$trace_id'))) and operation_name = 'query' - and name = 'tracestate' + and name = 'clickhouse.tracestate' and length(value) > 0 ; " } # Generate some random trace id so that the prevous runs of the test do not interfere. +echo "===http===" trace_id=$(${CLICKHOUSE_CLIENT} -q "select lower(hex(reverse(reinterpretAsString(generateUUIDv4()))))") # Check that the HTTP traceparent is read, and then passed through `remote` table function. # We expect 4 queries, because there are two DESC TABLE queries for the shard. # This is bug-ish, see https://github.com/ClickHouse/ClickHouse/issues/14228 -${CLICKHOUSE_CURL} --header "traceparent: 00-$trace_id-0000000000000010-01" --header "tracestate: some custom state" "http://localhost:8123/" --get --data-urlencode "query=select 1 from remote('127.0.0.2', system, one)" +${CLICKHOUSE_CURL} \ + --header "traceparent: 00-$trace_id-0000000000000010-01" \ + --header "tracestate: some custom state" "http://localhost:8123/" \ + --get \ + --data-urlencode "query=select 1 from remote('127.0.0.2', system, one)" check_log # With another trace id, check that clickhouse-client accepts traceparent, and # that it is passed through URL table function. We expect two query spans, one # for the initial query, and one for the HTTP query. +echo "===native===" trace_id=$(${CLICKHOUSE_CLIENT} -q "select lower(hex(reverse(reinterpretAsString(generateUUIDv4()))))") -${CLICKHOUSE_CLIENT} --opentelemetry-traceparent "00-$trace_id-0000000000000020-02" --opentelemetry-tracestate "another custom state" --query " - select * from url('http://127.0.0.2:8123/?query=select%201', CSV, 'a int') -" +${CLICKHOUSE_CLIENT} \ + --opentelemetry-traceparent "00-$trace_id-0000000000000020-02" \ + --opentelemetry-tracestate "another custom state" \ + --query "select * from url('http://127.0.0.2:8123/?query=select%201', CSV, 'a int')" check_log + +# Test sampled tracing. The traces should be started with the specified probability, +# only for initial queries. +echo "===sampled===" +query_id=$(${CLICKHOUSE_CLIENT} -q "select lower(hex(reverse(reinterpretAsString(generateUUIDv4()))))") + +for _ in {1..200} +do + ${CLICKHOUSE_CLIENT} \ + --opentelemetry_start_trace_probability=0.1 \ + --query_id "$query_id" \ + --query "select 1 from remote('127.0.0.2', system, one) format Null" +done + +${CLICKHOUSE_CLIENT} -q " + with count(*) as c + -- expect 200 * 0.1 = 20 sampled events on average + select c > 10, c < 30 + from system.opentelemetry_log + array join attribute.names as name, attribute.values as value + where name = 'clickhouse.query_id' + and value = '$query_id' + ; +" From f0c5459807baa594c5138a26a0fdd6b3714f6c9c Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Wed, 9 Sep 2020 14:05:49 +0300 Subject: [PATCH 011/145] tests --- src/Core/BaseSettings.h | 22 +++++++++++++------ .../queries/0_stateless/00396_uuid.reference | 1 + tests/queries/0_stateless/00396_uuid.sql | 6 +++++ .../01455_opentelemetry_distributed.reference | 2 +- .../01455_opentelemetry_distributed.sh | 2 +- 5 files changed, 24 insertions(+), 9 deletions(-) diff --git a/src/Core/BaseSettings.h b/src/Core/BaseSettings.h index 7de87b345c1..b193fdd4c93 100644 --- a/src/Core/BaseSettings.h +++ b/src/Core/BaseSettings.h @@ -390,13 +390,21 @@ String BaseSettings::valueToStringUtil(const std::string_view & name, c template Field BaseSettings::stringToValueUtil(const std::string_view & name, const String & str) { - const auto & accessor = Traits::Accessor::instance(); - if (size_t index = accessor.find(name); index != static_cast(-1)) - return accessor.stringToValueUtil(index, str); - if constexpr (Traits::allow_custom_settings) - return Field::restoreFromDump(str); - else - BaseSettingsHelpers::throwSettingNotFound(name); + try + { + const auto & accessor = Traits::Accessor::instance(); + if (size_t index = accessor.find(name); index != static_cast(-1)) + return accessor.stringToValueUtil(index, str); + if constexpr (Traits::allow_custom_settings) + return Field::restoreFromDump(str); + else + BaseSettingsHelpers::throwSettingNotFound(name); + } + catch (Exception & e) + { + e.addMessage("while parsing value '{}' for setting '{}'", str, name); + throw; + } } template diff --git a/tests/queries/0_stateless/00396_uuid.reference b/tests/queries/0_stateless/00396_uuid.reference index fe92b3684a6..d70322ec4c1 100644 --- a/tests/queries/0_stateless/00396_uuid.reference +++ b/tests/queries/0_stateless/00396_uuid.reference @@ -5,3 +5,4 @@ 01234567-89ab-cdef-0123-456789abcdef 01234567-89ab-cdef-0123-456789abcdef 01234567-89ab-cdef-0123-456789abcdef 01234567-89ab-cdef-0123-456789abcdef 01234567-89ab-cdef-0123-456789abcdef 01234567-89ab-cdef-0123-456789abcdef 3f1ed72e-f7fe-4459-9cbe-95fe9298f845 +1 diff --git a/tests/queries/0_stateless/00396_uuid.sql b/tests/queries/0_stateless/00396_uuid.sql index d671ce844e2..863ff13f5c2 100644 --- a/tests/queries/0_stateless/00396_uuid.sql +++ b/tests/queries/0_stateless/00396_uuid.sql @@ -5,3 +5,9 @@ SELECT hex(UUIDStringToNum(materialize('01234567-89ab-cdef-0123-456789abcdef'))) SELECT '01234567-89ab-cdef-0123-456789abcdef' AS str, UUIDNumToString(UUIDStringToNum(str)), UUIDNumToString(UUIDStringToNum(toFixedString(str, 36))); SELECT materialize('01234567-89ab-cdef-0123-456789abcdef') AS str, UUIDNumToString(UUIDStringToNum(str)), UUIDNumToString(UUIDStringToNum(toFixedString(str, 36))); SELECT toString(toUUID('3f1ed72e-f7fe-4459-9cbe-95fe9298f845')); + +-- conversion back and forth to big-endian hex string +with generateUUIDv4() as uuid, + identity(lower(hex(reverse(reinterpretAsString(uuid))))) as str, + reinterpretAsUUID(reverse(unhex(str))) uuid2 +select uuid = uuid2; diff --git a/tests/queries/0_stateless/01455_opentelemetry_distributed.reference b/tests/queries/0_stateless/01455_opentelemetry_distributed.reference index 176626befa7..e0eadd91a97 100644 --- a/tests/queries/0_stateless/01455_opentelemetry_distributed.reference +++ b/tests/queries/0_stateless/01455_opentelemetry_distributed.reference @@ -7,4 +7,4 @@ 2 1 ===sampled=== -1 1 +OK diff --git a/tests/queries/0_stateless/01455_opentelemetry_distributed.sh b/tests/queries/0_stateless/01455_opentelemetry_distributed.sh index 1cb65113179..d3114ab66ff 100755 --- a/tests/queries/0_stateless/01455_opentelemetry_distributed.sh +++ b/tests/queries/0_stateless/01455_opentelemetry_distributed.sh @@ -74,7 +74,7 @@ done ${CLICKHOUSE_CLIENT} -q " with count(*) as c -- expect 200 * 0.1 = 20 sampled events on average - select c > 10, c < 30 + select if(c > 10 and c < 30, 'OK', 'fail: ' || toString(c)) from system.opentelemetry_log array join attribute.names as name, attribute.values as value where name = 'clickhouse.query_id' From 8212d7fa2838c3f8c5251cc9070ecda6f50b29cb Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Thu, 17 Sep 2020 15:14:30 +0300 Subject: [PATCH 012/145] fix the uuid test --- tests/queries/0_stateless/00396_uuid.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/00396_uuid.sql b/tests/queries/0_stateless/00396_uuid.sql index 863ff13f5c2..9d8b48bddb0 100644 --- a/tests/queries/0_stateless/00396_uuid.sql +++ b/tests/queries/0_stateless/00396_uuid.sql @@ -9,5 +9,5 @@ SELECT toString(toUUID('3f1ed72e-f7fe-4459-9cbe-95fe9298f845')); -- conversion back and forth to big-endian hex string with generateUUIDv4() as uuid, identity(lower(hex(reverse(reinterpretAsString(uuid))))) as str, - reinterpretAsUUID(reverse(unhex(str))) uuid2 + reinterpretAsUUID(reverse(unhex(str))) as uuid2 select uuid = uuid2; From 7b8ad02a124cad4cb90280cac856b60d54551fda Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Thu, 17 Sep 2020 15:14:49 +0300 Subject: [PATCH 013/145] cleanup --- src/Server/HTTPHandler.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Server/HTTPHandler.cpp b/src/Server/HTTPHandler.cpp index fb630010198..2f1e978d48f 100644 --- a/src/Server/HTTPHandler.cpp +++ b/src/Server/HTTPHandler.cpp @@ -326,8 +326,6 @@ void HTTPHandler::processQuery( } context.getClientInfo().opentelemetry_tracestate = request.get("tracestate", ""); - - } /// The client can pass a HTTP header indicating supported compression method (gzip or deflate). From a374541214c7a4bece9bf77df132eda23e4c6e85 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Thu, 17 Sep 2020 15:15:05 +0300 Subject: [PATCH 014/145] straighten the protocol version --- base/daemon/BaseDaemon.cpp | 2 +- programs/copier/ClusterCopierApp.cpp | 2 +- programs/server/Server.cpp | 2 +- src/Client/Connection.cpp | 4 +- src/Common/ClickHouseRevision.cpp | 2 +- src/Common/ClickHouseRevision.h | 2 +- src/Common/StatusFile.cpp | 2 +- src/Common/config_version.h.in | 13 +---- src/Core/Defines.h | 4 +- src/DataStreams/TemporaryFileStream.h | 3 +- src/Interpreters/Aggregator.cpp | 2 +- src/Interpreters/ClientInfo.cpp | 9 ++-- src/Interpreters/ClientInfo.h | 2 +- src/Interpreters/CrashLog.cpp | 2 +- src/Interpreters/QueryLog.cpp | 4 +- src/Interpreters/QueryThreadLog.cpp | 2 +- src/Interpreters/TextLog.cpp | 2 +- src/Interpreters/TraceLog.cpp | 2 +- .../Transforms/AggregatingTransform.cpp | 3 +- src/Server/TCPHandler.cpp | 53 +++++++++---------- src/Server/TCPHandler.h | 2 +- src/Storages/Distributed/DirectoryMonitor.cpp | 7 ++- .../DistributedBlockOutputStream.cpp | 7 ++- .../System/StorageSystemProcesses.cpp | 2 +- 24 files changed, 58 insertions(+), 77 deletions(-) diff --git a/base/daemon/BaseDaemon.cpp b/base/daemon/BaseDaemon.cpp index 78801e71a6f..22455d09cf2 100644 --- a/base/daemon/BaseDaemon.cpp +++ b/base/daemon/BaseDaemon.cpp @@ -781,7 +781,7 @@ void BaseDaemon::initializeTerminationAndSignalProcessing() void BaseDaemon::logRevision() const { Poco::Logger::root().information("Starting " + std::string{VERSION_FULL} - + " with revision " + std::to_string(ClickHouseRevision::get()) + + " with revision " + std::to_string(ClickHouseRevision::getVersionRevision()) + ", " + build_id_info + ", PID " + std::to_string(getpid())); } diff --git a/programs/copier/ClusterCopierApp.cpp b/programs/copier/ClusterCopierApp.cpp index ec64e118f45..08a7e50a9d7 100644 --- a/programs/copier/ClusterCopierApp.cpp +++ b/programs/copier/ClusterCopierApp.cpp @@ -105,7 +105,7 @@ void ClusterCopierApp::mainImpl() ThreadStatus thread_status; auto * log = &logger(); - LOG_INFO(log, "Starting clickhouse-copier (id {}, host_id {}, path {}, revision {})", process_id, host_id, process_path, ClickHouseRevision::get()); + LOG_INFO(log, "Starting clickhouse-copier (id {}, host_id {}, path {}, revision {})", process_id, host_id, process_path, ClickHouseRevision::getVersionRevision()); SharedContextHolder shared_context = Context::createShared(); auto context = std::make_unique(Context::createGlobal(shared_context.get())); diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index 56778b8dd69..3f3f8ccef3a 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -256,7 +256,7 @@ int Server::main(const std::vector & /*args*/) #endif #endif - CurrentMetrics::set(CurrentMetrics::Revision, ClickHouseRevision::get()); + CurrentMetrics::set(CurrentMetrics::Revision, ClickHouseRevision::getVersionRevision()); CurrentMetrics::set(CurrentMetrics::VersionInteger, ClickHouseRevision::getVersionInteger()); if (ThreadFuzzer::instance().isEffective()) diff --git a/src/Client/Connection.cpp b/src/Client/Connection.cpp index ed27a878b5a..0adfd6a0103 100644 --- a/src/Client/Connection.cpp +++ b/src/Client/Connection.cpp @@ -162,14 +162,12 @@ void Connection::sendHello() || has_control_character(password)) throw Exception("Parameters 'default_database', 'user' and 'password' must not contain ASCII control characters", ErrorCodes::BAD_ARGUMENTS); - auto client_revision = ClickHouseRevision::get(); - writeVarUInt(Protocol::Client::Hello, *out); writeStringBinary((DBMS_NAME " ") + client_name, *out); writeVarUInt(DBMS_VERSION_MAJOR, *out); writeVarUInt(DBMS_VERSION_MINOR, *out); // NOTE For backward compatibility of the protocol, client cannot send its version_patch. - writeVarUInt(client_revision, *out); + writeVarUInt(DBMS_TCP_PROTOCOL_VERSION, *out); writeStringBinary(default_database, *out); writeStringBinary(user, *out); writeStringBinary(password, *out); diff --git a/src/Common/ClickHouseRevision.cpp b/src/Common/ClickHouseRevision.cpp index 0b81026adca..2c52ebb064a 100644 --- a/src/Common/ClickHouseRevision.cpp +++ b/src/Common/ClickHouseRevision.cpp @@ -6,6 +6,6 @@ namespace ClickHouseRevision { - unsigned get() { return VERSION_REVISION; } + unsigned getVersionRevision() { return VERSION_REVISION; } unsigned getVersionInteger() { return VERSION_INTEGER; } } diff --git a/src/Common/ClickHouseRevision.h b/src/Common/ClickHouseRevision.h index 1d097a5bf89..86d1e3db334 100644 --- a/src/Common/ClickHouseRevision.h +++ b/src/Common/ClickHouseRevision.h @@ -2,6 +2,6 @@ namespace ClickHouseRevision { - unsigned get(); + unsigned getVersionRevision(); unsigned getVersionInteger(); } diff --git a/src/Common/StatusFile.cpp b/src/Common/StatusFile.cpp index 7c6bbf814a0..b21454c9ed8 100644 --- a/src/Common/StatusFile.cpp +++ b/src/Common/StatusFile.cpp @@ -37,7 +37,7 @@ StatusFile::FillFunction StatusFile::write_full_info = [](WriteBuffer & out) { out << "PID: " << getpid() << "\n" << "Started at: " << LocalDateTime(time(nullptr)) << "\n" - << "Revision: " << ClickHouseRevision::get() << "\n"; + << "Revision: " << ClickHouseRevision::getVersionRevision() << "\n"; }; diff --git a/src/Common/config_version.h.in b/src/Common/config_version.h.in index c3c0c6df87b..880824f8ad0 100644 --- a/src/Common/config_version.h.in +++ b/src/Common/config_version.h.in @@ -2,18 +2,7 @@ // .h autogenerated by cmake! -#cmakedefine01 USE_DBMS_TCP_PROTOCOL_VERSION - -#if USE_DBMS_TCP_PROTOCOL_VERSION - #include "Core/Defines.h" - #ifndef VERSION_REVISION - #define VERSION_REVISION DBMS_TCP_PROTOCOL_VERSION - #endif -#else - #cmakedefine VERSION_REVISION @VERSION_REVISION@ -#endif - - +#cmakedefine VERSION_REVISION @VERSION_REVISION@ #cmakedefine VERSION_NAME "@VERSION_NAME@" #define DBMS_NAME VERSION_NAME #cmakedefine VERSION_MAJOR @VERSION_MAJOR@ diff --git a/src/Core/Defines.h b/src/Core/Defines.h index d19513d1434..0b8e0b8ae27 100644 --- a/src/Core/Defines.h +++ b/src/Core/Defines.h @@ -68,10 +68,10 @@ #define DBMS_MIN_REVISION_WITH_SETTINGS_SERIALIZED_AS_STRINGS 54429 /// Minimum revision supporting OpenTelemetry -#define DBMS_MIN_REVISION_WITH_OPENTELEMETRY 54227 +#define DBMS_MIN_REVISION_WITH_OPENTELEMETRY 54441 /// Version of ClickHouse TCP protocol. Set to git tag with latest protocol change. -#define DBMS_TCP_PROTOCOL_VERSION 54227 +#define DBMS_TCP_PROTOCOL_VERSION 54441 /// The boundary on which the blocks for asynchronous file operations should be aligned. #define DEFAULT_AIO_FILE_BLOCK_SIZE 4096 diff --git a/src/DataStreams/TemporaryFileStream.h b/src/DataStreams/TemporaryFileStream.h index 6871800a540..b481cef1bb2 100644 --- a/src/DataStreams/TemporaryFileStream.h +++ b/src/DataStreams/TemporaryFileStream.h @@ -1,6 +1,5 @@ #pragma once -#include #include #include #include @@ -23,7 +22,7 @@ struct TemporaryFileStream TemporaryFileStream(const std::string & path) : file_in(path) , compressed_in(file_in) - , block_in(std::make_shared(compressed_in, ClickHouseRevision::get())) + , block_in(std::make_shared(compressed_in, DBMS_TCP_PROTOCOL_VERSION)) {} TemporaryFileStream(const std::string & path, const Block & header_) diff --git a/src/Interpreters/Aggregator.cpp b/src/Interpreters/Aggregator.cpp index 86a33dccb53..466370a22a2 100644 --- a/src/Interpreters/Aggregator.cpp +++ b/src/Interpreters/Aggregator.cpp @@ -844,7 +844,7 @@ void Aggregator::writeToTemporaryFile(AggregatedDataVariants & data_variants, co const std::string & path = file->path(); WriteBufferFromFile file_buf(path); CompressedWriteBuffer compressed_buf(file_buf); - NativeBlockOutputStream block_out(compressed_buf, ClickHouseRevision::get(), getHeader(false)); + NativeBlockOutputStream block_out(compressed_buf, DBMS_TCP_PROTOCOL_VERSION, getHeader(false)); LOG_DEBUG(log, "Writing part of aggregation data into temporary file {}.", path); ProfileEvents::increment(ProfileEvents::ExternalAggregationWritePart); diff --git a/src/Interpreters/ClientInfo.cpp b/src/Interpreters/ClientInfo.cpp index da9f14ec879..25cf9b8294e 100644 --- a/src/Interpreters/ClientInfo.cpp +++ b/src/Interpreters/ClientInfo.cpp @@ -5,7 +5,6 @@ #include #include #include -#include #include #if !defined(ARCADIA_BUILD) @@ -44,7 +43,7 @@ void ClientInfo::write(WriteBuffer & out, const UInt64 server_protocol_revision) writeBinary(client_name, out); writeVarUInt(client_version_major, out); writeVarUInt(client_version_minor, out); - writeVarUInt(client_revision, out); + writeVarUInt(client_tcp_protocol_version, out); } else if (interface == Interface::HTTP) { @@ -111,7 +110,7 @@ void ClientInfo::read(ReadBuffer & in, const UInt64 client_protocol_revision) readBinary(client_name, in); readVarUInt(client_version_major, in); readVarUInt(client_version_minor, in); - readVarUInt(client_revision, in); + readVarUInt(client_tcp_protocol_version, in); } else if (interface == Interface::HTTP) { @@ -130,7 +129,7 @@ void ClientInfo::read(ReadBuffer & in, const UInt64 client_protocol_revision) if (client_protocol_revision >= DBMS_MIN_REVISION_WITH_VERSION_PATCH) readVarUInt(client_version_patch, in); else - client_version_patch = client_revision; + client_version_patch = client_tcp_protocol_version; } // TODO what does it even mean to read this structure over HTTP? I thought @@ -244,7 +243,7 @@ void ClientInfo::fillOSUserHostNameAndVersionInfo() client_version_major = DBMS_VERSION_MAJOR; client_version_minor = DBMS_VERSION_MINOR; client_version_patch = DBMS_VERSION_PATCH; - client_revision = ClickHouseRevision::get(); + client_tcp_protocol_version = DBMS_TCP_PROTOCOL_VERSION; } diff --git a/src/Interpreters/ClientInfo.h b/src/Interpreters/ClientInfo.h index e41956eaee0..5b5c3b400eb 100644 --- a/src/Interpreters/ClientInfo.h +++ b/src/Interpreters/ClientInfo.h @@ -82,7 +82,7 @@ public: UInt64 client_version_major = 0; UInt64 client_version_minor = 0; UInt64 client_version_patch = 0; - unsigned client_revision = 0; + unsigned client_tcp_protocol_version = 0; /// For http HTTPMethod http_method = HTTPMethod::UNKNOWN; diff --git a/src/Interpreters/CrashLog.cpp b/src/Interpreters/CrashLog.cpp index 12fd57c33dc..9d84d5a18e9 100644 --- a/src/Interpreters/CrashLog.cpp +++ b/src/Interpreters/CrashLog.cpp @@ -49,7 +49,7 @@ void CrashLogElement::appendToBlock(MutableColumns & columns) const columns[i++]->insert(trace); columns[i++]->insert(trace_full); columns[i++]->insert(VERSION_FULL); - columns[i++]->insert(ClickHouseRevision::get()); + columns[i++]->insert(ClickHouseRevision::getVersionRevision()); String build_id_hex; #if defined(__ELF__) && !defined(__FreeBSD__) diff --git a/src/Interpreters/QueryLog.cpp b/src/Interpreters/QueryLog.cpp index 62dbc114633..75e0fae615a 100644 --- a/src/Interpreters/QueryLog.cpp +++ b/src/Interpreters/QueryLog.cpp @@ -118,7 +118,7 @@ void QueryLogElement::appendToBlock(MutableColumns & columns) const appendClientInfo(client_info, columns, i); - columns[i++]->insert(ClickHouseRevision::get()); + columns[i++]->insert(ClickHouseRevision::getVersionRevision()); { Array threads_array; @@ -172,7 +172,7 @@ void QueryLogElement::appendClientInfo(const ClientInfo & client_info, MutableCo columns[i++]->insert(client_info.os_user); columns[i++]->insert(client_info.client_hostname); columns[i++]->insert(client_info.client_name); - columns[i++]->insert(client_info.client_revision); + columns[i++]->insert(client_info.client_tcp_protocol_version); columns[i++]->insert(client_info.client_version_major); columns[i++]->insert(client_info.client_version_minor); columns[i++]->insert(client_info.client_version_patch); diff --git a/src/Interpreters/QueryThreadLog.cpp b/src/Interpreters/QueryThreadLog.cpp index 22ad60d96b4..e5a8cf7c5cf 100644 --- a/src/Interpreters/QueryThreadLog.cpp +++ b/src/Interpreters/QueryThreadLog.cpp @@ -93,7 +93,7 @@ void QueryThreadLogElement::appendToBlock(MutableColumns & columns) const QueryLogElement::appendClientInfo(client_info, columns, i); - columns[i++]->insert(ClickHouseRevision::get()); + columns[i++]->insert(ClickHouseRevision::getVersionRevision()); if (profile_counters) { diff --git a/src/Interpreters/TextLog.cpp b/src/Interpreters/TextLog.cpp index d166b24ef4f..243bf6d299a 100644 --- a/src/Interpreters/TextLog.cpp +++ b/src/Interpreters/TextLog.cpp @@ -62,7 +62,7 @@ void TextLogElement::appendToBlock(MutableColumns & columns) const columns[i++]->insert(logger_name); columns[i++]->insert(message); - columns[i++]->insert(ClickHouseRevision::get()); + columns[i++]->insert(ClickHouseRevision::getVersionRevision()); columns[i++]->insert(source_file); columns[i++]->insert(source_line); diff --git a/src/Interpreters/TraceLog.cpp b/src/Interpreters/TraceLog.cpp index c4fa7307b1a..f7e82032f49 100644 --- a/src/Interpreters/TraceLog.cpp +++ b/src/Interpreters/TraceLog.cpp @@ -43,7 +43,7 @@ void TraceLogElement::appendToBlock(MutableColumns & columns) const columns[i++]->insert(DateLUT::instance().toDayNum(event_time)); columns[i++]->insert(event_time); columns[i++]->insert(timestamp_ns); - columns[i++]->insert(ClickHouseRevision::get()); + columns[i++]->insert(ClickHouseRevision::getVersionRevision()); columns[i++]->insert(static_cast(trace_type)); columns[i++]->insert(thread_id); columns[i++]->insertData(query_id.data(), query_id.size()); diff --git a/src/Processors/Transforms/AggregatingTransform.cpp b/src/Processors/Transforms/AggregatingTransform.cpp index 42caf4b3446..0a97cc3d4cb 100644 --- a/src/Processors/Transforms/AggregatingTransform.cpp +++ b/src/Processors/Transforms/AggregatingTransform.cpp @@ -1,6 +1,5 @@ #include -#include #include #include #include @@ -56,7 +55,7 @@ namespace public: SourceFromNativeStream(const Block & header, const std::string & path) : ISource(header), file_in(path), compressed_in(file_in), - block_in(std::make_shared(compressed_in, ClickHouseRevision::get())) + block_in(std::make_shared(compressed_in, DBMS_TCP_PROTOCOL_VERSION)) { block_in->readPrefix(); } diff --git a/src/Server/TCPHandler.cpp b/src/Server/TCPHandler.cpp index 2c7ca3ac6a4..ca8774c23ca 100644 --- a/src/Server/TCPHandler.cpp +++ b/src/Server/TCPHandler.cpp @@ -1,7 +1,6 @@ #include #include #include -#include #include #include #include @@ -183,7 +182,7 @@ void TCPHandler::runImpl() /// Should we send internal logs to client? const auto client_logs_level = query_context->getSettingsRef().send_logs_level; - if (client_revision >= DBMS_MIN_REVISION_WITH_SERVER_LOGS + if (client_tcp_protocol_version >= DBMS_MIN_REVISION_WITH_SERVER_LOGS && client_logs_level != LogsLevel::none) { state.logs_queue = std::make_shared(); @@ -218,7 +217,7 @@ void TCPHandler::runImpl() state.need_receive_data_for_input = true; /// Send ColumnsDescription for input storage. - if (client_revision >= DBMS_MIN_REVISION_WITH_COLUMN_DEFAULTS_METADATA + if (client_tcp_protocol_version >= DBMS_MIN_REVISION_WITH_COLUMN_DEFAULTS_METADATA && query_context->getSettingsRef().input_format_defaults_for_omitted_fields) { sendTableColumns(metadata_snapshot->getColumns()); @@ -248,7 +247,7 @@ void TCPHandler::runImpl() customizeContext(*query_context); - bool may_have_embedded_data = client_revision >= DBMS_MIN_REVISION_WITH_CLIENT_SUPPORT_EMBEDDED_DATA; + bool may_have_embedded_data = client_tcp_protocol_version >= DBMS_MIN_REVISION_WITH_CLIENT_SUPPORT_EMBEDDED_DATA; /// Processing Query state.io = executeQuery(state.query, *query_context, false, state.stage, may_have_embedded_data); @@ -482,7 +481,7 @@ void TCPHandler::processInsertQuery(const Settings & connection_settings) state.io.out->writePrefix(); /// Send ColumnsDescription for insertion table - if (client_revision >= DBMS_MIN_REVISION_WITH_COLUMN_DEFAULTS_METADATA) + if (client_tcp_protocol_version >= DBMS_MIN_REVISION_WITH_COLUMN_DEFAULTS_METADATA) { const auto & table_id = query_context->getInsertionTable(); if (query_context->getSettingsRef().input_format_defaults_for_omitted_fields) @@ -638,7 +637,7 @@ void TCPHandler::processOrdinaryQueryWithProcessors() void TCPHandler::processTablesStatusRequest() { TablesStatusRequest request; - request.read(*in, client_revision); + request.read(*in, client_tcp_protocol_version); TablesStatusResponse response; for (const QualifiedTableName & table_name: request.tables) @@ -661,13 +660,13 @@ void TCPHandler::processTablesStatusRequest() } writeVarUInt(Protocol::Server::TablesStatusResponse, *out); - response.write(*out, client_revision); + response.write(*out, client_tcp_protocol_version); } void TCPHandler::receiveUnexpectedTablesStatusRequest() { TablesStatusRequest skip_request; - skip_request.read(*in, client_revision); + skip_request.read(*in, client_tcp_protocol_version); throw NetException("Unexpected packet TablesStatusRequest received from client", ErrorCodes::UNEXPECTED_PACKET_FROM_CLIENT); } @@ -742,7 +741,7 @@ void TCPHandler::receiveHello() readVarUInt(client_version_major, *in); readVarUInt(client_version_minor, *in); // NOTE For backward compatibility of the protocol, client cannot send its version_patch. - readVarUInt(client_revision, *in); + readVarUInt(client_tcp_protocol_version, *in); readStringBinary(default_database, *in); readStringBinary(user, *in); readStringBinary(password, *in); @@ -750,7 +749,7 @@ void TCPHandler::receiveHello() LOG_DEBUG(log, "Connected {} version {}.{}.{}, revision: {}{}{}.", client_name, client_version_major, client_version_minor, client_version_patch, - client_revision, + client_tcp_protocol_version, (!default_database.empty() ? ", database: " + default_database : ""), (!user.empty() ? ", user: " + user : "")); @@ -781,12 +780,12 @@ void TCPHandler::sendHello() writeStringBinary(DBMS_NAME, *out); writeVarUInt(DBMS_VERSION_MAJOR, *out); writeVarUInt(DBMS_VERSION_MINOR, *out); - writeVarUInt(ClickHouseRevision::get(), *out); - if (client_revision >= DBMS_MIN_REVISION_WITH_SERVER_TIMEZONE) + writeVarUInt(DBMS_TCP_PROTOCOL_VERSION, *out); + if (client_tcp_protocol_version >= DBMS_MIN_REVISION_WITH_SERVER_TIMEZONE) writeStringBinary(DateLUT::instance().getTimeZone(), *out); - if (client_revision >= DBMS_MIN_REVISION_WITH_SERVER_DISPLAY_NAME) + if (client_tcp_protocol_version >= DBMS_MIN_REVISION_WITH_SERVER_DISPLAY_NAME) writeStringBinary(server_display_name, *out); - if (client_revision >= DBMS_MIN_REVISION_WITH_VERSION_PATCH) + if (client_tcp_protocol_version >= DBMS_MIN_REVISION_WITH_VERSION_PATCH) writeVarUInt(DBMS_VERSION_PATCH, *out); out->next(); } @@ -847,8 +846,8 @@ void TCPHandler::receiveQuery() /// Client info ClientInfo & client_info = query_context->getClientInfo(); - if (client_revision >= DBMS_MIN_REVISION_WITH_CLIENT_INFO) - client_info.read(*in, client_revision); + if (client_tcp_protocol_version >= DBMS_MIN_REVISION_WITH_CLIENT_INFO) + client_info.read(*in, client_tcp_protocol_version); /// For better support of old clients, that does not send ClientInfo. if (client_info.query_kind == ClientInfo::QueryKind::NO_QUERY) @@ -858,7 +857,7 @@ void TCPHandler::receiveQuery() client_info.client_version_major = client_version_major; client_info.client_version_minor = client_version_minor; client_info.client_version_patch = client_version_patch; - client_info.client_revision = client_revision; + client_info.client_tcp_protocol_version = client_tcp_protocol_version; } /// Set fields, that are known apriori. @@ -879,7 +878,7 @@ void TCPHandler::receiveQuery() /// Per query settings are also passed via TCP. /// We need to check them before applying due to they can violate the settings constraints. auto settings_format = - (client_revision >= DBMS_MIN_REVISION_WITH_SETTINGS_SERIALIZED_AS_STRINGS) + (client_tcp_protocol_version >= DBMS_MIN_REVISION_WITH_SETTINGS_SERIALIZED_AS_STRINGS) ? SettingsWriteFormat::STRINGS_WITH_FLAGS : SettingsWriteFormat::BINARY; @@ -900,7 +899,7 @@ void TCPHandler::receiveQuery() // Use the received query id, or generate a random default. It is convenient // to also generate the default OpenTelemetry trace id at the same time, and - // and and set the trace parent. + // set the trace parent. // Why is this done here and not earlier: // 1) ClientInfo might contain upstream trace id, so we decide whether to use // the default ids after we have received the ClientInfo. @@ -933,11 +932,11 @@ void TCPHandler::receiveUnexpectedQuery() readStringBinary(skip_string, *in); ClientInfo skip_client_info; - if (client_revision >= DBMS_MIN_REVISION_WITH_CLIENT_INFO) - skip_client_info.read(*in, client_revision); + if (client_tcp_protocol_version >= DBMS_MIN_REVISION_WITH_CLIENT_INFO) + skip_client_info.read(*in, client_tcp_protocol_version); Settings skip_settings; - auto settings_format = (client_revision >= DBMS_MIN_REVISION_WITH_SETTINGS_SERIALIZED_AS_STRINGS) ? SettingsWriteFormat::STRINGS_WITH_FLAGS + auto settings_format = (client_tcp_protocol_version >= DBMS_MIN_REVISION_WITH_SETTINGS_SERIALIZED_AS_STRINGS) ? SettingsWriteFormat::STRINGS_WITH_FLAGS : SettingsWriteFormat::BINARY; skip_settings.read(*in, settings_format); @@ -1011,7 +1010,7 @@ void TCPHandler::receiveUnexpectedData() auto skip_block_in = std::make_shared( *maybe_compressed_in, last_block_in.header, - client_revision); + client_tcp_protocol_version); skip_block_in->read(); throw NetException("Unexpected packet Data received from client", ErrorCodes::UNEXPECTED_PACKET_FROM_CLIENT); @@ -1038,7 +1037,7 @@ void TCPHandler::initBlockInput() state.block_in = std::make_shared( *state.maybe_compressed_in, header, - client_revision); + client_tcp_protocol_version); } } @@ -1069,7 +1068,7 @@ void TCPHandler::initBlockOutput(const Block & block) state.block_out = std::make_shared( *state.maybe_compressed_out, - client_revision, + client_tcp_protocol_version, block.cloneEmpty(), !connection_context.getSettingsRef().low_cardinality_allow_in_native_format); } @@ -1082,7 +1081,7 @@ void TCPHandler::initLogsBlockOutput(const Block & block) /// Use uncompressed stream since log blocks usually contain only one row state.logs_block_out = std::make_shared( *out, - client_revision, + client_tcp_protocol_version, block.cloneEmpty(), !connection_context.getSettingsRef().low_cardinality_allow_in_native_format); } @@ -1186,7 +1185,7 @@ void TCPHandler::sendProgress() { writeVarUInt(Protocol::Server::Progress, *out); auto increment = state.progress.fetchAndResetPiecewiseAtomically(); - increment.write(*out, client_revision); + increment.write(*out, client_tcp_protocol_version); out->next(); } diff --git a/src/Server/TCPHandler.h b/src/Server/TCPHandler.h index 3fec89264be..3c7747481f0 100644 --- a/src/Server/TCPHandler.h +++ b/src/Server/TCPHandler.h @@ -124,7 +124,7 @@ private: UInt64 client_version_major = 0; UInt64 client_version_minor = 0; UInt64 client_version_patch = 0; - UInt64 client_revision = 0; + UInt64 client_tcp_protocol_version = 0; Context connection_context; std::optional query_context; diff --git a/src/Storages/Distributed/DirectoryMonitor.cpp b/src/Storages/Distributed/DirectoryMonitor.cpp index b67d3283ac9..61708dce325 100644 --- a/src/Storages/Distributed/DirectoryMonitor.cpp +++ b/src/Storages/Distributed/DirectoryMonitor.cpp @@ -3,7 +3,6 @@ #include #include #include -#include #include #include #include @@ -357,7 +356,7 @@ void StorageDistributedDirectoryMonitor::readHeader( UInt64 initiator_revision; readVarUInt(initiator_revision, header_buf); - if (ClickHouseRevision::get() < initiator_revision) + if (DBMS_TCP_PROTOCOL_VERSION < initiator_revision) { LOG_WARNING(log, "ClickHouse shard version is older than ClickHouse initiator version. It may lack support for new features."); } @@ -576,7 +575,7 @@ public: explicit DirectoryMonitorBlockInputStream(const String & file_name) : in(file_name) , decompressing_in(in) - , block_in(decompressing_in, ClickHouseRevision::get()) + , block_in(decompressing_in, DBMS_TCP_PROTOCOL_VERSION) , log{&Poco::Logger::get("DirectoryMonitorBlockInputStream")} { Settings insert_settings; @@ -681,7 +680,7 @@ void StorageDistributedDirectoryMonitor::processFilesWithBatching(const std::map readHeader(in, insert_settings, insert_query, client_info, log); CompressedReadBuffer decompressing_in(in); - NativeBlockInputStream block_in(decompressing_in, ClickHouseRevision::get()); + NativeBlockInputStream block_in(decompressing_in, DBMS_TCP_PROTOCOL_VERSION); block_in.readPrefix(); while (Block block = block_in.read()) diff --git a/src/Storages/Distributed/DistributedBlockOutputStream.cpp b/src/Storages/Distributed/DistributedBlockOutputStream.cpp index 172a398258f..f08cdf76cbf 100644 --- a/src/Storages/Distributed/DistributedBlockOutputStream.cpp +++ b/src/Storages/Distributed/DistributedBlockOutputStream.cpp @@ -21,7 +21,6 @@ #include #include #include -#include #include #include #include @@ -583,16 +582,16 @@ void DistributedBlockOutputStream::writeToShard(const Block & block, const std:: { WriteBufferFromFile out{first_file_tmp_path}; CompressedWriteBuffer compress{out}; - NativeBlockOutputStream stream{compress, ClickHouseRevision::get(), block.cloneEmpty()}; + NativeBlockOutputStream stream{compress, DBMS_TCP_PROTOCOL_VERSION, block.cloneEmpty()}; /// Prepare the header. /// We wrap the header into a string for compatibility with older versions: /// a shard will able to read the header partly and ignore other parts based on its version. WriteBufferFromOwnString header_buf; - writeVarUInt(ClickHouseRevision::get(), header_buf); + writeVarUInt(DBMS_TCP_PROTOCOL_VERSION, header_buf); writeStringBinary(query_string, header_buf); context.getSettingsRef().write(header_buf); - context.getClientInfo().write(header_buf, ClickHouseRevision::get()); + context.getClientInfo().write(header_buf, DBMS_TCP_PROTOCOL_VERSION); /// Add new fields here, for example: /// writeVarUInt(my_new_data, header_buf); diff --git a/src/Storages/System/StorageSystemProcesses.cpp b/src/Storages/System/StorageSystemProcesses.cpp index c65a6b78e41..d899a1708bf 100644 --- a/src/Storages/System/StorageSystemProcesses.cpp +++ b/src/Storages/System/StorageSystemProcesses.cpp @@ -91,7 +91,7 @@ void StorageSystemProcesses::fillData(MutableColumns & res_columns, const Contex res_columns[i++]->insert(process.client_info.os_user); res_columns[i++]->insert(process.client_info.client_hostname); res_columns[i++]->insert(process.client_info.client_name); - res_columns[i++]->insert(process.client_info.client_revision); + res_columns[i++]->insert(process.client_info.client_tcp_protocol_version); res_columns[i++]->insert(process.client_info.client_version_major); res_columns[i++]->insert(process.client_info.client_version_minor); res_columns[i++]->insert(process.client_info.client_version_patch); From 4c6da6b70cb4105ab3383c342e4bcb4523215ae0 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Tue, 29 Sep 2020 21:01:49 +0300 Subject: [PATCH 015/145] allow fast test to run locally --- docker/test/fasttest/run.sh | 165 +++++++++++++++++++++++++----------- 1 file changed, 114 insertions(+), 51 deletions(-) diff --git a/docker/test/fasttest/run.sh b/docker/test/fasttest/run.sh index a277ddf9d36..b93aa0ce5cd 100755 --- a/docker/test/fasttest/run.sh +++ b/docker/test/fasttest/run.sh @@ -16,28 +16,55 @@ stage=${stage:-} read -ra FASTTEST_CMAKE_FLAGS <<< "${FASTTEST_CMAKE_FLAGS:-}" -function kill_clickhouse +FASTTEST_WORKSPACE=$(readlink -f "${FASTTEST_WORKSPACE:-.}") +FASTTEST_SOURCE=$(readlink -f "${FASTTEST_SOURCE:-$FASTTEST_WORKSPACE/ch}") +FASTTEST_BUILD=$(readlink -f "${FASTTEST_BUILD:-${BUILD:-$FASTTEST_WORKSPACE/build}}") +FASTTEST_DATA=$(readlink -f "${FASTTEST_DATA:-$FASTTEST_WORKSPACE/db-fasttest}") +FASTTEST_OUTPUT=$(readlink -f "${FASTTEST_OUTPUT:-$FASTTEST_WORKSPACE}") + +server_pid=none + +function stop_server { + if ! kill -- "$server_pid" + then + echo "The server we started ($server_pid) is not present, won't do anything" + return 0 + fi + for _ in {1..60} do - if ! pkill -f clickhouse-server ; then break ; fi + if ! kill -- "$server_pid" ; then break ; fi sleep 1 done - if pgrep -f clickhouse-server + if kill -0 -- "$server_pid" then pstree -apgT jobs - echo "Failed to kill the ClickHouse server $(pgrep -f clickhouse-server)" + echo "Failed to kill the ClickHouse server pid '$server_pid'" return 1 fi } -function wait_for_server_start +function start_server { + set -m # Spawn server in its own process groups + clickhouse-server --config-file="$FASTTEST_DATA/config.xml" -- --path "$FASTTEST_DATA" --user_files_path "$FASTTEST_DATA/user_files" &>> "$FASTTEST_OUTPUT/server.log" & + server_pid=$! + set +m + + if [ "$server_pid" == "0" ] + then + echo "Failed to start ClickHouse server" + # Avoid zero PID because `kill` treats it as our process group PID. + server_pid="none" + return 1 + fi + for _ in {1..60} do - if clickhouse-client --query "select 1" || ! pgrep -f clickhouse-server + if clickhouse-client --query "select 1" || ! kill -0 -- "$server_pid" then break fi @@ -47,20 +74,26 @@ function wait_for_server_start if ! clickhouse-client --query "select 1" then echo "Failed to wait until ClickHouse server starts." + server_pid="none" return 1 fi - echo "ClickHouse server pid '$(pgrep -f clickhouse-server)' started and responded" + if ! kill -0 -- "$server_pid" + then + echo "Wrong clickhouse server started: PID '$server_pid' we started is not running, but '$(pgrep -f clickhouse-server)' is running" + server_pid="none" + return 1 + fi + + echo "ClickHouse server pid '$server_pid' started and responded" } function clone_root { -git clone https://github.com/ClickHouse/ClickHouse.git | ts '%Y-%m-%d %H:%M:%S' | tee /test_output/clone_log.txt -cd ClickHouse -CLICKHOUSE_DIR=$(pwd) -export CLICKHOUSE_DIR - +git clone https://github.com/ClickHouse/ClickHouse.git -- "$FASTTEST_SOURCE" | ts '%Y-%m-%d %H:%M:%S' | tee "$FASTTEST_OUTPUT/clone_log.txt" +( +cd "$FASTTEST_SOURCE" if [ "$PULL_REQUEST_NUMBER" != "0" ]; then if git fetch origin "+refs/pull/$PULL_REQUEST_NUMBER/merge"; then git checkout FETCH_HEAD @@ -71,22 +104,37 @@ if [ "$PULL_REQUEST_NUMBER" != "0" ]; then echo 'Checked out to commit' fi else - if [ "$COMMIT_SHA" != "" ]; then + if [ -v COMMIT_SHA ]; then git checkout "$COMMIT_SHA" fi fi +) } -function run +function clone_submodules { +( +cd "$FASTTEST_SOURCE" + SUBMODULES_TO_UPDATE=(contrib/boost contrib/zlib-ng contrib/libxml2 contrib/poco contrib/libunwind contrib/ryu contrib/fmtlib contrib/base64 contrib/cctz contrib/libcpuid contrib/double-conversion contrib/libcxx contrib/libcxxabi contrib/libc-headers contrib/lz4 contrib/zstd contrib/fastops contrib/rapidjson contrib/re2 contrib/sparsehash-c11) -git submodule update --init --recursive "${SUBMODULES_TO_UPDATE[@]}" | ts '%Y-%m-%d %H:%M:%S' | tee /test_output/submodule_log.txt +git submodule sync +git submodule update --init --recursive "${SUBMODULES_TO_UPDATE[@]}" +git submodule foreach git reset --hard +git submodule foreach git checkout @ -f +git submodule foreach git clean -xfd +) +} + +function build +{ CMAKE_LIBS_CONFIG=(-DENABLE_LIBRARIES=0 -DENABLE_TESTS=0 -DENABLE_UTILS=0 -DENABLE_EMBEDDED_COMPILER=0 -DENABLE_THINLTO=0 -DUSE_UNWIND=1) -export CCACHE_DIR=/ccache -export CCACHE_BASEDIR=/ClickHouse +# TODO remove this? we don't use ccache anyway. An option would be to download it +# from S3 simultaneously with cloning. +export CCACHE_DIR="$FASTTEST_WORKSPACE/ccache" +export CCACHE_BASEDIR="$FASTTEST_SOURCE" export CCACHE_NOHASHDIR=true export CCACHE_COMPILERCHECK=content export CCACHE_MAXSIZE=15G @@ -94,34 +142,40 @@ export CCACHE_MAXSIZE=15G ccache --show-stats ||: ccache --zero-stats ||: -mkdir build -cd build -cmake .. -DCMAKE_INSTALL_PREFIX=/usr -DCMAKE_CXX_COMPILER=clang++-10 -DCMAKE_C_COMPILER=clang-10 "${CMAKE_LIBS_CONFIG[@]}" "${FASTTEST_CMAKE_FLAGS[@]}" | ts '%Y-%m-%d %H:%M:%S' | tee /test_output/cmake_log.txt -time ninja clickhouse-bundle | ts '%Y-%m-%d %H:%M:%S' | tee /test_output/build_log.txt -ninja install | ts '%Y-%m-%d %H:%M:%S' | tee /test_output/install_log.txt - +mkdir "$FASTTEST_BUILD" ||: +( +cd "$FASTTEST_BUILD" +cmake "$FASTTEST_SOURCE" -DCMAKE_CXX_COMPILER=clang++-10 -DCMAKE_C_COMPILER=clang-10 "${CMAKE_LIBS_CONFIG[@]}" "${FASTTEST_CMAKE_FLAGS[@]}" | ts '%Y-%m-%d %H:%M:%S' | tee "$FASTTEST_OUTPUT/cmake_log.txt" +time ninja clickhouse-bundle | ts '%Y-%m-%d %H:%M:%S' | tee "$FASTTEST_OUTPUT/build_log.txt" +) ccache --show-stats ||: +} -mkdir -p /etc/clickhouse-server -mkdir -p /etc/clickhouse-client -mkdir -p /etc/clickhouse-server/config.d -mkdir -p /etc/clickhouse-server/users.d -ln -s /test_output /var/log/clickhouse-server -cp "$CLICKHOUSE_DIR/programs/server/config.xml" /etc/clickhouse-server/ -cp "$CLICKHOUSE_DIR/programs/server/users.xml" /etc/clickhouse-server/ +function configure +{ +export PATH="$FASTTEST_BUILD/programs:$FASTTEST_SOURCE/tests:$PATH" -# install tests config -$CLICKHOUSE_DIR/tests/config/install.sh +clickhouse-client --version +clickhouse-test --help + +mkdir -p "$FASTTEST_DATA"{,/client-config} +cp -a "$FASTTEST_SOURCE/programs/server/"{config,users}.xml "$FASTTEST_DATA" +cp -a "$FASTTEST_SOURCE/programs/server/"{config,users}.xml "$FASTTEST_DATA" +"$FASTTEST_SOURCE/tests/config/install.sh" "$FASTTEST_DATA" "$FASTTEST_DATA/client-config" # doesn't support SSL -rm -f /etc/clickhouse-server/config.d/secure_ports.xml +rm -f "$FASTTEST_DATA/config.d/secure_ports.xml" +} + +function run_tests +{ +clickhouse-server --version +clickhouse-test --help # Kill the server in case we are running locally and not in docker -kill_clickhouse +stop_server ||: -clickhouse-server --config /etc/clickhouse-server/config.xml --daemon - -wait_for_server_start +start_server TESTS_TO_SKIP=( parquet @@ -191,11 +245,10 @@ TESTS_TO_SKIP=( 01460_DistributedFilesToInsert ) -time clickhouse-test -j 8 --no-long --testname --shard --zookeeper --skip "${TESTS_TO_SKIP[@]}" 2>&1 | ts '%Y-%m-%d %H:%M:%S' | tee /test_output/test_log.txt - +time clickhouse-test -j 8 --no-long --testname --shard --zookeeper --skip "${TESTS_TO_SKIP[@]}" 2>&1 | ts '%Y-%m-%d %H:%M:%S' | tee "$FASTTEST_OUTPUT/test_log.txt" # substr is to remove semicolon after test name -readarray -t FAILED_TESTS < <(awk '/FAIL|TIMEOUT|ERROR/ { print substr($3, 1, length($3)-1) }' /test_output/test_log.txt | tee /test_output/failed-parallel-tests.txt) +readarray -t FAILED_TESTS < <(awk '/FAIL|TIMEOUT|ERROR/ { print substr($3, 1, length($3)-1) }' "$FASTTEST_OUTPUT/test_log.txt" | tee "$FASTTEST_OUTPUT/failed-parallel-tests.txt") # We will rerun sequentially any tests that have failed during parallel run. # They might have failed because there was some interference from other tests @@ -206,19 +259,16 @@ readarray -t FAILED_TESTS < <(awk '/FAIL|TIMEOUT|ERROR/ { print substr($3, 1, le # explicit instead of guessing. if [[ -n "${FAILED_TESTS[*]}" ]] then - kill_clickhouse + stop_server ||: # Clean the data so that there is no interference from the previous test run. - rm -rf /var/lib/clickhouse ||: - mkdir /var/lib/clickhouse + rm -rf "$FASTTEST_DATA"/{meta,}data ||: - clickhouse-server --config /etc/clickhouse-server/config.xml --daemon - - wait_for_server_start + start_server echo "Going to run again: ${FAILED_TESTS[*]}" - clickhouse-test --no-long --testname --shard --zookeeper "${FAILED_TESTS[@]}" 2>&1 | ts '%Y-%m-%d %H:%M:%S' | tee -a /test_output/test_log.txt + clickhouse-test --no-long --testname --shard --zookeeper "${FAILED_TESTS[@]}" 2>&1 | ts '%Y-%m-%d %H:%M:%S' | tee -a "$FASTTEST_OUTPUT/test_log.txt" else echo "No failed tests" fi @@ -228,20 +278,33 @@ case "$stage" in "") ls -la ;& - "clone_root") clone_root # Pass control to the script from cloned sources, unless asked otherwise. if ! [ -v FASTTEST_LOCAL_SCRIPT ] then - stage=run "$CLICKHOUSE_DIR/docker/test/fasttest/run.sh" + # 'run' is deprecated, used for compatibility with old scripts. + # Replace with 'clone_submodules' after Nov 1, 2020. + stage=run "$FASTTEST_SOURCE/docker/test/fasttest/run.sh" exit $? fi ;& - "run") - run + # A deprecated stage that is called by old script and equivalent to everything + # after cloning root, starting with cloning submodules. + ;& +"clone_submodules") + clone_submodules | ts '%Y-%m-%d %H:%M:%S' | tee "$FASTTEST_OUTPUT/submodule_log.txt" + ;& +"build") + build + ;& +"configure") + configure + ;& +"run_tests") + run_tests ;& esac From 00f9888c5d3f631f0ac2b9f9ba06098d56bc33a6 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Tue, 29 Sep 2020 21:55:06 +0300 Subject: [PATCH 016/145] fasttest fixup --- docker/test/fasttest/run.sh | 27 ++++++++++++-------- tests/config/install.sh | 50 ++++++++++++++++++------------------- 2 files changed, 41 insertions(+), 36 deletions(-) diff --git a/docker/test/fasttest/run.sh b/docker/test/fasttest/run.sh index b93aa0ce5cd..6a3f16d52f6 100755 --- a/docker/test/fasttest/run.sh +++ b/docker/test/fasttest/run.sh @@ -26,15 +26,9 @@ server_pid=none function stop_server { - if ! kill -- "$server_pid" - then - echo "The server we started ($server_pid) is not present, won't do anything" - return 0 - fi - for _ in {1..60} do - if ! kill -- "$server_pid" ; then break ; fi + if ! pkill -f "clickhouse-server" && ! kill -- "$server_pid" ; then break ; fi sleep 1 done @@ -45,6 +39,8 @@ function stop_server echo "Failed to kill the ClickHouse server pid '$server_pid'" return 1 fi + + server_pid=none } function start_server @@ -126,10 +122,9 @@ git submodule foreach git clean -xfd ) } - -function build +function run_cmake { -CMAKE_LIBS_CONFIG=(-DENABLE_LIBRARIES=0 -DENABLE_TESTS=0 -DENABLE_UTILS=0 -DENABLE_EMBEDDED_COMPILER=0 -DENABLE_THINLTO=0 -DUSE_UNWIND=1) +CMAKE_LIBS_CONFIG=("-DENABLE_LIBRARIES=0" "-DENABLE_TESTS=0" "-DENABLE_UTILS=0" "-DENABLE_EMBEDDED_COMPILER=0" "-DENABLE_THINLTO=0" "-DUSE_UNWIND=1") # TODO remove this? we don't use ccache anyway. An option would be to download it # from S3 simultaneously with cloning. @@ -143,13 +138,20 @@ ccache --show-stats ||: ccache --zero-stats ||: mkdir "$FASTTEST_BUILD" ||: + ( cd "$FASTTEST_BUILD" cmake "$FASTTEST_SOURCE" -DCMAKE_CXX_COMPILER=clang++-10 -DCMAKE_C_COMPILER=clang-10 "${CMAKE_LIBS_CONFIG[@]}" "${FASTTEST_CMAKE_FLAGS[@]}" | ts '%Y-%m-%d %H:%M:%S' | tee "$FASTTEST_OUTPUT/cmake_log.txt" -time ninja clickhouse-bundle | ts '%Y-%m-%d %H:%M:%S' | tee "$FASTTEST_OUTPUT/build_log.txt" ) +} +function build +{ +( +cd "$FASTTEST_BUILD" +time ninja clickhouse-bundle | ts '%Y-%m-%d %H:%M:%S' | tee "$FASTTEST_OUTPUT/build_log.txt" ccache --show-stats ||: +) } function configure @@ -297,6 +299,9 @@ case "$stage" in "clone_submodules") clone_submodules | ts '%Y-%m-%d %H:%M:%S' | tee "$FASTTEST_OUTPUT/submodule_log.txt" ;& +"run_cmake") + run_cmake + ;& "build") build ;& diff --git a/tests/config/install.sh b/tests/config/install.sh index 0f33854ef95..ef9604904e7 100755 --- a/tests/config/install.sh +++ b/tests/config/install.sh @@ -15,40 +15,40 @@ mkdir -p $DEST_SERVER_PATH/config.d/ mkdir -p $DEST_SERVER_PATH/users.d/ mkdir -p $DEST_CLIENT_PATH -ln -s $SRC_PATH/config.d/zookeeper.xml $DEST_SERVER_PATH/config.d/ -ln -s $SRC_PATH/config.d/listen.xml $DEST_SERVER_PATH/config.d/ -ln -s $SRC_PATH/config.d/part_log.xml $DEST_SERVER_PATH/config.d/ -ln -s $SRC_PATH/config.d/text_log.xml $DEST_SERVER_PATH/config.d/ -ln -s $SRC_PATH/config.d/metric_log.xml $DEST_SERVER_PATH/config.d/ -ln -s $SRC_PATH/config.d/custom_settings_prefixes.xml $DEST_SERVER_PATH/config.d/ -ln -s $SRC_PATH/config.d/macros.xml $DEST_SERVER_PATH/config.d/ -ln -s $SRC_PATH/config.d/disks.xml $DEST_SERVER_PATH/config.d/ -ln -s $SRC_PATH/config.d/secure_ports.xml $DEST_SERVER_PATH/config.d/ -ln -s $SRC_PATH/config.d/clusters.xml $DEST_SERVER_PATH/config.d/ -ln -s $SRC_PATH/config.d/graphite.xml $DEST_SERVER_PATH/config.d/ -ln -s $SRC_PATH/config.d/database_atomic.xml $DEST_SERVER_PATH/config.d/ -ln -s $SRC_PATH/users.d/log_queries.xml $DEST_SERVER_PATH/users.d/ -ln -s $SRC_PATH/users.d/readonly.xml $DEST_SERVER_PATH/users.d/ -ln -s $SRC_PATH/users.d/access_management.xml $DEST_SERVER_PATH/users.d/ +ln -sf $SRC_PATH/config.d/zookeeper.xml $DEST_SERVER_PATH/config.d/ +ln -sf $SRC_PATH/config.d/listen.xml $DEST_SERVER_PATH/config.d/ +ln -sf $SRC_PATH/config.d/part_log.xml $DEST_SERVER_PATH/config.d/ +ln -sf $SRC_PATH/config.d/text_log.xml $DEST_SERVER_PATH/config.d/ +ln -sf $SRC_PATH/config.d/metric_log.xml $DEST_SERVER_PATH/config.d/ +ln -sf $SRC_PATH/config.d/custom_settings_prefixes.xml $DEST_SERVER_PATH/config.d/ +ln -sf $SRC_PATH/config.d/macros.xml $DEST_SERVER_PATH/config.d/ +ln -sf $SRC_PATH/config.d/disks.xml $DEST_SERVER_PATH/config.d/ +ln -sf $SRC_PATH/config.d/secure_ports.xml $DEST_SERVER_PATH/config.d/ +ln -sf $SRC_PATH/config.d/clusters.xml $DEST_SERVER_PATH/config.d/ +ln -sf $SRC_PATH/config.d/graphite.xml $DEST_SERVER_PATH/config.d/ +ln -sf $SRC_PATH/config.d/database_atomic.xml $DEST_SERVER_PATH/config.d/ +ln -sf $SRC_PATH/users.d/log_queries.xml $DEST_SERVER_PATH/users.d/ +ln -sf $SRC_PATH/users.d/readonly.xml $DEST_SERVER_PATH/users.d/ +ln -sf $SRC_PATH/users.d/access_management.xml $DEST_SERVER_PATH/users.d/ -ln -s $SRC_PATH/ints_dictionary.xml $DEST_SERVER_PATH/ -ln -s $SRC_PATH/strings_dictionary.xml $DEST_SERVER_PATH/ -ln -s $SRC_PATH/decimals_dictionary.xml $DEST_SERVER_PATH/ -ln -s $SRC_PATH/executable_dictionary.xml $DEST_SERVER_PATH/ +ln -sf $SRC_PATH/ints_dictionary.xml $DEST_SERVER_PATH/ +ln -sf $SRC_PATH/strings_dictionary.xml $DEST_SERVER_PATH/ +ln -sf $SRC_PATH/decimals_dictionary.xml $DEST_SERVER_PATH/ +ln -sf $SRC_PATH/executable_dictionary.xml $DEST_SERVER_PATH/ -ln -s $SRC_PATH/server.key $DEST_SERVER_PATH/ -ln -s $SRC_PATH/server.crt $DEST_SERVER_PATH/ -ln -s $SRC_PATH/dhparam.pem $DEST_SERVER_PATH/ +ln -sf $SRC_PATH/server.key $DEST_SERVER_PATH/ +ln -sf $SRC_PATH/server.crt $DEST_SERVER_PATH/ +ln -sf $SRC_PATH/dhparam.pem $DEST_SERVER_PATH/ # Retain any pre-existing config and allow ClickHouse to load it if required -ln -s --backup=simple --suffix=_original.xml \ +ln -sf --backup=simple --suffix=_original.xml \ $SRC_PATH/config.d/query_masking_rules.xml $DEST_SERVER_PATH/config.d/ if [[ -n "$USE_POLYMORPHIC_PARTS" ]] && [[ "$USE_POLYMORPHIC_PARTS" -eq 1 ]]; then - ln -s $SRC_PATH/config.d/polymorphic_parts.xml $DEST_SERVER_PATH/config.d/ + ln -sf $SRC_PATH/config.d/polymorphic_parts.xml $DEST_SERVER_PATH/config.d/ fi if [[ -n "$USE_DATABASE_ORDINARY" ]] && [[ "$USE_DATABASE_ORDINARY" -eq 1 ]]; then - ln -s $SRC_PATH/users.d/database_ordinary.xml $DEST_SERVER_PATH/users.d/ + ln -sf $SRC_PATH/users.d/database_ordinary.xml $DEST_SERVER_PATH/users.d/ fi ln -sf $SRC_PATH/client_config.xml $DEST_CLIENT_PATH/config.xml From 85ec02dcedcadea5cacd18913722a421100ccade Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Tue, 29 Sep 2020 23:09:52 +0300 Subject: [PATCH 017/145] fasttest fixup 2 --- docker/test/fasttest/run.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docker/test/fasttest/run.sh b/docker/test/fasttest/run.sh index 6a3f16d52f6..6898000185d 100755 --- a/docker/test/fasttest/run.sh +++ b/docker/test/fasttest/run.sh @@ -306,7 +306,9 @@ case "$stage" in build ;& "configure") - configure + # The `install_log.txt` is also needed for compatibility with old CI task -- + # if there is no log, it will decide that build failed. + configure | ts '%Y-%m-%d %H:%M:%S' | tee "$FASTTEST_OUTPUT/install_log.txt" ;& "run_tests") run_tests From 13c6fd48296594f8dc705110fd142349a491a624 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Wed, 30 Sep 2020 00:05:05 +0300 Subject: [PATCH 018/145] fasttest fixup 3 --- docker/test/fasttest/run.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docker/test/fasttest/run.sh b/docker/test/fasttest/run.sh index 6898000185d..a9cbe87ecf9 100755 --- a/docker/test/fasttest/run.sh +++ b/docker/test/fasttest/run.sh @@ -156,7 +156,8 @@ ccache --show-stats ||: function configure { -export PATH="$FASTTEST_BUILD/programs:$FASTTEST_SOURCE/tests:$PATH" +PATH="$FASTTEST_BUILD/programs:$FASTTEST_SOURCE/tests:$PATH" +export PATH clickhouse-client --version clickhouse-test --help @@ -171,6 +172,7 @@ rm -f "$FASTTEST_DATA/config.d/secure_ports.xml" function run_tests { +echo "$PATH" clickhouse-server --version clickhouse-test --help From edf5797d117467e879783a50a05b840740fd19ee Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Wed, 30 Sep 2020 17:21:06 +0300 Subject: [PATCH 019/145] fasttest fixup 3 --- docker/test/fasttest/run.sh | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/docker/test/fasttest/run.sh b/docker/test/fasttest/run.sh index a9cbe87ecf9..4a47fcfe4dc 100755 --- a/docker/test/fasttest/run.sh +++ b/docker/test/fasttest/run.sh @@ -156,9 +156,6 @@ ccache --show-stats ||: function configure { -PATH="$FASTTEST_BUILD/programs:$FASTTEST_SOURCE/tests:$PATH" -export PATH - clickhouse-client --version clickhouse-test --help @@ -172,7 +169,6 @@ rm -f "$FASTTEST_DATA/config.d/secure_ports.xml" function run_tests { -echo "$PATH" clickhouse-server --version clickhouse-test --help @@ -306,6 +302,8 @@ case "$stage" in ;& "build") build + PATH="$FASTTEST_BUILD/programs:$FASTTEST_SOURCE/tests:$PATH" + export PATH ;& "configure") # The `install_log.txt` is also needed for compatibility with old CI task -- From 222b6555175019c487d9f71196ea462231fa1530 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Wed, 30 Sep 2020 18:12:53 +0300 Subject: [PATCH 020/145] fasttest compat with old script --- docker/test/fasttest/run.sh | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/docker/test/fasttest/run.sh b/docker/test/fasttest/run.sh index 4a47fcfe4dc..c4a9a0ea5f3 100755 --- a/docker/test/fasttest/run.sh +++ b/docker/test/fasttest/run.sh @@ -284,8 +284,14 @@ case "$stage" in # Pass control to the script from cloned sources, unless asked otherwise. if ! [ -v FASTTEST_LOCAL_SCRIPT ] then - # 'run' is deprecated, used for compatibility with old scripts. + # 'run' stage is deprecated, used for compatibility with old scripts. # Replace with 'clone_submodules' after Nov 1, 2020. + # cd and CLICKHOUSE_DIR are also a setup for old scripts, remove as well. + # In modern script we undo it by changing back into workspace dir right + # away, see below. Remove that as well. + cd "$FASTTEST_SOURCE" + CLICKHOUSE_DIR=$(pwd) + export CLICKHOUSE_DIR stage=run "$FASTTEST_SOURCE/docker/test/fasttest/run.sh" exit $? fi @@ -295,6 +301,10 @@ case "$stage" in # after cloning root, starting with cloning submodules. ;& "clone_submodules") + # Recover after being called from the old script that changes into source directory. + # See the compatibility hacks in `clone_root` stage above. Remove at the same time, + # after Nov 1, 2020. + cd "$FASTTEST_WORKSPACE" clone_submodules | ts '%Y-%m-%d %H:%M:%S' | tee "$FASTTEST_OUTPUT/submodule_log.txt" ;& "run_cmake") From 626c2a3e28e2b179aa78ad363866bc1a53dd90d6 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Wed, 30 Sep 2020 19:16:33 +0300 Subject: [PATCH 021/145] microsecond precision for start/finish time --- programs/server/config.xml | 2 +- src/Interpreters/ClientInfo.cpp | 6 +++-- src/Interpreters/OpenTelemetryLog.cpp | 11 ++++---- src/Interpreters/OpenTelemetryLog.h | 8 ++++-- src/Interpreters/executeQuery.cpp | 37 ++++++++++++--------------- 5 files changed, 34 insertions(+), 30 deletions(-) diff --git a/programs/server/config.xml b/programs/server/config.xml index b3269f2e842..9a1b626b26a 100644 --- a/programs/server/config.xml +++ b/programs/server/config.xml @@ -611,7 +611,7 @@ engine MergeTree partition by toYYYYMM(finish_date) - order by (finish_date, finish_time, trace_id) + order by (finish_date, finish_time_us, trace_id) system opentelemetry_log
diff --git a/src/Interpreters/ClientInfo.cpp b/src/Interpreters/ClientInfo.cpp index 25cf9b8294e..5570a1bc88d 100644 --- a/src/Interpreters/ClientInfo.cpp +++ b/src/Interpreters/ClientInfo.cpp @@ -62,11 +62,12 @@ void ClientInfo::write(WriteBuffer & out, const UInt64 server_protocol_revision) if (server_protocol_revision >= DBMS_MIN_REVISION_WITH_OPENTELEMETRY) { - // No point writing these numbers with variable length, because they - // are random and will probably require the full length anyway. if (opentelemetry_trace_id) { + // Have OpenTelemetry header. writeBinary(uint8_t(1), out); + // No point writing these numbers with variable length, because they + // are random and will probably require the full length anyway. writeBinary(opentelemetry_trace_id, out); writeBinary(opentelemetry_span_id, out); writeBinary(opentelemetry_parent_span_id, out); @@ -75,6 +76,7 @@ void ClientInfo::write(WriteBuffer & out, const UInt64 server_protocol_revision) } else { + // Don't have OpenTelemetry header. writeBinary(uint8_t(0), out); } } diff --git a/src/Interpreters/OpenTelemetryLog.cpp b/src/Interpreters/OpenTelemetryLog.cpp index 161af01107f..a247e557fcc 100644 --- a/src/Interpreters/OpenTelemetryLog.cpp +++ b/src/Interpreters/OpenTelemetryLog.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -17,8 +18,8 @@ Block OpenTelemetrySpanLogElement::createBlock() {std::make_shared(), "span_id"}, {std::make_shared(), "parent_span_id"}, {std::make_shared(), "operation_name"}, - {std::make_shared(), "start_time"}, - {std::make_shared(), "finish_time"}, + {std::make_shared(6), "start_time_us"}, + {std::make_shared(6), "finish_time_us"}, {std::make_shared(), "finish_date"}, {std::make_shared(std::make_shared()), "attribute.names"}, @@ -35,9 +36,9 @@ void OpenTelemetrySpanLogElement::appendToBlock(MutableColumns & columns) const columns[i++]->insert(span_id); columns[i++]->insert(parent_span_id); columns[i++]->insert(operation_name); - columns[i++]->insert(start_time); - columns[i++]->insert(finish_time); - columns[i++]->insert(DateLUT::instance().toDayNum(finish_time)); + columns[i++]->insert(start_time_us); + columns[i++]->insert(finish_time_us); + columns[i++]->insert(DateLUT::instance().toDayNum(finish_time_us / 1000000)); columns[i++]->insert(attribute_names); columns[i++]->insert(attribute_values); } diff --git a/src/Interpreters/OpenTelemetryLog.h b/src/Interpreters/OpenTelemetryLog.h index e2906a99a02..2d8536ff47e 100644 --- a/src/Interpreters/OpenTelemetryLog.h +++ b/src/Interpreters/OpenTelemetryLog.h @@ -15,6 +15,10 @@ struct OpenTelemetrySpanContext }; */ +// using TimeMicroseconds = std::chrono::time_point< +// std::chrono::local_t, +// std::chrono::duration>; + // TODO figure out precisely which part of this is run time, and which part we // must log. struct OpenTelemetrySpan @@ -23,8 +27,8 @@ struct OpenTelemetrySpan UInt64 span_id; UInt64 parent_span_id; std::string operation_name; - time_t start_time; - time_t finish_time; + UInt64 start_time_us; + UInt64 finish_time_us; UInt64 duration_ns; Array attribute_names; Array attribute_values; diff --git a/src/Interpreters/executeQuery.cpp b/src/Interpreters/executeQuery.cpp index 30f3dcd87b1..d89725b30f4 100644 --- a/src/Interpreters/executeQuery.cpp +++ b/src/Interpreters/executeQuery.cpp @@ -205,7 +205,7 @@ inline UInt64 time_in_seconds(std::chrono::time_point return std::chrono::duration_cast(timepoint.time_since_epoch()).count(); } -static void onExceptionBeforeStart(const String & query_for_logging, Context & context, time_t current_time, UInt64 current_time_microseconds, ASTPtr ast) +static void onExceptionBeforeStart(const String & query_for_logging, Context & context, UInt64 current_time_us, ASTPtr ast) { /// Exception before the query execution. if (auto quota = context.getQuota()) @@ -221,9 +221,9 @@ static void onExceptionBeforeStart(const String & query_for_logging, Context & c // all callers to onExceptionBeforeStart upstream construct the timespec for event_time and // event_time_microseconds from the same timespec. So it can be assumed that both of these // times are equal upto the precision of a second. - elem.event_time = current_time; - elem.query_start_time = current_time; - elem.query_start_time_microseconds = current_time_microseconds; + elem.event_time = current_time_us / 1000000; + elem.query_start_time = current_time_us / 1000000; + elem.query_start_time_microseconds = current_time_us; elem.current_database = context.getCurrentDatabase(); elem.query = query_for_logging; @@ -252,8 +252,8 @@ static void onExceptionBeforeStart(const String & query_for_logging, Context & c span.span_id = context.getClientInfo().opentelemetry_span_id; span.parent_span_id = context.getClientInfo().opentelemetry_parent_span_id; span.operation_name = "query"; - span.start_time = current_time; - span.finish_time = current_time; + span.start_time_us = current_time_us; + span.finish_time_us = current_time_us; span.duration_ns = 0; // keep values synchonized to type enum in QueryLogElement::createBlock @@ -309,12 +309,7 @@ static std::tuple executeQueryImpl( bool has_query_tail, ReadBuffer * istr) { - // current_time and current_time_microseconds are both constructed from the same time point - // to ensure that both the times are equal upto the precision of a second. - const auto now = std::chrono::system_clock::now(); - - auto current_time = time_in_seconds(now); - auto current_time_microseconds = time_in_microseconds(now); + const auto current_time = std::chrono::system_clock::now(); /// If we already executing query and it requires to execute internal query, than /// don't replace thread context with given (it can be temporary). Otherwise, attach context to thread. @@ -364,7 +359,7 @@ static std::tuple executeQueryImpl( logQuery(query_for_logging, context, internal); if (!internal) - onExceptionBeforeStart(query_for_logging, context, current_time, current_time_microseconds, ast); + onExceptionBeforeStart(query_for_logging, context, time_in_microseconds(current_time), ast); throw; } @@ -528,9 +523,9 @@ static std::tuple executeQueryImpl( elem.type = QueryLogElementType::QUERY_START; - elem.event_time = current_time; - elem.query_start_time = current_time; - elem.query_start_time_microseconds = current_time_microseconds; + elem.event_time = time_in_seconds(current_time); + elem.query_start_time = time_in_seconds(current_time); + elem.query_start_time_microseconds = time_in_microseconds(current_time); elem.current_database = context.getCurrentDatabase(); elem.query = query_for_logging; @@ -599,7 +594,9 @@ static std::tuple executeQueryImpl( elem.type = QueryLogElementType::QUERY_FINISH; - elem.event_time = time(nullptr); + const auto current_time = std::chrono::system_clock::now(); + + elem.event_time = time_in_seconds(current_time); status_info_to_query_log(elem, info, ast); @@ -660,8 +657,8 @@ static std::tuple executeQueryImpl( span.span_id = context.getClientInfo().opentelemetry_span_id; span.parent_span_id = context.getClientInfo().opentelemetry_parent_span_id; span.operation_name = "query"; - span.start_time = elem.query_start_time; - span.finish_time = elem.event_time; + span.start_time_us = elem.query_start_time_microseconds; + span.finish_time_us = time_in_microseconds(current_time); span.duration_ns = elapsed_seconds * 1000000000; // keep values synchonized to type enum in QueryLogElement::createBlock @@ -751,7 +748,7 @@ static std::tuple executeQueryImpl( if (query_for_logging.empty()) query_for_logging = prepareQueryForLogging(query, context); - onExceptionBeforeStart(query_for_logging, context, current_time, current_time_microseconds, ast); + onExceptionBeforeStart(query_for_logging, context, time_in_microseconds(current_time), ast); } throw; From a2cd346303d89043855bd6bf0aa385386d72e2fd Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Wed, 30 Sep 2020 20:35:46 +0300 Subject: [PATCH 022/145] fasttest fixup 5 --- docker/test/fasttest/run.sh | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/docker/test/fasttest/run.sh b/docker/test/fasttest/run.sh index c4a9a0ea5f3..5e586402bd7 100755 --- a/docker/test/fasttest/run.sh +++ b/docker/test/fasttest/run.sh @@ -15,13 +15,20 @@ stage=${stage:-} # empty parameter. read -ra FASTTEST_CMAKE_FLAGS <<< "${FASTTEST_CMAKE_FLAGS:-}" - FASTTEST_WORKSPACE=$(readlink -f "${FASTTEST_WORKSPACE:-.}") FASTTEST_SOURCE=$(readlink -f "${FASTTEST_SOURCE:-$FASTTEST_WORKSPACE/ch}") FASTTEST_BUILD=$(readlink -f "${FASTTEST_BUILD:-${BUILD:-$FASTTEST_WORKSPACE/build}}") FASTTEST_DATA=$(readlink -f "${FASTTEST_DATA:-$FASTTEST_WORKSPACE/db-fasttest}") FASTTEST_OUTPUT=$(readlink -f "${FASTTEST_OUTPUT:-$FASTTEST_WORKSPACE}") +# Export these variables, so that all subsequent invocations of the script +# use them, and not try to guess them anew, which leads to weird effects. +export FASTTEST_WORKSPACE +export FASTTEST_SOURCE +export FASTTEST_BUILD +export FASTTEST_DATA +export FASTTEST_OUT + server_pid=none function stop_server From 16ab4d59e6a90360064c765c6c390bb49ffbb764 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Wed, 30 Sep 2020 20:36:02 +0300 Subject: [PATCH 023/145] don't shadow local var --- src/Interpreters/executeQuery.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Interpreters/executeQuery.cpp b/src/Interpreters/executeQuery.cpp index d89725b30f4..94a58a1bda8 100644 --- a/src/Interpreters/executeQuery.cpp +++ b/src/Interpreters/executeQuery.cpp @@ -594,9 +594,9 @@ static std::tuple executeQueryImpl( elem.type = QueryLogElementType::QUERY_FINISH; - const auto current_time = std::chrono::system_clock::now(); + const auto finish_time = std::chrono::system_clock::now(); - elem.event_time = time_in_seconds(current_time); + elem.event_time = time_in_seconds(finish_time); status_info_to_query_log(elem, info, ast); @@ -658,7 +658,7 @@ static std::tuple executeQueryImpl( span.parent_span_id = context.getClientInfo().opentelemetry_parent_span_id; span.operation_name = "query"; span.start_time_us = elem.query_start_time_microseconds; - span.finish_time_us = time_in_microseconds(current_time); + span.finish_time_us = time_in_microseconds(finish_time); span.duration_ns = elapsed_seconds * 1000000000; // keep values synchonized to type enum in QueryLogElement::createBlock From 3a963c988d613d316bfaf9e0d129452586033173 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Fri, 2 Oct 2020 14:38:32 +0300 Subject: [PATCH 024/145] fixup --- docker/test/fasttest/run.sh | 5 ++++- src/Common/Exception.h | 22 ++++++++++++++++------ 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/docker/test/fasttest/run.sh b/docker/test/fasttest/run.sh index 5e586402bd7..a0830ba5f12 100755 --- a/docker/test/fasttest/run.sh +++ b/docker/test/fasttest/run.sh @@ -329,7 +329,10 @@ case "$stage" in ;& "run_tests") run_tests - ;& + ;; +*) + echo "Unknown test stage '$stage'" + exit 1 esac pstree -apgT diff --git a/src/Common/Exception.h b/src/Common/Exception.h index d0de8d6a3f2..314c59cbf51 100644 --- a/src/Common/Exception.h +++ b/src/Common/Exception.h @@ -22,10 +22,14 @@ public: Exception() = default; Exception(const std::string & msg, int code); + Exception(int code, const std::string & message) + : Exception(message, code) + {} + // Format message with fmt::format, like the logging functions. - template - Exception(int code, Fmt&&... fmt) - : Exception(fmt::format(std::forward(fmt)...), code) + template + Exception(int code, const std::string & fmt, Args&&... args) + : Exception(fmt::format(fmt, std::forward(args)...), code) {} struct CreateFromPocoTag {}; @@ -39,10 +43,16 @@ public: const char * name() const throw() override { return "DB::Exception"; } const char * what() const throw() override { return message().data(); } - template - void addMessage(Fmt&&... fmt) + /// Add something to the existing message. + template + void addMessage(const std::string& format, Args&&... args) { - extendedMessage(fmt::format(std::forward(fmt)...)); + extendedMessage(fmt::format(format, std::forward(args)...)); + } + + void addMessage(const std::string& message) + { + extendedMessage(message); } std::string getStackTraceString() const; From 72fddba2b1153598d9cc61c82b1ce49fbafde688 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Wed, 14 Oct 2020 17:08:51 +0300 Subject: [PATCH 025/145] more fasttest fixes --- docker/test/fasttest/run.sh | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/docker/test/fasttest/run.sh b/docker/test/fasttest/run.sh index a0830ba5f12..d77d7233893 100755 --- a/docker/test/fasttest/run.sh +++ b/docker/test/fasttest/run.sh @@ -33,6 +33,12 @@ server_pid=none function stop_server { + if kill -0 -- "$server_pid" + then + echo "ClickHouse server pid '$server_pid' is not running" + return 0 + fi + for _ in {1..60} do if ! pkill -f "clickhouse-server" && ! kill -- "$server_pid" ; then break ; fi @@ -167,8 +173,7 @@ clickhouse-client --version clickhouse-test --help mkdir -p "$FASTTEST_DATA"{,/client-config} -cp -a "$FASTTEST_SOURCE/programs/server/"{config,users}.xml "$FASTTEST_DATA" -cp -a "$FASTTEST_SOURCE/programs/server/"{config,users}.xml "$FASTTEST_DATA" +cp -a "$FASTTEST_SOURCE/programs/server/"{config,users}.{xml,d} "$FASTTEST_DATA" "$FASTTEST_SOURCE/tests/config/install.sh" "$FASTTEST_DATA" "$FASTTEST_DATA/client-config" # doesn't support SSL rm -f "$FASTTEST_DATA/config.d/secure_ports.xml" From ead6d5992c32921da59947ff2a7689ddcc9ca7d1 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Wed, 14 Oct 2020 18:53:14 +0300 Subject: [PATCH 026/145] fixup --- docker/test/fasttest/run.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docker/test/fasttest/run.sh b/docker/test/fasttest/run.sh index d77d7233893..299b748e4eb 100755 --- a/docker/test/fasttest/run.sh +++ b/docker/test/fasttest/run.sh @@ -33,7 +33,7 @@ server_pid=none function stop_server { - if kill -0 -- "$server_pid" + if ! kill -0 -- "$server_pid" then echo "ClickHouse server pid '$server_pid' is not running" return 0 @@ -274,7 +274,7 @@ then stop_server ||: # Clean the data so that there is no interference from the previous test run. - rm -rf "$FASTTEST_DATA"/{meta,}data ||: + rm -rf "$FASTTEST_DATA"/{{meta,}data,user_files} ||: start_server From ee8a9d2cf12220fb1420e2186cb8735256ed4fa5 Mon Sep 17 00:00:00 2001 From: Pavel Kruglov Date: Thu, 15 Oct 2020 18:57:17 +0300 Subject: [PATCH 027/145] Don't touch MySQL database if it's unnecessary --- src/Databases/DatabaseLazy.h | 4 ++++ src/Databases/IDatabase.h | 4 ++++ src/Databases/MySQL/DatabaseConnectionMySQL.h | 6 ++++++ src/Interpreters/AsynchronousMetrics.cpp | 4 ++-- src/Server/ReplicasStatusHandler.cpp | 4 ++-- src/Storages/System/StorageSystemDistributionQueue.cpp | 4 ++-- src/Storages/System/StorageSystemGraphite.cpp | 4 ++-- src/Storages/System/StorageSystemMutations.cpp | 4 ++-- src/Storages/System/StorageSystemPartsBase.cpp | 6 +++--- src/Storages/System/StorageSystemReplicas.cpp | 4 ++-- src/Storages/System/StorageSystemReplicationQueue.cpp | 4 ++-- 11 files changed, 31 insertions(+), 17 deletions(-) diff --git a/src/Databases/DatabaseLazy.h b/src/Databases/DatabaseLazy.h index 13c14863efb..2d091297c91 100644 --- a/src/Databases/DatabaseLazy.h +++ b/src/Databases/DatabaseLazy.h @@ -22,6 +22,10 @@ public: String getEngineName() const override { return "Lazy"; } + bool canContainMergeTreeTables() const override { return false; } + + bool canContainDistributedTables() const override { return false; } + void loadStoredObjects( Context & context, bool has_force_restore_data_flag, bool force_attach) override; diff --git a/src/Databases/IDatabase.h b/src/Databases/IDatabase.h index b28bd5fd599..fadec5fe7a9 100644 --- a/src/Databases/IDatabase.h +++ b/src/Databases/IDatabase.h @@ -147,6 +147,10 @@ public: /// Get name of database engine. virtual String getEngineName() const = 0; + virtual bool canContainMergeTreeTables() const { return true; } + + virtual bool canContainDistributedTables() const { return true; } + /// Load a set of existing tables. /// You can call only once, right after the object is created. virtual void loadStoredObjects(Context & /*context*/, bool /*has_force_restore_data_flag*/, bool /*force_attach*/ = false) {} diff --git a/src/Databases/MySQL/DatabaseConnectionMySQL.h b/src/Databases/MySQL/DatabaseConnectionMySQL.h index 7bf5e8c1d88..d8694e71db2 100644 --- a/src/Databases/MySQL/DatabaseConnectionMySQL.h +++ b/src/Databases/MySQL/DatabaseConnectionMySQL.h @@ -42,6 +42,12 @@ public: String getEngineName() const override { return "MySQL"; } + bool canContainMergeTreeTables() const override { return false; } + + bool canContainDistributedTables() const override { return false; } + + bool shouldBeEmptyOnDetach() const override { return false; } + bool empty() const override; DatabaseTablesIteratorPtr getTablesIterator(const Context & context, const FilterByNameFunction & filter_by_table_name) override; diff --git a/src/Interpreters/AsynchronousMetrics.cpp b/src/Interpreters/AsynchronousMetrics.cpp index feb2036a0d6..e1a9a820ebb 100644 --- a/src/Interpreters/AsynchronousMetrics.cpp +++ b/src/Interpreters/AsynchronousMetrics.cpp @@ -233,8 +233,8 @@ void AsynchronousMetrics::update() for (const auto & db : databases) { - /// Lazy database can not contain MergeTree tables - if (db.second->getEngineName() == "Lazy") + /// Check if database can contain MergeTree tables + if (!db.second->canContainMergeTreeTables()) continue; for (auto iterator = db.second->getTablesIterator(context); iterator->isValid(); iterator->next()) { diff --git a/src/Server/ReplicasStatusHandler.cpp b/src/Server/ReplicasStatusHandler.cpp index bc5436f00ee..1aa5c10afd7 100644 --- a/src/Server/ReplicasStatusHandler.cpp +++ b/src/Server/ReplicasStatusHandler.cpp @@ -43,8 +43,8 @@ void ReplicasStatusHandler::handleRequest(Poco::Net::HTTPServerRequest & request /// Iterate through all the replicated tables. for (const auto & db : databases) { - /// Lazy database can not contain replicated tables - if (db.second->getEngineName() == "Lazy") + /// Check if database can contain replicated tables + if (!db.second->canContainMergeTreeTables()) continue; for (auto iterator = db.second->getTablesIterator(context); iterator->isValid(); iterator->next()) diff --git a/src/Storages/System/StorageSystemDistributionQueue.cpp b/src/Storages/System/StorageSystemDistributionQueue.cpp index 2459be0ba71..39ccea64e26 100644 --- a/src/Storages/System/StorageSystemDistributionQueue.cpp +++ b/src/Storages/System/StorageSystemDistributionQueue.cpp @@ -38,8 +38,8 @@ void StorageSystemDistributionQueue::fillData(MutableColumns & res_columns, cons std::map> tables; for (const auto & db : DatabaseCatalog::instance().getDatabases()) { - /// Lazy database can not contain distributed tables - if (db.second->getEngineName() == "Lazy") + /// Check if database can contain distributed tables + if (!db.second->canContainDistributedTables()) continue; const bool check_access_for_tables = check_access_for_databases && !access->isGranted(AccessType::SHOW_TABLES, db.first); diff --git a/src/Storages/System/StorageSystemGraphite.cpp b/src/Storages/System/StorageSystemGraphite.cpp index ffa789a4751..93bc16785b2 100644 --- a/src/Storages/System/StorageSystemGraphite.cpp +++ b/src/Storages/System/StorageSystemGraphite.cpp @@ -32,8 +32,8 @@ static StorageSystemGraphite::Configs getConfigs(const Context & context) for (const auto & db : databases) { - /// Lazy database can not contain MergeTree tables - if (db.second->getEngineName() == "Lazy") + /// Check if database can contain MergeTree tables + if (!db.second->canContainMergeTreeTables()) continue; for (auto iterator = db.second->getTablesIterator(context); iterator->isValid(); iterator->next()) diff --git a/src/Storages/System/StorageSystemMutations.cpp b/src/Storages/System/StorageSystemMutations.cpp index 32f672b8401..f66f57ef5d1 100644 --- a/src/Storages/System/StorageSystemMutations.cpp +++ b/src/Storages/System/StorageSystemMutations.cpp @@ -44,8 +44,8 @@ void StorageSystemMutations::fillData(MutableColumns & res_columns, const Contex std::map> merge_tree_tables; for (const auto & db : DatabaseCatalog::instance().getDatabases()) { - /// Lazy database can not contain MergeTree tables - if (db.second->getEngineName() == "Lazy") + /// Check if database can contain MergeTree tables + if (!db.second->canContainMergeTreeTables()) continue; const bool check_access_for_tables = check_access_for_databases && !access->isGranted(AccessType::SHOW_TABLES, db.first); diff --git a/src/Storages/System/StorageSystemPartsBase.cpp b/src/Storages/System/StorageSystemPartsBase.cpp index faa2ec0e1c3..d10346af89f 100644 --- a/src/Storages/System/StorageSystemPartsBase.cpp +++ b/src/Storages/System/StorageSystemPartsBase.cpp @@ -83,9 +83,9 @@ StoragesInfoStream::StoragesInfoStream(const SelectQueryInfo & query_info, const MutableColumnPtr database_column_mut = ColumnString::create(); for (const auto & database : databases) { - /// Lazy database can not contain MergeTree tables - /// and it's unnecessary to load all tables of Lazy database just to filter all of them. - if (database.second->getEngineName() != "Lazy") + /// Checck if database can contain MergeTree tables, + /// if not it's unnecessary to load all tables of database just to filter all of them. + if (database.second->canContainMergeTreeTables()) database_column_mut->insert(database.first); } block_to_filter.insert(ColumnWithTypeAndName( diff --git a/src/Storages/System/StorageSystemReplicas.cpp b/src/Storages/System/StorageSystemReplicas.cpp index 7ab6e939815..973ccfbf464 100644 --- a/src/Storages/System/StorageSystemReplicas.cpp +++ b/src/Storages/System/StorageSystemReplicas.cpp @@ -74,8 +74,8 @@ Pipe StorageSystemReplicas::read( std::map> replicated_tables; for (const auto & db : DatabaseCatalog::instance().getDatabases()) { - /// Lazy database can not contain replicated tables - if (db.second->getEngineName() == "Lazy") + /// Check if database can contain replicated tables + if (db.second->canContainMergeTreeTables()) continue; const bool check_access_for_tables = check_access_for_databases && !access->isGranted(AccessType::SHOW_TABLES, db.first); for (auto iterator = db.second->getTablesIterator(context); iterator->isValid(); iterator->next()) diff --git a/src/Storages/System/StorageSystemReplicationQueue.cpp b/src/Storages/System/StorageSystemReplicationQueue.cpp index f04d8759507..9cd5e8b8ff3 100644 --- a/src/Storages/System/StorageSystemReplicationQueue.cpp +++ b/src/Storages/System/StorageSystemReplicationQueue.cpp @@ -55,8 +55,8 @@ void StorageSystemReplicationQueue::fillData(MutableColumns & res_columns, const std::map> replicated_tables; for (const auto & db : DatabaseCatalog::instance().getDatabases()) { - /// Lazy database can not contain replicated tables - if (db.second->getEngineName() == "Lazy") + /// Check if database can contain replicated tables + if (!db.second->canContainMergeTreeTables()) continue; const bool check_access_for_tables = check_access_for_databases && !access->isGranted(AccessType::SHOW_TABLES, db.first); From f02840449e6b3c5bfcdb4d7cb03e6489e8e22b1f Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Thu, 15 Oct 2020 20:27:18 +0300 Subject: [PATCH 028/145] fix the test --- .../01455_opentelemetry_distributed.sh | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/tests/queries/0_stateless/01455_opentelemetry_distributed.sh b/tests/queries/0_stateless/01455_opentelemetry_distributed.sh index d3114ab66ff..446f713c11e 100755 --- a/tests/queries/0_stateless/01455_opentelemetry_distributed.sh +++ b/tests/queries/0_stateless/01455_opentelemetry_distributed.sh @@ -63,14 +63,24 @@ check_log echo "===sampled===" query_id=$(${CLICKHOUSE_CLIENT} -q "select lower(hex(reverse(reinterpretAsString(generateUUIDv4()))))") -for _ in {1..200} +for i in {1..200} do ${CLICKHOUSE_CLIENT} \ --opentelemetry_start_trace_probability=0.1 \ - --query_id "$query_id" \ - --query "select 1 from remote('127.0.0.2', system, one) format Null" -done + --query_id "$query_id-$i" \ + --query "select 1 from remote('127.0.0.2', system, one) format Null" \ + & + # clickhouse-client is slow to start, so run them in parallel, but not too + # much. + if [[ $((i % 10)) -eq 0 ]] + then + wait + fi +done +wait + +${CLICKHOUSE_CLIENT} -q "system flush logs" ${CLICKHOUSE_CLIENT} -q " with count(*) as c -- expect 200 * 0.1 = 20 sampled events on average @@ -78,6 +88,8 @@ ${CLICKHOUSE_CLIENT} -q " from system.opentelemetry_log array join attribute.names as name, attribute.values as value where name = 'clickhouse.query_id' - and value = '$query_id' + and operation_name = 'query' + and parent_span_id = 0 -- only account for the initial queries + and value like '$query_id-%' ; " From dc8e165ce4fa98d715f2d3817057a4eb2d5b47cf Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Thu, 15 Oct 2020 20:27:26 +0300 Subject: [PATCH 029/145] improve fasttest usability --- docker/test/fasttest/Dockerfile | 1 - docker/test/fasttest/run.sh | 100 +++++++++--------- tests/clickhouse-test | 8 +- .../0_stateless/01293_show_clusters.reference | 6 -- .../0_stateless/01293_show_clusters.sql | 4 +- 5 files changed, 58 insertions(+), 61 deletions(-) diff --git a/docker/test/fasttest/Dockerfile b/docker/test/fasttest/Dockerfile index 3cfa57bd747..6547a98c58b 100644 --- a/docker/test/fasttest/Dockerfile +++ b/docker/test/fasttest/Dockerfile @@ -56,7 +56,6 @@ RUN apt-get update \ python3-lxml \ python3-requests \ python3-termcolor \ - qemu-user-static \ rename \ software-properties-common \ tzdata \ diff --git a/docker/test/fasttest/run.sh b/docker/test/fasttest/run.sh index ad44cfc3cf9..f12ecbb2c9c 100755 --- a/docker/test/fasttest/run.sh +++ b/docker/test/fasttest/run.sh @@ -191,63 +191,65 @@ stop_server ||: start_server TESTS_TO_SKIP=( - parquet - avro - h3 - odbc - mysql - sha256 - _orc_ - arrow - 01098_temporary_and_external_tables - 01083_expressions_in_engine_arguments - hdfs - 00911_tautological_compare - protobuf - capnproto - java_hash - hashing - secure - 00490_special_line_separators_and_characters_outside_of_bmp - 00436_convert_charset 00105_shard_collations - 01354_order_by_tuple_collate_const - 01292_create_user - 01098_msgpack_format - 00929_multi_match_edit_distance - 00926_multimatch - 00834_cancel_http_readonly_queries_on_client_close - brotli - parallel_alter + 00109_shard_totals_after_having + 00110_external_sort 00302_http_compression 00417_kill_query - 01294_lazy_database_concurrent - 01193_metadata_loading - base64 - 01031_mutations_interpreter_and_context - json - client - 01305_replica_create_drop_zookeeper - 01092_memory_profiler - 01355_ilike - 01281_unsucceeded_insert_select_queries_counter - live_view - limit_memory - memory_limit - memory_leak - 00110_external_sort + 00436_convert_charset + 00490_special_line_separators_and_characters_outside_of_bmp + 00652_replicated_mutations_zookeeper 00682_empty_parts_merge 00701_rollup - 00109_shard_totals_after_having - ddl_dictionaries + 00834_cancel_http_readonly_queries_on_client_close + 00911_tautological_compare + 00926_multimatch + 00929_multi_match_edit_distance + 01031_mutations_interpreter_and_context + 01053_ssd_dictionary # this test mistakenly requires acces to /var/lib/clickhouse -- can't run this locally, disabled + 01083_expressions_in_engine_arguments + 01092_memory_profiler + 01098_msgpack_format + 01098_temporary_and_external_tables + 01103_check_cpu_instructions_at_startup # avoid dependency on qemu -- invonvenient when running locally + 01193_metadata_loading + 01238_http_memory_tracking # max_memory_usage_for_user can interfere another queries running concurrently 01251_dict_is_in_infinite_loop 01259_dictionary_custom_settings_ddl 01268_dictionary_direct_layout 01280_ssd_complex_key_dictionary - 00652_replicated_mutations_zookeeper - 01411_bayesian_ab_testing - 01238_http_memory_tracking # max_memory_usage_for_user can interfere another queries running concurrently 01281_group_by_limit_memory_tracking # max_memory_usage_for_user can interfere another queries running concurrently + 01281_unsucceeded_insert_select_queries_counter + 01292_create_user + 01294_lazy_database_concurrent + 01305_replica_create_drop_zookeeper + 01354_order_by_tuple_collate_const + 01355_ilike + 01411_bayesian_ab_testing + _orc_ + arrow + avro + base64 + brotli + capnproto + client + ddl_dictionaries + h3 + hashing + hdfs + java_hash + json + limit_memory + live_view + memory_leak + memory_limit + mysql + odbc + parallel_alter + parquet + protobuf + secure + sha256 # Not sure why these two fail even in sequential mode. Disabled for now # to make some progress. @@ -258,7 +260,7 @@ TESTS_TO_SKIP=( 01460_DistributedFilesToInsert ) -time clickhouse-test -j 8 --no-long --testname --shard --zookeeper --skip "${TESTS_TO_SKIP[@]}" 2>&1 | ts '%Y-%m-%d %H:%M:%S' | tee "$FASTTEST_OUTPUT/test_log.txt" +time clickhouse-test -j 8 --order=random --no-long --testname --shard --zookeeper --skip "${TESTS_TO_SKIP[@]}" 2>&1 | ts '%Y-%m-%d %H:%M:%S' | tee "$FASTTEST_OUTPUT/test_log.txt" # substr is to remove semicolon after test name readarray -t FAILED_TESTS < <(awk '/FAIL|TIMEOUT|ERROR/ { print substr($3, 1, length($3)-1) }' "$FASTTEST_OUTPUT/test_log.txt" | tee "$FASTTEST_OUTPUT/failed-parallel-tests.txt") @@ -281,7 +283,7 @@ then echo "Going to run again: ${FAILED_TESTS[*]}" - clickhouse-test --no-long --testname --shard --zookeeper "${FAILED_TESTS[@]}" 2>&1 | ts '%Y-%m-%d %H:%M:%S' | tee -a "$FASTTEST_OUTPUT/test_log.txt" + clickhouse-test --order=random --no-long --testname --shard --zookeeper "${FAILED_TESTS[@]}" 2>&1 | ts '%Y-%m-%d %H:%M:%S' | tee -a "$FASTTEST_OUTPUT/test_log.txt" else echo "No failed tests" fi diff --git a/tests/clickhouse-test b/tests/clickhouse-test index be8f9551db2..d5736001d60 100755 --- a/tests/clickhouse-test +++ b/tests/clickhouse-test @@ -353,20 +353,22 @@ def run_tests_array(all_tests_with_params): if os.path.isfile(stdout_file): print(", result:\n") - print(open(stdout_file).read()) + print('\n'.join(open(stdout_file).read().split('\n')[:100])) elif stderr: failures += 1 failures_chain += 1 print(MSG_FAIL, end='') print_test_time(total_time) - print(" - having stderror:\n{}".format(stderr)) + print(" - having stderror:\n{}".format( + '\n'.join(stderr.split('\n')[:100]))) elif 'Exception' in stdout: failures += 1 failures_chain += 1 print(MSG_FAIL, end='') print_test_time(total_time) - print(" - having exception:\n{}".format(stdout)) + print(" - having exception:\n{}".format( + '\n'.join(stdout.split('\n')[:100]))) elif not os.path.isfile(reference_file): print(MSG_UNKNOWN, end='') print_test_time(total_time) diff --git a/tests/queries/0_stateless/01293_show_clusters.reference b/tests/queries/0_stateless/01293_show_clusters.reference index 39e25143131..590ca348458 100644 --- a/tests/queries/0_stateless/01293_show_clusters.reference +++ b/tests/queries/0_stateless/01293_show_clusters.reference @@ -1,8 +1,2 @@ -test_cluster_two_shards -test_cluster_two_shards_different_databases -test_cluster_two_shards_localhost test_shard_localhost -test_shard_localhost_secure -test_unavailable_shard -test_cluster_two_shards test_shard_localhost 1 1 1 localhost ::1 9000 1 default 0 0 diff --git a/tests/queries/0_stateless/01293_show_clusters.sql b/tests/queries/0_stateless/01293_show_clusters.sql index af450680dac..ad5e51531e3 100644 --- a/tests/queries/0_stateless/01293_show_clusters.sql +++ b/tests/queries/0_stateless/01293_show_clusters.sql @@ -1,3 +1,3 @@ -show clusters; -show clusters like 'test%' limit 1; +-- don't show all clusters to reduce dependency on the configuration of server +show clusters like 'test_shard%' limit 1; show cluster 'test_shard_localhost'; From 708fedbcf8bc700d21bdab7ecf66b590abb6fd75 Mon Sep 17 00:00:00 2001 From: MyroTk Date: Thu, 15 Oct 2020 22:23:49 +0200 Subject: [PATCH 030/145] testflows RBAC tests for views, distributed tables, public tables, and privileges: alter, updated select, updated insert, and show tables --- tests/testflows/example/regression.py | 6 +- tests/testflows/helpers/argparser.py | 15 +- tests/testflows/helpers/cluster.py | 89 +- tests/testflows/ldap/regression.py | 6 +- .../configs/clickhouse/config.d/remote.xml | 73 +- .../rbac/configs/clickhouse/config.d/ssl.xml | 1 + .../rbac/configs/clickhouse/config.xml | 20 +- .../docker-compose/clickhouse-service.yml | 0 .../rbac/docker-compose/docker-compose.yml | 4 +- .../rbac/docker-compose/zookeeper-service.yml | 8 +- tests/testflows/rbac/helper/__init__.py | 0 tests/testflows/rbac/helper/common.py | 124 + tests/testflows/rbac/helper/errors.py | 123 + tests/testflows/rbac/helper/tables.py | 41 + tests/testflows/rbac/regression.py | 118 +- tests/testflows/rbac/requirements/__init__.py | 2 +- .../rbac/requirements/requirements.md | 959 ++++++- .../rbac/requirements/requirements.py | 1332 +++++++++- .../rbac/tests/privileges/__init__.py | 7 + .../rbac/tests/privileges/alter/__init__.py | 0 .../tests/privileges/alter/alter_column.py | 993 ++++++++ .../privileges/alter/alter_constraint.py | 629 +++++ .../tests/privileges/alter/alter_index.py | 815 ++++++ .../tests/privileges/alter/alter_settings.py | 476 ++++ .../rbac/tests/privileges/alter/alter_ttl.py | 605 +++++ .../tests/privileges/distributed_table.py | 1000 ++++++++ .../rbac/tests/privileges/feature.py | 24 +- .../testflows/rbac/tests/privileges/insert.py | 570 +++-- .../rbac/tests/privileges/public_tables.py | 38 + .../testflows/rbac/tests/privileges/select.py | 419 ++- .../rbac/tests/privileges/show_tables.py | 62 + .../rbac/tests/syntax/alter_quota.py | 8 +- .../testflows/rbac/tests/syntax/alter_role.py | 2 +- .../rbac/tests/syntax/alter_row_policy.py | 2 +- .../tests/syntax/alter_settings_profile.py | 10 +- .../testflows/rbac/tests/syntax/alter_user.py | 2 +- .../rbac/tests/syntax/create_quota.py | 6 +- .../rbac/tests/syntax/create_role.py | 6 +- .../rbac/tests/syntax/create_row_policy.py | 8 +- .../tests/syntax/create_settings_profile.py | 4 +- .../rbac/tests/syntax/create_user.py | 2 +- .../testflows/rbac/tests/syntax/drop_quota.py | 2 +- .../testflows/rbac/tests/syntax/drop_role.py | 6 +- .../rbac/tests/syntax/drop_row_policy.py | 2 +- .../tests/syntax/drop_settings_profile.py | 2 +- .../testflows/rbac/tests/syntax/drop_user.py | 2 +- .../rbac/tests/syntax/grant_privilege.py | 6 +- .../testflows/rbac/tests/syntax/grant_role.py | 10 +- .../rbac/tests/syntax/revoke_privilege.py | 52 +- .../rbac/tests/syntax/revoke_role.py | 6 +- .../rbac/tests/syntax/set_default_role.py | 4 +- tests/testflows/rbac/tests/syntax/set_role.py | 2 +- .../rbac/tests/syntax/show_create_role.py | 2 +- tests/testflows/rbac/tests/views/__init__.py | 0 tests/testflows/rbac/tests/views/feature.py | 20 + tests/testflows/rbac/tests/views/live_view.py | 1141 +++++++++ .../rbac/tests/views/materialized_view.py | 2268 +++++++++++++++++ tests/testflows/rbac/tests/views/view.py | 1162 +++++++++ tests/testflows/regression.py | 4 +- 59 files changed, 12433 insertions(+), 867 deletions(-) mode change 100644 => 100755 tests/testflows/helpers/cluster.py mode change 100644 => 100755 tests/testflows/rbac/docker-compose/clickhouse-service.yml mode change 100644 => 100755 tests/testflows/rbac/docker-compose/docker-compose.yml mode change 100644 => 100755 tests/testflows/rbac/docker-compose/zookeeper-service.yml create mode 100644 tests/testflows/rbac/helper/__init__.py create mode 100755 tests/testflows/rbac/helper/common.py create mode 100755 tests/testflows/rbac/helper/errors.py create mode 100755 tests/testflows/rbac/helper/tables.py mode change 100644 => 100755 tests/testflows/rbac/requirements/requirements.py create mode 100755 tests/testflows/rbac/tests/privileges/alter/__init__.py create mode 100755 tests/testflows/rbac/tests/privileges/alter/alter_column.py create mode 100755 tests/testflows/rbac/tests/privileges/alter/alter_constraint.py create mode 100755 tests/testflows/rbac/tests/privileges/alter/alter_index.py create mode 100755 tests/testflows/rbac/tests/privileges/alter/alter_settings.py create mode 100755 tests/testflows/rbac/tests/privileges/alter/alter_ttl.py create mode 100755 tests/testflows/rbac/tests/privileges/distributed_table.py create mode 100755 tests/testflows/rbac/tests/privileges/public_tables.py mode change 100644 => 100755 tests/testflows/rbac/tests/privileges/select.py create mode 100755 tests/testflows/rbac/tests/privileges/show_tables.py create mode 100755 tests/testflows/rbac/tests/views/__init__.py create mode 100755 tests/testflows/rbac/tests/views/feature.py create mode 100755 tests/testflows/rbac/tests/views/live_view.py create mode 100755 tests/testflows/rbac/tests/views/materialized_view.py create mode 100755 tests/testflows/rbac/tests/views/view.py diff --git a/tests/testflows/example/regression.py b/tests/testflows/example/regression.py index 2c0a778d39b..cb58b42ba4c 100755 --- a/tests/testflows/example/regression.py +++ b/tests/testflows/example/regression.py @@ -2,7 +2,7 @@ import sys from testflows.core import * -append_path(sys.path, "..") +append_path(sys.path, "..") from helpers.cluster import Cluster from helpers.argparser import argparser @@ -10,13 +10,13 @@ from helpers.argparser import argparser @TestFeature @Name("example") @ArgumentParser(argparser) -def regression(self, local, clickhouse_binary_path): +def regression(self, local, clickhouse_binary_path, stress=None, parallel=None): """Simple example of how you can use TestFlows to test ClickHouse. """ nodes = { "clickhouse": ("clickhouse1",), } - + with Cluster(local, clickhouse_binary_path, nodes=nodes) as cluster: self.context.cluster = cluster diff --git a/tests/testflows/helpers/argparser.py b/tests/testflows/helpers/argparser.py index 033c15a3bfe..03014becb76 100644 --- a/tests/testflows/helpers/argparser.py +++ b/tests/testflows/helpers/argparser.py @@ -1,5 +1,12 @@ import os +def onoff(v): + if v in ["yes", "1", "on"]: + return True + elif v in ["no", "0", "off"]: + return False + raise ValueError(f"invalid {v}") + def argparser(parser): """Default argument parser for regressions. """ @@ -10,4 +17,10 @@ def argparser(parser): parser.add_argument("--clickhouse-binary-path", type=str, dest="clickhouse_binary_path", help="path to ClickHouse binary, default: /usr/bin/clickhouse", metavar="path", - default=os.getenv("CLICKHOUSE_TESTS_SERVER_BIN_PATH", "/usr/bin/clickhouse")) \ No newline at end of file + default=os.getenv("CLICKHOUSE_TESTS_SERVER_BIN_PATH", "/usr/bin/clickhouse")) + + parser.add_argument("--stress", action="store_true", default=False, + help="enable stress testing (might take a long time)") + + parser.add_argument("--parallel", type=onoff, default=True, choices=["yes", "no", "on", "off", 0, 1], + help="enable parallelism for tests that support it") \ No newline at end of file diff --git a/tests/testflows/helpers/cluster.py b/tests/testflows/helpers/cluster.py old mode 100644 new mode 100755 index 8fda8ac43d8..490a9f4e17e --- a/tests/testflows/helpers/cluster.py +++ b/tests/testflows/helpers/cluster.py @@ -7,6 +7,7 @@ import tempfile from testflows.core import * from testflows.asserts import error from testflows.connect import Shell +from testflows.uexpect import ExpectTimeoutError class QueryRuntimeException(Exception): """Exception during query execution on the server. @@ -78,32 +79,43 @@ class ClickHouseNode(Node): def query(self, sql, message=None, exitcode=None, steps=True, no_checks=False, raise_on_exception=False, step=By, settings=None, *args, **kwargs): """Execute and check query. - :param sql: sql query :param message: expected message that should be in the output, default: None :param exitcode: expected exitcode, default: None """ + settings = list(settings or []) + + if hasattr(current().context, "default_query_settings"): + settings += current().context.default_query_settings + if len(sql) > 1024: with tempfile.NamedTemporaryFile("w", encoding="utf-8") as query: query.write(sql) query.flush() command = f"cat \"{query.name}\" | {self.cluster.docker_compose} exec -T {self.name} clickhouse client -n" - for setting in settings or []: + for setting in settings: name, value = setting command += f" --{name} \"{value}\"" description = f""" echo -e \"{sql[:100]}...\" > {query.name} {command} """ - with step("executing command", description=description) if steps else NullStep(): - r = self.cluster.bash(None)(command, *args, **kwargs) + with step("executing command", description=description, format_description=False) if steps else NullStep(): + try: + r = self.cluster.bash(None)(command, *args, **kwargs) + except ExpectTimeoutError: + self.cluster.close_bash(None) else: command = f"echo -e \"{sql}\" | clickhouse client -n" - for setting in settings or []: + for setting in settings: name, value = setting command += f" --{name} \"{value}\"" - with step("executing command", description=command) if steps else NullStep(): - r = self.cluster.bash(self.name)(command, *args, **kwargs) + with step("executing command", description=command, format_description=False) if steps else NullStep(): + try: + r = self.cluster.bash(self.name)(command, *args, **kwargs) + except ExpectTimeoutError: + self.cluster.close_bash(self.name) + raise if no_checks: return r @@ -134,6 +146,7 @@ class Cluster(object): docker_compose="docker-compose", docker_compose_project_dir=None, docker_compose_file="docker-compose.yml"): + self.terminating = False self._bash = {} self.clickhouse_binary_path = clickhouse_binary_path self.configs_dir = configs_dir @@ -183,11 +196,19 @@ class Cluster(object): def bash(self, node, timeout=120): """Returns thread-local bash terminal to a specific node. - :param node: name of the service """ + test = current() + + if self.terminating: + if test and (test.cflags & MANDATORY): + pass + else: + raise InterruptedError("terminating") + current_thread = threading.current_thread() - id = f"{current_thread.ident}-{node}" + id = f"{current_thread.name}-{node}" + with self.lock: if self._bash.get(id) is None: if node is None: @@ -196,9 +217,30 @@ class Cluster(object): self._bash[id] = Shell(command=[ "/bin/bash", "--noediting", "-c", f"{self.docker_compose} exec {node} bash --noediting" ], name=node).__enter__() + self._bash[id].timeout = timeout + + # clean up any stale open shells for threads that have exited + active_thread_names = {thread.name for thread in threading.enumerate()} + + for bash_id in list(self._bash.keys()): + thread_name, node_name = bash_id.rsplit("-", 1) + if thread_name not in active_thread_names: + self._bash[bash_id].__exit__(None, None, None) + del self._bash[bash_id] + return self._bash[id] + def close_bash(self, node): + current_thread = threading.current_thread() + id = f"{current_thread.name}-{node}" + + with self.lock: + if self._bash.get(id) is None: + return + self._bash[id].__exit__(None, None, None) + del self._bash[id] + def __enter__(self): with Given("docker-compose cluster"): self.up() @@ -210,20 +252,21 @@ class Cluster(object): self.down() finally: with self.lock: - for shell in list(self._bash.values()): + for shell in self._bash.values(): shell.__exit__(type, value, traceback) def node(self, name): """Get object with node bound methods. - :param name: name of service name """ if name.startswith("clickhouse"): return ClickHouseNode(self, name) return Node(self, name) - def down(self, timeout=120): + def down(self, timeout=300): """Bring cluster down by executing docker-compose down.""" + self.terminating = True + try: bash = self.bash(None) with self.lock: @@ -235,7 +278,7 @@ class Cluster(object): else: self._bash[id] = shell finally: - return self.command(None, f"{self.docker_compose} down", timeout=timeout) + return self.command(None, f"{self.docker_compose} down", bash=bash, timeout=timeout) def up(self, timeout=30*60): if self.local: @@ -264,7 +307,7 @@ class Cluster(object): if cmd.exitcode != 0: continue with And("executing docker-compose down just in case it is up"): - cmd = self.command(None, f"{self.docker_compose} down 2>&1 | tee", exitcode=None, timeout=timeout) + cmd = self.command(None, f"{self.docker_compose} down --remove-orphans 2>&1 | tee", exitcode=None, timeout=timeout) if cmd.exitcode != 0: continue with And("executing docker-compose up"): @@ -285,22 +328,26 @@ class Cluster(object): for name in self.nodes["clickhouse"]: self.node(name).wait_healthy() - def command(self, node, command, message=None, exitcode=None, steps=True, *args, **kwargs): + def command(self, node, command, message=None, exitcode=None, steps=True, bash=None, *args, **kwargs): """Execute and check command. - :param node: name of the service :param command: command :param message: expected message that should be in the output, default: None :param exitcode: expected exitcode, default: None :param steps: don't break command into steps, default: True """ - debug(f"command() {node}, {command}") - with By("executing command", description=command) if steps else NullStep(): - r = self.bash(node)(command, *args, **kwargs) + with By("executing command", description=command, format_description=False) if steps else NullStep(): + if bash is None: + bash = self.bash(node) + try: + r = bash(command, *args, **kwargs) + except ExpectTimeoutError: + self.close_bash(node) + raise if exitcode is not None: - with Then(f"exitcode should be {exitcode}") if steps else NullStep(): + with Then(f"exitcode should be {exitcode}", format_name=False) if steps else NullStep(): assert r.exitcode == exitcode, error(r.output) if message is not None: - with Then(f"output should contain message", description=message) if steps else NullStep(): + with Then(f"output should contain message", description=message, format_description=False) if steps else NullStep(): assert message in r.output, error(r.output) return r diff --git a/tests/testflows/ldap/regression.py b/tests/testflows/ldap/regression.py index 567807fc0a8..1c0edcb57d9 100755 --- a/tests/testflows/ldap/regression.py +++ b/tests/testflows/ldap/regression.py @@ -2,7 +2,7 @@ import sys from testflows.core import * -append_path(sys.path, "..") +append_path(sys.path, "..") from helpers.cluster import Cluster from helpers.argparser import argparser @@ -33,13 +33,13 @@ xfails = { RQ_SRS_007_LDAP_Authentication("1.0") ) @XFails(xfails) -def regression(self, local, clickhouse_binary_path): +def regression(self, local, clickhouse_binary_path, stress=None, parallel=None): """ClickHouse integration with LDAP regression module. """ nodes = { "clickhouse": ("clickhouse1", "clickhouse2", "clickhouse3"), } - + with Cluster(local, clickhouse_binary_path, nodes=nodes) as cluster: self.context.cluster = cluster diff --git a/tests/testflows/rbac/configs/clickhouse/config.d/remote.xml b/tests/testflows/rbac/configs/clickhouse/config.d/remote.xml index ada8eec5fc9..a7ed0d6e2b4 100644 --- a/tests/testflows/rbac/configs/clickhouse/config.d/remote.xml +++ b/tests/testflows/rbac/configs/clickhouse/config.d/remote.xml @@ -58,9 +58,44 @@ 9440 1 - - - + + + + + + clickhouse1 + 9440 + 1 + + + + + + + clickhouse1 + 9000 + + + + + clickhouse2 + 9000 + + + + + + + clickhouse1 + 9000 + + + clickhouse2 + 9000 + + + + clickhouse2 @@ -73,8 +108,20 @@ 9000 - - + + + + + clickhouse2 + 9000 + + + clickhouse3 + 9000 + + + + clickhouse1 @@ -94,6 +141,22 @@ + + + + clickhouse1 + 9000 + + + clickhouse2 + 9000 + + + clickhouse3 + 9000 + + + diff --git a/tests/testflows/rbac/configs/clickhouse/config.d/ssl.xml b/tests/testflows/rbac/configs/clickhouse/config.d/ssl.xml index ca65ffd5e04..768d2250b79 100644 --- a/tests/testflows/rbac/configs/clickhouse/config.d/ssl.xml +++ b/tests/testflows/rbac/configs/clickhouse/config.d/ssl.xml @@ -3,6 +3,7 @@ /etc/clickhouse-server/ssl/server.crt /etc/clickhouse-server/ssl/server.key + /etc/clickhouse-server/ssl/dhparam.pem none true diff --git a/tests/testflows/rbac/configs/clickhouse/config.xml b/tests/testflows/rbac/configs/clickhouse/config.xml index 65187edf806..4ec12232539 100644 --- a/tests/testflows/rbac/configs/clickhouse/config.xml +++ b/tests/testflows/rbac/configs/clickhouse/config.xml @@ -69,7 +69,7 @@ - + 0.0.0.0 /var/lib/clickhouse/access/ + + + + + users.xml + + + + /var/lib/clickhouse/access/ + + + users.xml @@ -160,7 +172,7 @@ - + @@ -220,7 +232,7 @@ See https://clickhouse.yandex/docs/en/table_engines/replication/ --> - + - + - + + + + + + + <${CLICKHOUSE_USER}> + default + + ::/0 + + ${CLICKHOUSE_PASSWORD} + default + + + +EOT +fi + +if [ -n "$(ls /docker-entrypoint-initdb.d/)" ] || [ -n "$CLICKHOUSE_DB" ]; then + # Listen only on localhost until the initialization is done + $gosu /usr/bin/clickhouse-server --config-file=$CLICKHOUSE_CONFIG -- --listen_host=127.0.0.1 & + pid="$!" + + # check if clickhouse is ready to accept connections + # will try to send ping clickhouse via http_port (max 6 retries, with 1 sec timeout and 1 sec delay between retries) + tries=6 + while ! wget --spider -T 1 -q "http://localhost:$HTTP_PORT/ping" 2>/dev/null; do + if [ "$tries" -le "0" ]; then + echo >&2 'ClickHouse init process failed.' + exit 1 + fi + tries=$(( tries-1 )) + sleep 1 + done + + if [ ! -z "$CLICKHOUSE_PASSWORD" ]; then + printf -v WITH_PASSWORD '%s %q' "--password" "$CLICKHOUSE_PASSWORD" + fi + + clickhouseclient="clickhouse-client --multiquery -u $CLICKHOUSE_USER $WITH_PASSWORD " + + # create default database, if defined + if [ -n "$CLICKHOUSE_DB" ]; then + echo "$0: create database '$CLICKHOUSE_DB'" + "$clickhouseclient" -q "CREATE DATABASE IF NOT EXISTS $CLICKHOUSE_DB"; + fi + + for f in /docker-entrypoint-initdb.d/*; do + case "$f" in + *.sh) + if [ -x "$f" ]; then + echo "$0: running $f" + "$f" + else + echo "$0: sourcing $f" + . "$f" + fi + ;; + *.sql) echo "$0: running $f"; cat "$f" | "$clickhouseclient" ; echo ;; + *.sql.gz) echo "$0: running $f"; gunzip -c "$f" | "$clickhouseclient"; echo ;; + *) echo "$0: ignoring $f" ;; + esac + echo + done + + if ! kill -s TERM "$pid" || ! wait "$pid"; then + echo >&2 'Finishing of ClickHouse init process failed.' + exit 1 + fi +fi + +# if no args passed to `docker run` or first argument start with `--`, then the user is passing clickhouse-server arguments +if [[ $# -lt 1 ]] || [[ "$1" == "--"* ]]; then + exec $gosu /usr/bin/clickhouse-server --config-file=$CLICKHOUSE_CONFIG "$@" +fi + +# Otherwise, we assume the user want to run his own process, for example a `bash` shell to explore this image +exec "$@" From 2d28d97233622beb8d7a97299e8c7a437ee52dfc Mon Sep 17 00:00:00 2001 From: filimonov <1549571+filimonov@users.noreply.github.com> Date: Wed, 28 Oct 2020 15:13:27 +0100 Subject: [PATCH 137/145] Update other-functions.md --- docs/en/sql-reference/functions/other-functions.md | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/docs/en/sql-reference/functions/other-functions.md b/docs/en/sql-reference/functions/other-functions.md index 518479fb728..2cc80dcffc1 100644 --- a/docs/en/sql-reference/functions/other-functions.md +++ b/docs/en/sql-reference/functions/other-functions.md @@ -626,7 +626,12 @@ neighbor(column, offset[, default_value]) ``` The result of the function depends on the affected data blocks and the order of data in the block. -If you make a subquery with ORDER BY and call the function from outside the subquery, you can get the expected result. + +!!! warning "Warning" + It can reach the neighbor rows only inside the currently processed data block. + +The rows order used during the calculation of `neighbor` can differ from the order of rows returned to the user. +To prevent that you can make a subquery with ORDER BY and call the function from outside the subquery. **Parameters** @@ -731,8 +736,13 @@ Result: Calculates the difference between successive row values ​​in the data block. Returns 0 for the first row and the difference from the previous row for each subsequent row. +!!! warning "Warning" + It can reach the previos row only inside the currently processed data block. + The result of the function depends on the affected data blocks and the order of data in the block. -If you make a subquery with ORDER BY and call the function from outside the subquery, you can get the expected result. + +The rows order used during the calculation of `runningDifference` can differ from the order of rows returned to the user. +To prevent that you can make a subquery with ORDER BY and call the function from outside the subquery. Example: From a1f1db753b7705177f9472181f816e2a036289f8 Mon Sep 17 00:00:00 2001 From: tavplubix Date: Wed, 28 Oct 2020 18:23:10 +0300 Subject: [PATCH 138/145] Update CompressedReadBufferBase.cpp --- src/Compression/CompressedReadBufferBase.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Compression/CompressedReadBufferBase.cpp b/src/Compression/CompressedReadBufferBase.cpp index be2f697e1b3..7a6b605d015 100644 --- a/src/Compression/CompressedReadBufferBase.cpp +++ b/src/Compression/CompressedReadBufferBase.cpp @@ -185,9 +185,9 @@ void CompressedReadBufferBase::decompress(char * to, size_t size_decompressed, s } else { - throw Exception("Data compressed with different methods, given method byte " + throw Exception("Data compressed with different methods, given method byte 0x" + getHexUIntLowercase(method) - + ", previous method byte " + + ", previous method byte 0x" + getHexUIntLowercase(codec->getMethodByte()), ErrorCodes::CANNOT_DECOMPRESS); } From 885bd847201befc61f0063a113ab2eb8ad575741 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Wed, 28 Oct 2020 20:48:02 +0300 Subject: [PATCH 139/145] Remove trash from CMakeLists --- CMakeLists.txt | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 21cc74bbd2b..783a9f80b66 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -59,25 +59,6 @@ set(CMAKE_DEBUG_POSTFIX "d" CACHE STRING "Generate debug library name with a pos # For more info see https://cmake.org/cmake/help/latest/prop_gbl/USE_FOLDERS.html set_property(GLOBAL PROPERTY USE_FOLDERS ON) -# cmake 3.9+ needed. -# Usually impractical. -# See also ${ENABLE_THINLTO} -option(ENABLE_IPO "Full link time optimization") - -if(ENABLE_IPO) - cmake_policy(SET CMP0069 NEW) - include(CheckIPOSupported) - check_ipo_supported(RESULT IPO_SUPPORTED OUTPUT IPO_NOT_SUPPORTED) - if(IPO_SUPPORTED) - message(STATUS "IPO/LTO is supported, enabling") - set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE) - else() - message (${RECONFIGURE_MESSAGE_LEVEL} "IPO/LTO is not supported: <${IPO_NOT_SUPPORTED}>") - endif() -else() - message(STATUS "IPO/LTO not enabled.") -endif() - # Check that submodules are present only if source was downloaded with git if (EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/.git" AND NOT EXISTS "${ClickHouse_SOURCE_DIR}/contrib/boost/boost") message (FATAL_ERROR "Submodules are not initialized. Run\n\tgit submodule update --init --recursive") From 6eaba28e012310ec89827529d054f58bfd538955 Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Wed, 28 Oct 2020 22:44:37 +0300 Subject: [PATCH 140/145] Trigger CI --- tests/integration/test_disabled_mysql_server/test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_disabled_mysql_server/test.py b/tests/integration/test_disabled_mysql_server/test.py index df5f123c6bd..a2cbcb17534 100644 --- a/tests/integration/test_disabled_mysql_server/test.py +++ b/tests/integration/test_disabled_mysql_server/test.py @@ -27,7 +27,7 @@ class MySQLNodeInstance: self.port = port self.hostname = hostname self.password = password - self.mysql_connection = None # lazy init + self.mysql_connection = None # lazy init def alloc_connection(self): if self.mysql_connection is None: From 5531bbdc980b32e49bb28f903de7dfad1cbd7f10 Mon Sep 17 00:00:00 2001 From: robot-clickhouse Date: Thu, 29 Oct 2020 01:21:32 +0300 Subject: [PATCH 141/145] Update version_date.tsv after release 20.10.3.30 --- utils/list-versions/version_date.tsv | 1 + 1 file changed, 1 insertion(+) diff --git a/utils/list-versions/version_date.tsv b/utils/list-versions/version_date.tsv index f7d6536a890..3b2681a4ec2 100644 --- a/utils/list-versions/version_date.tsv +++ b/utils/list-versions/version_date.tsv @@ -1,3 +1,4 @@ +v20.10.3.30-stable 2020-10-29 v20.10.2.20-stable 2020-10-23 v20.9.3.45-stable 2020-10-09 v20.9.2.20-stable 2020-09-22 From e094b001d309a38bc887873923949dd9e72c35e0 Mon Sep 17 00:00:00 2001 From: robot-clickhouse Date: Thu, 29 Oct 2020 02:01:30 +0300 Subject: [PATCH 142/145] Update version_date.tsv after release 20.9.4.76 --- utils/list-versions/version_date.tsv | 1 + 1 file changed, 1 insertion(+) diff --git a/utils/list-versions/version_date.tsv b/utils/list-versions/version_date.tsv index 3b2681a4ec2..c58c64e249c 100644 --- a/utils/list-versions/version_date.tsv +++ b/utils/list-versions/version_date.tsv @@ -1,5 +1,6 @@ v20.10.3.30-stable 2020-10-29 v20.10.2.20-stable 2020-10-23 +v20.9.4.76-stable 2020-10-29 v20.9.3.45-stable 2020-10-09 v20.9.2.20-stable 2020-09-22 v20.8.4.11-lts 2020-10-09 From c2ca5d29a85b343d1e2bf558db3b4d07c3d4c2ea Mon Sep 17 00:00:00 2001 From: feng lv Date: Thu, 29 Oct 2020 02:40:39 +0000 Subject: [PATCH 143/145] fix build --- src/Interpreters/InterpreterDropQuery.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Interpreters/InterpreterDropQuery.cpp b/src/Interpreters/InterpreterDropQuery.cpp index 8bf8675b15d..144e045ecee 100644 --- a/src/Interpreters/InterpreterDropQuery.cpp +++ b/src/Interpreters/InterpreterDropQuery.cpp @@ -250,7 +250,7 @@ BlockIO InterpreterDropQuery::executeToDatabase(const ASTDropQuery & query) { if (query.no_delay) { - for (const auto table_uuid : tables_to_wait) + for (const auto & table_uuid : tables_to_wait) waitForTableToBeActuallyDroppedOrDetached(query, database, table_uuid); } throw; @@ -258,7 +258,7 @@ BlockIO InterpreterDropQuery::executeToDatabase(const ASTDropQuery & query) if (query.no_delay) { - for (const auto table_uuid : tables_to_wait) + for (const auto & table_uuid : tables_to_wait) waitForTableToBeActuallyDroppedOrDetached(query, database, table_uuid); } return res; From 745cb4ab2f8d5b04e2b2481a9f9bc585cb583807 Mon Sep 17 00:00:00 2001 From: robot-clickhouse Date: Thu, 29 Oct 2020 09:57:42 +0300 Subject: [PATCH 144/145] Update version_date.tsv after release 20.8.5.45 --- utils/list-versions/version_date.tsv | 1 + 1 file changed, 1 insertion(+) diff --git a/utils/list-versions/version_date.tsv b/utils/list-versions/version_date.tsv index c58c64e249c..37e3e412a63 100644 --- a/utils/list-versions/version_date.tsv +++ b/utils/list-versions/version_date.tsv @@ -3,6 +3,7 @@ v20.10.2.20-stable 2020-10-23 v20.9.4.76-stable 2020-10-29 v20.9.3.45-stable 2020-10-09 v20.9.2.20-stable 2020-09-22 +v20.8.5.45-lts 2020-10-29 v20.8.4.11-lts 2020-10-09 v20.8.3.18-stable 2020-09-18 v20.8.2.3-stable 2020-09-08 From 2a66c17472387c09afc3ac0556b1c4b4a8c90564 Mon Sep 17 00:00:00 2001 From: tavplubix Date: Thu, 29 Oct 2020 16:37:59 +0300 Subject: [PATCH 145/145] Update InterpreterDropQuery.cpp --- src/Interpreters/InterpreterDropQuery.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Interpreters/InterpreterDropQuery.cpp b/src/Interpreters/InterpreterDropQuery.cpp index 8bf8675b15d..144e045ecee 100644 --- a/src/Interpreters/InterpreterDropQuery.cpp +++ b/src/Interpreters/InterpreterDropQuery.cpp @@ -250,7 +250,7 @@ BlockIO InterpreterDropQuery::executeToDatabase(const ASTDropQuery & query) { if (query.no_delay) { - for (const auto table_uuid : tables_to_wait) + for (const auto & table_uuid : tables_to_wait) waitForTableToBeActuallyDroppedOrDetached(query, database, table_uuid); } throw; @@ -258,7 +258,7 @@ BlockIO InterpreterDropQuery::executeToDatabase(const ASTDropQuery & query) if (query.no_delay) { - for (const auto table_uuid : tables_to_wait) + for (const auto & table_uuid : tables_to_wait) waitForTableToBeActuallyDroppedOrDetached(query, database, table_uuid); } return res;