Merge pull request #50045 from azat/ddl-fix-opentelemetry

Add proper escaping for DDL OpenTelemetry context serialization

src/Common/OpenTelemetryTraceContext.cpp

@@ -5,7 +5,8 @@
 #include <Common/Exception.h>
 #include <base/hex.h>
 #include <Core/Settings.h>
-#include <IO/Operators.h>
+#include <IO/ReadHelpers.h>
+#include <IO/WriteHelpers.h>
 
 #include <Common/AsyncTaskExecutor.h>
 
@@ -249,26 +250,26 @@ String TracingContext::composeTraceparentHeader() const
 
 void TracingContext::deserialize(ReadBuffer & buf)
 {
-    buf >> this->trace_id
+    readUUIDText(trace_id, buf);
-        >> "\n"
+    assertChar('\n', buf);
-        >> this->span_id
+    readIntText(span_id, buf);
-        >> "\n"
+    assertChar('\n', buf);
-        >> this->tracestate
+    readEscapedString(tracestate, buf);
-        >> "\n"
+    assertChar('\n', buf);
-        >> this->trace_flags
+    readIntText(trace_flags, buf);
-        >> "\n";
+    assertChar('\n', buf);
 }
 
 void TracingContext::serialize(WriteBuffer & buf) const
 {
-    buf << this->trace_id
+    writeUUIDText(trace_id, buf);
-        << "\n"
+    writeChar('\n', buf);
-        << this->span_id
+    writeIntText(span_id, buf);
-        << "\n"
+    writeChar('\n', buf);
-        << this->tracestate
+    writeEscapedString(tracestate, buf);
-        << "\n"
+    writeChar('\n', buf);
-        << this->trace_flags
+    writeIntText(trace_flags, buf);
-        << "\n";
+    writeChar('\n', buf);
 }
 
 const TracingContextOnThread & CurrentContext()

tests/queries/0_stateless/02423_ddl_for_opentelemetry.sh

@@ -15,13 +15,15 @@ CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
 # $3 - Query Settings
 function execute_query()
 {
-    # Some queries are supposed to fail, use -f to suppress error messages
+    local trace_id=$1 && shift
-    echo $2 | ${CLICKHOUSE_CURL_COMMAND} -q -s --max-time 180 \
+    local ddl_version=$1 && shift
-                -X POST \
+    local opts=(
-                -H "traceparent: 00-$1-5150000000000515-01" \
+        --opentelemetry-traceparent "00-$trace_id-5150000000000515-01"
-                -H "tracestate: a\nb cd" \
+        --opentelemetry-tracestate $'a\nb cd'
-                "${CLICKHOUSE_URL}&${3}" \
+        --distributed_ddl_output_mode "none"
-                --data @-
+        --distributed_ddl_entry_format_version "$ddl_version"
+    )
+    ${CLICKHOUSE_CLIENT} "${opts[@]}" "$@"
 }
 
 # This function takes following argument:
@@ -82,9 +84,9 @@ for ddl_version in 3 4; do
     echo "===ddl_format_version ${ddl_version}===="
 
     trace_id=$(${CLICKHOUSE_CLIENT} -q "select lower(hex(generateUUIDv4()))");
-    execute_query $trace_id "CREATE TABLE ${CLICKHOUSE_DATABASE}.ddl_test_for_opentelemetry ON CLUSTER ${cluster_name} (id UInt64) Engine=MergeTree ORDER BY id" "distributed_ddl_output_mode=none&distributed_ddl_entry_format_version=${ddl_version}"
+    execute_query $trace_id $ddl_version -q "CREATE TABLE ${CLICKHOUSE_DATABASE}.ddl_test_for_opentelemetry ON CLUSTER ${cluster_name} (id UInt64) Engine=MergeTree ORDER BY id"
 
-    check_span 1 $trace_id "HTTPHandler"
+    check_span 1 $trace_id "TCPHandler"
 
     if [ $cluster_name = "test_shard_localhost" ]; then
         check_span 1 $trace_id "%executeDDLQueryOnCluster%" "attribute['clickhouse.cluster']='${cluster_name}'"
@@ -106,7 +108,7 @@ for ddl_version in 3 4; do
     check_span $expected $trace_id "%DDLWorker::processTask%"
     
     # For queries that tracing are enabled(format version is 4 or Replicated database engine), there should be two 'query' spans,
-    # one is for the HTTPHandler, the other is for the DDL executing in DDLWorker.
+    # one is for the TCPHandler, the other is for the DDL executing in DDLWorker.
     #
     # For other format, there should be only one 'query' span
     if [ $cluster_name = "test_shard_localhost" ]; then
@@ -134,9 +136,9 @@ done
 echo "===exception===="
 
 trace_id=$(${CLICKHOUSE_CLIENT} -q "select lower(hex(generateUUIDv4()))");
-execute_query $trace_id "DROP TABLE ${CLICKHOUSE_DATABASE}.ddl_test_for_opentelemetry_non_exist ON CLUSTER ${cluster_name}" "distributed_ddl_output_mode=none&distributed_ddl_entry_format_version=4" 2>&1| grep -Fv "UNKNOWN_TABLE"
+execute_query $trace_id 4 -q "DROP TABLE ${CLICKHOUSE_DATABASE}.ddl_test_for_opentelemetry_non_exist ON CLUSTER ${cluster_name}" 2>&1 | grep 'DB::Exception ' | grep -Fv "UNKNOWN_TABLE"
 
-check_span 1 $trace_id "HTTPHandler"
+check_span 1 $trace_id "TCPHandler"
 
 if [ $cluster_name = "test_shard_localhost" ]; then
     expected=1
@@ -148,7 +150,7 @@ check_span $expected $trace_id "%executeDDLQueryOnCluster%" "attribute['clickhou
 check_span $expected $trace_id "%DDLWorker::processTask%" "kind = 'CONSUMER'"
 
 if [ $cluster_name = "test_shard_localhost" ]; then
-    # There should be two 'query' spans, one is for the HTTPHandler, the other is for the DDL executing in DDLWorker.
+    # There should be two 'query' spans, one is for the TCPHandler, the other is for the DDL executing in DDLWorker.
     # Both of these two spans contain exception
     expected=2
 else