From 1c73df548d2aa82e7f624642c19b61f6cdc1a246 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Mon, 19 May 2025 19:54:57 -0700 Subject: [PATCH 01/28] Suppress OpenTelemetry warning on recent Clang --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 29dde4071691..df74191fe21e 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -4849,6 +4849,14 @@ macro(build_opentelemetry) version) set(OPENTELEMETRY_BUILD_BYPRODUCTS) set(OPENTELEMETRY_LIBRARIES) + set(OPENTELEMETRY_CXX_FLAGS "${EP_CXX_FLAGS}") + # GH-46508: Supress warnings that won't be demoted by -Wno-error in recent Clang. + if((CMAKE_CXX_COMPILER_ID STREQUAL "Clang" AND + CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL "19.1.0") OR + (CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" AND + CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL "17.0.0")) + string(APPEND OPENTELEMETRY_CXX_FLAGS " -Wno-error=missing-template-arg-list-after-template-kw") + endif() foreach(_OPENTELEMETRY_LIB ${_OPENTELEMETRY_APIS}) add_library(opentelemetry-cpp::${_OPENTELEMETRY_LIB} INTERFACE IMPORTED) @@ -4894,7 +4902,9 @@ macro(build_opentelemetry) endforeach() set(OPENTELEMETRY_CMAKE_ARGS - ${EP_COMMON_CMAKE_ARGS} "-DCMAKE_INSTALL_PREFIX=${OPENTELEMETRY_PREFIX}" + ${EP_COMMON_CMAKE_ARGS} + "-DCMAKE_INSTALL_PREFIX=${OPENTELEMETRY_PREFIX}" + -DCMAKE_CXX_FLAGS=${OPENTELEMETRY_CXX_FLAGS} -DWITH_EXAMPLES=OFF) set(OPENTELEMETRY_PREFIX_PATH_LIST) From 87f52a9e0262f7b725c1848e58a29355da232688 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Mon, 19 May 2025 21:24:06 -0700 Subject: [PATCH 02/28] Fix lint --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index df74191fe21e..c3da742d722d 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -4851,11 +4851,12 @@ macro(build_opentelemetry) set(OPENTELEMETRY_LIBRARIES) set(OPENTELEMETRY_CXX_FLAGS "${EP_CXX_FLAGS}") # GH-46508: Supress warnings that won't be demoted by -Wno-error in recent Clang. - if((CMAKE_CXX_COMPILER_ID STREQUAL "Clang" AND - CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL "19.1.0") OR - (CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" AND - CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL "17.0.0")) - string(APPEND OPENTELEMETRY_CXX_FLAGS " -Wno-error=missing-template-arg-list-after-template-kw") + if((CMAKE_CXX_COMPILER_ID STREQUAL "Clang" AND CMAKE_CXX_COMPILER_VERSION + VERSION_GREATER_EQUAL "19.1.0") + OR (CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" AND CMAKE_CXX_COMPILER_VERSION + VERSION_GREATER_EQUAL "17.0.0")) + string(APPEND OPENTELEMETRY_CXX_FLAGS + " -Wno-error=missing-template-arg-list-after-template-kw") endif() foreach(_OPENTELEMETRY_LIB ${_OPENTELEMETRY_APIS}) @@ -4902,10 +4903,8 @@ macro(build_opentelemetry) endforeach() set(OPENTELEMETRY_CMAKE_ARGS - ${EP_COMMON_CMAKE_ARGS} - "-DCMAKE_INSTALL_PREFIX=${OPENTELEMETRY_PREFIX}" - -DCMAKE_CXX_FLAGS=${OPENTELEMETRY_CXX_FLAGS} - -DWITH_EXAMPLES=OFF) + ${EP_COMMON_CMAKE_ARGS} "-DCMAKE_INSTALL_PREFIX=${OPENTELEMETRY_PREFIX}" + -DCMAKE_CXX_FLAGS=${OPENTELEMETRY_CXX_FLAGS} -DWITH_EXAMPLES=OFF) set(OPENTELEMETRY_PREFIX_PATH_LIST) # Don't specify the DEPENDS unless we actually have dependencies, else From e7c13393ea1bacce68191d39e0c5d6b398063d87 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Mon, 19 May 2025 23:29:05 -0700 Subject: [PATCH 03/28] Use latest opentelemetry-cpp and opentelemetry-proto --- cpp/src/arrow/util/tracing_internal.cc | 4 ++++ cpp/thirdparty/versions.txt | 8 ++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/cpp/src/arrow/util/tracing_internal.cc b/cpp/src/arrow/util/tracing_internal.cc index 5e28bdb461dd..77d145f0ffb5 100644 --- a/cpp/src/arrow/util/tracing_internal.cc +++ b/cpp/src/arrow/util/tracing_internal.cc @@ -93,6 +93,10 @@ class OtlpOStreamExporter final : public sdktrace::SpanExporter { return otel::sdk::common::ExportResult::kSuccess; } + bool ForceFlush(std::chrono::microseconds timeout) noexcept override { + (*out_).flush(); + return true; + } bool Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept override { return exporter_.Shutdown(timeout); diff --git a/cpp/thirdparty/versions.txt b/cpp/thirdparty/versions.txt index bb8dca4a78e3..39852c5b7cf7 100644 --- a/cpp/thirdparty/versions.txt +++ b/cpp/thirdparty/versions.txt @@ -86,10 +86,10 @@ ARROW_MIMALLOC_BUILD_VERSION=v2.0.6 ARROW_MIMALLOC_BUILD_SHA256_CHECKSUM=9f05c94cc2b017ed13698834ac2a3567b6339a8bde27640df5a1581d49d05ce5 ARROW_NLOHMANN_JSON_BUILD_VERSION=v3.12.0 ARROW_NLOHMANN_JSON_BUILD_SHA256_CHECKSUM=4b92eb0c06d10683f7447ce9406cb97cd4b453be18d7279320f7b2f025c10187 -ARROW_OPENTELEMETRY_BUILD_VERSION=v1.13.0 -ARROW_OPENTELEMETRY_BUILD_SHA256_CHECKSUM=7735cc56507149686e6019e06f588317099d4522480be5f38a2a09ec69af1706 -ARROW_OPENTELEMETRY_PROTO_BUILD_VERSION=v0.17.0 -ARROW_OPENTELEMETRY_PROTO_BUILD_SHA256_CHECKSUM=f269fbcb30e17b03caa1decd231ce826e59d7651c0f71c3b28eb5140b4bb5412 +ARROW_OPENTELEMETRY_BUILD_VERSION=v1.20.0 +ARROW_OPENTELEMETRY_BUILD_SHA256_CHECKSUM=4b6eeb852f075133c21b95948017f13a3e21740e55b921d27e42970a47314297 +ARROW_OPENTELEMETRY_PROTO_BUILD_VERSION=v1.6.0 +ARROW_OPENTELEMETRY_PROTO_BUILD_SHA256_CHECKSUM=92682778affe8d00cd36f68308b49295db34fce379bef0a781c50837eccbc3c0 ARROW_ORC_BUILD_VERSION=2.1.2 ARROW_ORC_BUILD_SHA256_CHECKSUM=55451e65dea6ed42afb39fe33a88f9dcea8928dca0a0c9c23ef5545587810b4c ARROW_PROTOBUF_BUILD_VERSION=v21.3 From 16d4330d83ff4fb151bfc69ab90f37de1b5ac15b Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Tue, 20 May 2025 10:02:30 -0700 Subject: [PATCH 04/28] Comment useless argument name --- cpp/src/arrow/util/tracing_internal.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cpp/src/arrow/util/tracing_internal.cc b/cpp/src/arrow/util/tracing_internal.cc index 77d145f0ffb5..347c93d4ad5d 100644 --- a/cpp/src/arrow/util/tracing_internal.cc +++ b/cpp/src/arrow/util/tracing_internal.cc @@ -93,12 +93,11 @@ class OtlpOStreamExporter final : public sdktrace::SpanExporter { return otel::sdk::common::ExportResult::kSuccess; } - bool ForceFlush(std::chrono::microseconds timeout) noexcept override { + bool ForceFlush(std::chrono::microseconds /*timeout*/) noexcept override { (*out_).flush(); return true; } - bool Shutdown(std::chrono::microseconds timeout = - std::chrono::microseconds(0)) noexcept override { + bool Shutdown(std::chrono::microseconds timeout) noexcept override { return exporter_.Shutdown(timeout); } // XXX: OTel 1.19 silent breaking change: this must be overridden From 6c04aaaab19a6ffa1e4fabfd97b63a7ab34d13c7 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Tue, 20 May 2025 15:06:44 -0700 Subject: [PATCH 05/28] Revert the original compiler option tweaks --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index c3da742d722d..29dde4071691 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -4849,15 +4849,6 @@ macro(build_opentelemetry) version) set(OPENTELEMETRY_BUILD_BYPRODUCTS) set(OPENTELEMETRY_LIBRARIES) - set(OPENTELEMETRY_CXX_FLAGS "${EP_CXX_FLAGS}") - # GH-46508: Supress warnings that won't be demoted by -Wno-error in recent Clang. - if((CMAKE_CXX_COMPILER_ID STREQUAL "Clang" AND CMAKE_CXX_COMPILER_VERSION - VERSION_GREATER_EQUAL "19.1.0") - OR (CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" AND CMAKE_CXX_COMPILER_VERSION - VERSION_GREATER_EQUAL "17.0.0")) - string(APPEND OPENTELEMETRY_CXX_FLAGS - " -Wno-error=missing-template-arg-list-after-template-kw") - endif() foreach(_OPENTELEMETRY_LIB ${_OPENTELEMETRY_APIS}) add_library(opentelemetry-cpp::${_OPENTELEMETRY_LIB} INTERFACE IMPORTED) @@ -4904,7 +4895,7 @@ macro(build_opentelemetry) set(OPENTELEMETRY_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} "-DCMAKE_INSTALL_PREFIX=${OPENTELEMETRY_PREFIX}" - -DCMAKE_CXX_FLAGS=${OPENTELEMETRY_CXX_FLAGS} -DWITH_EXAMPLES=OFF) + -DWITH_EXAMPLES=OFF) set(OPENTELEMETRY_PREFIX_PATH_LIST) # Don't specify the DEPENDS unless we actually have dependencies, else From c400d72d4c39935267713cb941bcdc6e6890dd73 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Tue, 20 May 2025 19:34:34 -0700 Subject: [PATCH 06/28] Suppress lsan error for otel --- cpp/build-support/lsan-suppressions.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp/build-support/lsan-suppressions.txt b/cpp/build-support/lsan-suppressions.txt index a8918e10d947..dbcddb291103 100644 --- a/cpp/build-support/lsan-suppressions.txt +++ b/cpp/build-support/lsan-suppressions.txt @@ -26,3 +26,4 @@ leak:CRYPTO_zalloc # without LSAN_OPTIONS=fast_unwind_on_malloc=0:malloc_context_size=100 leak:opentelemetry::v1::context::ThreadLocalContextStorage::GetStack leak:opentelemetry::v1::context::ThreadLocalContextStorage::Stack::Resize +leak:std::make_shared From 1e59a79afc1c2000aa6a9eafee33567f3ed62e2c Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Fri, 23 May 2025 00:09:42 -0700 Subject: [PATCH 07/28] Add asan suppressions for opentelemetry --- cpp/build-support/asan-suppressions.txt | 19 +++++++++++++++++++ cpp/build-support/run-test.sh | 4 ++++ 2 files changed, 23 insertions(+) create mode 100644 cpp/build-support/asan-suppressions.txt diff --git a/cpp/build-support/asan-suppressions.txt b/cpp/build-support/asan-suppressions.txt new file mode 100644 index 000000000000..b0ba30267c9e --- /dev/null +++ b/cpp/build-support/asan-suppressions.txt @@ -0,0 +1,19 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# OpenTelemetry. +type:opentelemetry::v1::trace::NoopTracer diff --git a/cpp/build-support/run-test.sh b/cpp/build-support/run-test.sh index 55e3fe098074..0aa421728119 100755 --- a/cpp/build-support/run-test.sh +++ b/cpp/build-support/run-test.sh @@ -80,6 +80,10 @@ function setup_sanitizers() { # ASAN_OPTIONS="$ASAN_OPTIONS detect_leaks=1" # export ASAN_OPTIONS + # Set up suppressions for AddressSanitizer + ASAN_OPTIONS="$ASAN_OPTIONS suppressions=$ROOT/build-support/asan-suppressions.txt" + export ASAN_OPTIONS + # Set up suppressions for LeakSanitizer LSAN_OPTIONS="$LSAN_OPTIONS suppressions=$ROOT/build-support/lsan-suppressions.txt" export LSAN_OPTIONS From 5c8f259707c8591f408167b74ff8dc2d96418137 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Fri, 23 May 2025 10:11:45 -0700 Subject: [PATCH 08/28] Fix asan suppression parsing --- cpp/build-support/asan-suppressions.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cpp/build-support/asan-suppressions.txt b/cpp/build-support/asan-suppressions.txt index b0ba30267c9e..b8a3473cdf27 100644 --- a/cpp/build-support/asan-suppressions.txt +++ b/cpp/build-support/asan-suppressions.txt @@ -16,4 +16,5 @@ # under the License. # OpenTelemetry. -type:opentelemetry::v1::trace::NoopTracer +heap-use-after-free +fun:*opentelemetry::v1::trace::NoopTracer* From cfc8e99d9e2a7bf49be83f4d7ace81b75e399161 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Fri, 23 May 2025 11:12:22 -0700 Subject: [PATCH 09/28] Try another way to suppress asan error --- cpp/build-support/asan-suppressions.txt | 4 ++-- cpp/build-support/sanitizer-disallowed-entries.txt | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/cpp/build-support/asan-suppressions.txt b/cpp/build-support/asan-suppressions.txt index b8a3473cdf27..45c8282f4b9c 100644 --- a/cpp/build-support/asan-suppressions.txt +++ b/cpp/build-support/asan-suppressions.txt @@ -16,5 +16,5 @@ # under the License. # OpenTelemetry. -heap-use-after-free -fun:*opentelemetry::v1::trace::NoopTracer* +# heap-use-after-free +# fun:*opentelemetry::v1::trace::NoopTracer* diff --git a/cpp/build-support/sanitizer-disallowed-entries.txt b/cpp/build-support/sanitizer-disallowed-entries.txt index 636cfda233ad..fac6ed672377 100644 --- a/cpp/build-support/sanitizer-disallowed-entries.txt +++ b/cpp/build-support/sanitizer-disallowed-entries.txt @@ -23,3 +23,6 @@ fun:*testing*internal*InvokeWith* # Workaround for RapidJSON https://github.com/Tencent/rapidjson/issues/1724 src:*/rapidjson/internal/* + +# Workaround for heap-use-after-free error for OpenTelemetry. +fun:*opentelemetry::v1::trace::NoopTracer* \ No newline at end of file From adb3a947716a1631a1c4f2389c72d6bb66a9fa90 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Fri, 23 May 2025 14:00:21 -0700 Subject: [PATCH 10/28] Try another way to suppress NoopTracer --- cpp/build-support/sanitizer-disallowed-entries.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cpp/build-support/sanitizer-disallowed-entries.txt b/cpp/build-support/sanitizer-disallowed-entries.txt index fac6ed672377..7a4d368fa737 100644 --- a/cpp/build-support/sanitizer-disallowed-entries.txt +++ b/cpp/build-support/sanitizer-disallowed-entries.txt @@ -25,4 +25,5 @@ fun:*testing*internal*InvokeWith* src:*/rapidjson/internal/* # Workaround for heap-use-after-free error for OpenTelemetry. -fun:*opentelemetry::v1::trace::NoopTracer* \ No newline at end of file +# fun:*opentelemetry::v1::trace::NoopTracer* +src:*/sdk/src/trace/tracer.cc From b8a3f6cf0fdcac6b2ec402b61b0eef7ddbba5f7c Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Tue, 27 May 2025 15:55:05 -0700 Subject: [PATCH 11/28] Hack to not flush, see tsan behaves how --- cpp/src/arrow/telemetry/logging.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/src/arrow/telemetry/logging.cc b/cpp/src/arrow/telemetry/logging.cc index 11a70ae1319c..31a53b16aa02 100644 --- a/cpp/src/arrow/telemetry/logging.cc +++ b/cpp/src/arrow/telemetry/logging.cc @@ -229,9 +229,9 @@ class OtelLoggerImpl : public OtelLogger { logger_->EmitLogRecord(std::move(log)); - if (details.severity >= options_.flush_severity) { - util::Logger::Flush(); - } + // if (details.severity >= options_.flush_severity) { + // util::Logger::Flush(); + // } } bool Flush(std::chrono::microseconds timeout) override { From 79e459f0ded791520525f2b436a9e454b7c837fe Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Tue, 27 May 2025 16:48:48 -0700 Subject: [PATCH 12/28] Hack to not use MessageToJsonString, see tsan behaves how --- cpp/src/arrow/telemetry/logging.cc | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/cpp/src/arrow/telemetry/logging.cc b/cpp/src/arrow/telemetry/logging.cc index 31a53b16aa02..daa19fff4ac9 100644 --- a/cpp/src/arrow/telemetry/logging.cc +++ b/cpp/src/arrow/telemetry/logging.cc @@ -71,12 +71,13 @@ class OtlpOStreamLogRecordExporter final : public otel::sdk::logs::LogRecordExpo otel::exporter::otlp::OtlpRecordableUtils::PopulateRequest(records, &request); for (const auto& logs : request.resource_logs()) { - std::string out; - auto status = - google::protobuf::util::MessageToJsonString(logs, &out, pb_json_options_); - if (ARROW_PREDICT_FALSE(!status.ok())) { - return otel::sdk::common::ExportResult::kFailure; - } + std::string out = logs.DebugString(); + // std::string out; + // auto status = + // google::protobuf::util::MessageToJsonString(logs, &out, pb_json_options_); + // if (ARROW_PREDICT_FALSE(!status.ok())) { + // return otel::sdk::common::ExportResult::kFailure; + // } (*sink_) << out << std::endl; } @@ -229,9 +230,9 @@ class OtelLoggerImpl : public OtelLogger { logger_->EmitLogRecord(std::move(log)); - // if (details.severity >= options_.flush_severity) { - // util::Logger::Flush(); - // } + if (details.severity >= options_.flush_severity) { + util::Logger::Flush(); + } } bool Flush(std::chrono::microseconds timeout) override { From 029549c9ac6dc4f756fce860a93be40983c12297 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Wed, 28 May 2025 15:21:46 -0700 Subject: [PATCH 13/28] Another way to eliminate tsan error --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 29dde4071691..fd81bad487be 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -4849,6 +4849,10 @@ macro(build_opentelemetry) version) set(OPENTELEMETRY_BUILD_BYPRODUCTS) set(OPENTELEMETRY_LIBRARIES) + set(OPENTELEMETRY_CXX_FLAGS "${EP_CXX_FLAGS}") + if(ARROW_USE_TSAN) + string(APPEND OPENTELEMETRY_CXX_FLAGS " -fsanitize=thread -DTHREAD_SANITIZE") + endif() foreach(_OPENTELEMETRY_LIB ${_OPENTELEMETRY_APIS}) add_library(opentelemetry-cpp::${_OPENTELEMETRY_LIB} INTERFACE IMPORTED) @@ -4895,7 +4899,7 @@ macro(build_opentelemetry) set(OPENTELEMETRY_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} "-DCMAKE_INSTALL_PREFIX=${OPENTELEMETRY_PREFIX}" - -DWITH_EXAMPLES=OFF) + -DCMAKE_CXX_FLAGS=${OPENTELEMETRY_CXX_FLAGS} -DWITH_EXAMPLES=OFF) set(OPENTELEMETRY_PREFIX_PATH_LIST) # Don't specify the DEPENDS unless we actually have dependencies, else From 542124032cf20ee1a732197953492ddee2666065 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Wed, 28 May 2025 15:23:29 -0700 Subject: [PATCH 14/28] Revert previous hack --- cpp/src/arrow/telemetry/logging.cc | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/cpp/src/arrow/telemetry/logging.cc b/cpp/src/arrow/telemetry/logging.cc index daa19fff4ac9..11a70ae1319c 100644 --- a/cpp/src/arrow/telemetry/logging.cc +++ b/cpp/src/arrow/telemetry/logging.cc @@ -71,13 +71,12 @@ class OtlpOStreamLogRecordExporter final : public otel::sdk::logs::LogRecordExpo otel::exporter::otlp::OtlpRecordableUtils::PopulateRequest(records, &request); for (const auto& logs : request.resource_logs()) { - std::string out = logs.DebugString(); - // std::string out; - // auto status = - // google::protobuf::util::MessageToJsonString(logs, &out, pb_json_options_); - // if (ARROW_PREDICT_FALSE(!status.ok())) { - // return otel::sdk::common::ExportResult::kFailure; - // } + std::string out; + auto status = + google::protobuf::util::MessageToJsonString(logs, &out, pb_json_options_); + if (ARROW_PREDICT_FALSE(!status.ok())) { + return otel::sdk::common::ExportResult::kFailure; + } (*sink_) << out << std::endl; } From 4f6be34bb5ceef2e4460056c23fbc9f1e715ea2c Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Wed, 28 May 2025 16:49:15 -0700 Subject: [PATCH 15/28] Hack to instrument otel for asan --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index fd81bad487be..b0d5c8a1c021 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -4850,8 +4850,11 @@ macro(build_opentelemetry) set(OPENTELEMETRY_BUILD_BYPRODUCTS) set(OPENTELEMETRY_LIBRARIES) set(OPENTELEMETRY_CXX_FLAGS "${EP_CXX_FLAGS}") + if(ARROW_USE_ASAN) + string(APPEND OPENTELEMETRY_CXX_FLAGS " -fsanitize=address -DADDRESS_SANITIZER") + endif() if(ARROW_USE_TSAN) - string(APPEND OPENTELEMETRY_CXX_FLAGS " -fsanitize=thread -DTHREAD_SANITIZE") + string(APPEND OPENTELEMETRY_CXX_FLAGS " -fsanitize=thread -DTHREAD_SANITIZER") endif() foreach(_OPENTELEMETRY_LIB ${_OPENTELEMETRY_APIS}) From ad237e4c13c5d8934cd0223d9c41d75eb69828ac Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Wed, 28 May 2025 17:57:32 -0700 Subject: [PATCH 16/28] Use suppression for tsan --- cpp/build-support/tsan-suppressions.txt | 3 +++ cpp/cmake_modules/ThirdpartyToolchain.cmake | 3 --- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/build-support/tsan-suppressions.txt b/cpp/build-support/tsan-suppressions.txt index ce897c859118..4d97e981d267 100644 --- a/cpp/build-support/tsan-suppressions.txt +++ b/cpp/build-support/tsan-suppressions.txt @@ -17,3 +17,6 @@ # Thread leak in CUDA thread:libcuda.so + +# False-positives in OpenTelemetry because non-instrumented code. +race:^opentelemetry diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index b0d5c8a1c021..a267fa1ef033 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -4853,9 +4853,6 @@ macro(build_opentelemetry) if(ARROW_USE_ASAN) string(APPEND OPENTELEMETRY_CXX_FLAGS " -fsanitize=address -DADDRESS_SANITIZER") endif() - if(ARROW_USE_TSAN) - string(APPEND OPENTELEMETRY_CXX_FLAGS " -fsanitize=thread -DTHREAD_SANITIZER") - endif() foreach(_OPENTELEMETRY_LIB ${_OPENTELEMETRY_APIS}) add_library(opentelemetry-cpp::${_OPENTELEMETRY_LIB} INTERFACE IMPORTED) From 4e56e482809ec089e314d18f507b8f4ed36e4657 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Wed, 28 May 2025 19:35:48 -0700 Subject: [PATCH 17/28] Remove asan hack --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 3 --- 1 file changed, 3 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index a267fa1ef033..a57a1f0123cc 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -4850,9 +4850,6 @@ macro(build_opentelemetry) set(OPENTELEMETRY_BUILD_BYPRODUCTS) set(OPENTELEMETRY_LIBRARIES) set(OPENTELEMETRY_CXX_FLAGS "${EP_CXX_FLAGS}") - if(ARROW_USE_ASAN) - string(APPEND OPENTELEMETRY_CXX_FLAGS " -fsanitize=address -DADDRESS_SANITIZER") - endif() foreach(_OPENTELEMETRY_LIB ${_OPENTELEMETRY_APIS}) add_library(opentelemetry-cpp::${_OPENTELEMETRY_LIB} INTERFACE IMPORTED) From 54004921b65a4b69dc5292f02c806604186c5f75 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Wed, 28 May 2025 20:32:00 -0700 Subject: [PATCH 18/28] Fix asan suppression --- cpp/build-support/asan-suppressions.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cpp/build-support/asan-suppressions.txt b/cpp/build-support/asan-suppressions.txt index 45c8282f4b9c..fc5f89e11f50 100644 --- a/cpp/build-support/asan-suppressions.txt +++ b/cpp/build-support/asan-suppressions.txt @@ -16,5 +16,4 @@ # under the License. # OpenTelemetry. -# heap-use-after-free -# fun:*opentelemetry::v1::trace::NoopTracer* +interceptor_via_fun:*opentelemetry::v1::trace::NoopTracer* \ No newline at end of file From f87904b803234bbab2a7431c045836ca58804543 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Wed, 28 May 2025 20:57:41 -0700 Subject: [PATCH 19/28] Format --- cpp/build-support/tsan-suppressions.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/build-support/tsan-suppressions.txt b/cpp/build-support/tsan-suppressions.txt index 4d97e981d267..fc4a2f6ee1db 100644 --- a/cpp/build-support/tsan-suppressions.txt +++ b/cpp/build-support/tsan-suppressions.txt @@ -18,5 +18,5 @@ # Thread leak in CUDA thread:libcuda.so -# False-positives in OpenTelemetry because non-instrumented code. -race:^opentelemetry +# False-positives in OpenTelemetry because of non-instrumented code. +race:^opentelemetry \ No newline at end of file From 120f82264b9b4914cb1325939e434f25e6b206b4 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Thu, 29 May 2025 20:30:19 -0700 Subject: [PATCH 20/28] Try suppress the entire asan error test --- cpp/src/arrow/telemetry/telemetry_test.cc | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/cpp/src/arrow/telemetry/telemetry_test.cc b/cpp/src/arrow/telemetry/telemetry_test.cc index d9215053822f..1db2f5caf7d5 100644 --- a/cpp/src/arrow/telemetry/telemetry_test.cc +++ b/cpp/src/arrow/telemetry/telemetry_test.cc @@ -94,15 +94,23 @@ class TestLogging : public ::testing::Test { otel::trace::Scope MakeScope() { return tracer_->WithActiveSpan(span_); } + ARROW_DISABLE_UBSAN("address") + void TestBasics() { + auto scope = MakeScope(); + Log(LogLevel::ARROW_ERROR, "foo bar"); + Log(LogLevel::ARROW_WARNING, "baz bal"); + } + protected: otel::trace::Tracer* tracer_; otel::nostd::shared_ptr span_; }; TEST_F(TestLogging, Basics) { - auto scope = MakeScope(); - Log(LogLevel::ARROW_ERROR, "foo bar"); - Log(LogLevel::ARROW_WARNING, "baz bal"); + TestBasics(); + // auto scope = MakeScope(); + // Log(LogLevel::ARROW_ERROR, "foo bar"); + // Log(LogLevel::ARROW_WARNING, "baz bal"); } } // namespace telemetry From 01e0b95286799759eccfccd3ec1b863da996933b Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Thu, 29 May 2025 21:57:34 -0700 Subject: [PATCH 21/28] Try suppress the entire asan error test --- cpp/src/arrow/flight/flight_test.cc | 108 +++++++++++++++------------- 1 file changed, 58 insertions(+), 50 deletions(-) diff --git a/cpp/src/arrow/flight/flight_test.cc b/cpp/src/arrow/flight/flight_test.cc index 863f21f8db5e..0247d5d2c049 100644 --- a/cpp/src/arrow/flight/flight_test.cc +++ b/cpp/src/arrow/flight/flight_test.cc @@ -1729,59 +1729,67 @@ class TestTracing : public ::testing::Test { protected: std::unique_ptr client_; std::unique_ptr server_; -}; #ifdef ARROW_WITH_OPENTELEMETRY -// Must define it ourselves to avoid a linker error -constexpr size_t kSpanIdSize = opentelemetry::trace::SpanId::kSize; -constexpr size_t kTraceIdSize = opentelemetry::trace::TraceId::kSize; - -TEST_F(TestTracing, NoParentTrace) { - ASSERT_OK_AND_ASSIGN(auto results, client_->DoAction(Action{})); - - ASSERT_OK_AND_ASSIGN(auto result, results->Next()); - ASSERT_NE(result, nullptr); - ASSERT_NE(result->body, nullptr); - // Span ID should be a valid span ID, i.e. the server must have started a span - ASSERT_EQ(result->body->size(), kSpanIdSize); - opentelemetry::trace::SpanId span_id({result->body->data(), kSpanIdSize}); - ASSERT_TRUE(span_id.IsValid()); - - ASSERT_OK_AND_ASSIGN(result, results->Next()); - ASSERT_NE(result, nullptr); - ASSERT_NE(result->body, nullptr); - ASSERT_EQ(result->body->size(), kTraceIdSize); - opentelemetry::trace::TraceId trace_id({result->body->data(), kTraceIdSize}); - ASSERT_TRUE(trace_id.IsValid()); -} -TEST_F(TestTracing, WithParentTrace) { - auto* tracer = arrow::internal::tracing::GetTracer(); - auto span = tracer->StartSpan("test"); - auto scope = tracer->WithActiveSpan(span); - - auto span_context = span->GetContext(); - auto current_trace_id = span_context.trace_id().Id(); - - ASSERT_OK_AND_ASSIGN(auto results, client_->DoAction(Action{})); - - ASSERT_OK_AND_ASSIGN(auto result, results->Next()); - ASSERT_NE(result, nullptr); - ASSERT_NE(result->body, nullptr); - ASSERT_EQ(result->body->size(), kSpanIdSize); - opentelemetry::trace::SpanId span_id({result->body->data(), kSpanIdSize}); - ASSERT_TRUE(span_id.IsValid()); + protected: + // Must define it ourselves to avoid a linker error + constexpr static size_t kSpanIdSize = opentelemetry::trace::SpanId::kSize; + constexpr static size_t kTraceIdSize = opentelemetry::trace::TraceId::kSize; + + ARROW_DISABLE_UBSAN("address") + void NoParentTrace() { + ASSERT_OK_AND_ASSIGN(auto results, client_->DoAction(Action{})); + + ASSERT_OK_AND_ASSIGN(auto result, results->Next()); + ASSERT_NE(result, nullptr); + ASSERT_NE(result->body, nullptr); + // Span ID should be a valid span ID, i.e. the server must have started a span + ASSERT_EQ(result->body->size(), kSpanIdSize); + opentelemetry::trace::SpanId span_id({result->body->data(), kSpanIdSize}); + ASSERT_TRUE(span_id.IsValid()); + + ASSERT_OK_AND_ASSIGN(result, results->Next()); + ASSERT_NE(result, nullptr); + ASSERT_NE(result->body, nullptr); + ASSERT_EQ(result->body->size(), kTraceIdSize); + opentelemetry::trace::TraceId trace_id({result->body->data(), kTraceIdSize}); + ASSERT_TRUE(trace_id.IsValid()); + } + ARROW_DISABLE_UBSAN("address") + void WithParentTrace() { + auto* tracer = arrow::internal::tracing::GetTracer(); + auto span = tracer->StartSpan("test"); + auto scope = tracer->WithActiveSpan(span); + + auto span_context = span->GetContext(); + auto current_trace_id = span_context.trace_id().Id(); + + ASSERT_OK_AND_ASSIGN(auto results, client_->DoAction(Action{})); + + ASSERT_OK_AND_ASSIGN(auto result, results->Next()); + ASSERT_NE(result, nullptr); + ASSERT_NE(result->body, nullptr); + ASSERT_EQ(result->body->size(), kSpanIdSize); + opentelemetry::trace::SpanId span_id({result->body->data(), kSpanIdSize}); + ASSERT_TRUE(span_id.IsValid()); + + ASSERT_OK_AND_ASSIGN(result, results->Next()); + ASSERT_NE(result, nullptr); + ASSERT_NE(result->body, nullptr); + ASSERT_EQ(result->body->size(), kTraceIdSize); + opentelemetry::trace::TraceId trace_id({result->body->data(), kTraceIdSize}); + // The server span should have the same trace ID as the client span. + ASSERT_EQ(std::string_view(reinterpret_cast(trace_id.Id().data()), + trace_id.Id().size()), + std::string_view(reinterpret_cast(current_trace_id.data()), + current_trace_id.size())); + } +#endif +}; - ASSERT_OK_AND_ASSIGN(result, results->Next()); - ASSERT_NE(result, nullptr); - ASSERT_NE(result->body, nullptr); - ASSERT_EQ(result->body->size(), kTraceIdSize); - opentelemetry::trace::TraceId trace_id({result->body->data(), kTraceIdSize}); - // The server span should have the same trace ID as the client span. - ASSERT_EQ(std::string_view(reinterpret_cast(trace_id.Id().data()), - trace_id.Id().size()), - std::string_view(reinterpret_cast(current_trace_id.data()), - current_trace_id.size())); -} +#ifdef ARROW_WITH_OPENTELEMETRY +TEST_F(TestTracing, NoParentTrace) { NoParentTrace(); } +TEST_F(TestTracing, WithParentTrace) { WithParentTrace(); } #else TEST_F(TestTracing, NoOp) { // The middleware should not cause any trouble when OTel is not enabled. From a81c208640137aa34f481d5aa37e0dcbe5ac1031 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Fri, 30 May 2025 09:47:28 -0700 Subject: [PATCH 22/28] Resolve --- cpp/src/arrow/util/tracing_internal.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/cpp/src/arrow/util/tracing_internal.cc b/cpp/src/arrow/util/tracing_internal.cc index 347c93d4ad5d..e001c31291bf 100644 --- a/cpp/src/arrow/util/tracing_internal.cc +++ b/cpp/src/arrow/util/tracing_internal.cc @@ -93,10 +93,6 @@ class OtlpOStreamExporter final : public sdktrace::SpanExporter { return otel::sdk::common::ExportResult::kSuccess; } - bool ForceFlush(std::chrono::microseconds /*timeout*/) noexcept override { - (*out_).flush(); - return true; - } bool Shutdown(std::chrono::microseconds timeout) noexcept override { return exporter_.Shutdown(timeout); } From fc588ac7fa4eb312b1a38317b21f71c3723138a7 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Sat, 31 May 2025 17:17:40 -0700 Subject: [PATCH 23/28] Disable false-postive tests under asan --- cpp/src/arrow/flight/flight_test.cc | 116 +++++++++++++--------------- 1 file changed, 54 insertions(+), 62 deletions(-) diff --git a/cpp/src/arrow/flight/flight_test.cc b/cpp/src/arrow/flight/flight_test.cc index 0247d5d2c049..4d4a3ed618c1 100644 --- a/cpp/src/arrow/flight/flight_test.cc +++ b/cpp/src/arrow/flight/flight_test.cc @@ -70,7 +70,7 @@ // > other API headers. This approach efficiently avoids the conflict // > between the two different versions of Abseil. #include "arrow/util/tracing_internal.h" -#ifdef ARROW_WITH_OPENTELEMETRY +#if defined(ARROW_WITH_OPENTELEMETRY) && !defined(ADDRESS_SANITIZER) # include # include # include @@ -95,7 +95,7 @@ const char kAuthHeader[] = "authorization"; class OtelEnvironment : public ::testing::Environment { public: void SetUp() override { -#ifdef ARROW_WITH_OPENTELEMETRY +#if defined(ARROW_WITH_OPENTELEMETRY) && !defined(ADDRESS_SANITIZER) // The default tracer always generates no-op spans which have no // span/trace ID. Set up a different tracer. Note, this needs to be run // before Arrow uses OTel as GetTracer() gets a tracer once and keeps it @@ -1682,7 +1682,7 @@ class TracingTestServer : public FlightServerBase { auto* middleware = reinterpret_cast(call_context.GetMiddleware("tracing")); if (!middleware) return Status::Invalid("Could not find middleware"); -#ifdef ARROW_WITH_OPENTELEMETRY +#if defined(ARROW_WITH_OPENTELEMETRY) && !defined(ADDRESS_SANITIZER) // Ensure the trace context is present (but the value is random so // we cannot assert any particular value) EXPECT_FALSE(middleware->GetTraceContext().empty()); @@ -1729,67 +1729,59 @@ class TestTracing : public ::testing::Test { protected: std::unique_ptr client_; std::unique_ptr server_; - -#ifdef ARROW_WITH_OPENTELEMETRY - protected: - // Must define it ourselves to avoid a linker error - constexpr static size_t kSpanIdSize = opentelemetry::trace::SpanId::kSize; - constexpr static size_t kTraceIdSize = opentelemetry::trace::TraceId::kSize; - - ARROW_DISABLE_UBSAN("address") - void NoParentTrace() { - ASSERT_OK_AND_ASSIGN(auto results, client_->DoAction(Action{})); - - ASSERT_OK_AND_ASSIGN(auto result, results->Next()); - ASSERT_NE(result, nullptr); - ASSERT_NE(result->body, nullptr); - // Span ID should be a valid span ID, i.e. the server must have started a span - ASSERT_EQ(result->body->size(), kSpanIdSize); - opentelemetry::trace::SpanId span_id({result->body->data(), kSpanIdSize}); - ASSERT_TRUE(span_id.IsValid()); - - ASSERT_OK_AND_ASSIGN(result, results->Next()); - ASSERT_NE(result, nullptr); - ASSERT_NE(result->body, nullptr); - ASSERT_EQ(result->body->size(), kTraceIdSize); - opentelemetry::trace::TraceId trace_id({result->body->data(), kTraceIdSize}); - ASSERT_TRUE(trace_id.IsValid()); - } - ARROW_DISABLE_UBSAN("address") - void WithParentTrace() { - auto* tracer = arrow::internal::tracing::GetTracer(); - auto span = tracer->StartSpan("test"); - auto scope = tracer->WithActiveSpan(span); - - auto span_context = span->GetContext(); - auto current_trace_id = span_context.trace_id().Id(); - - ASSERT_OK_AND_ASSIGN(auto results, client_->DoAction(Action{})); - - ASSERT_OK_AND_ASSIGN(auto result, results->Next()); - ASSERT_NE(result, nullptr); - ASSERT_NE(result->body, nullptr); - ASSERT_EQ(result->body->size(), kSpanIdSize); - opentelemetry::trace::SpanId span_id({result->body->data(), kSpanIdSize}); - ASSERT_TRUE(span_id.IsValid()); - - ASSERT_OK_AND_ASSIGN(result, results->Next()); - ASSERT_NE(result, nullptr); - ASSERT_NE(result->body, nullptr); - ASSERT_EQ(result->body->size(), kTraceIdSize); - opentelemetry::trace::TraceId trace_id({result->body->data(), kTraceIdSize}); - // The server span should have the same trace ID as the client span. - ASSERT_EQ(std::string_view(reinterpret_cast(trace_id.Id().data()), - trace_id.Id().size()), - std::string_view(reinterpret_cast(current_trace_id.data()), - current_trace_id.size())); - } -#endif }; -#ifdef ARROW_WITH_OPENTELEMETRY -TEST_F(TestTracing, NoParentTrace) { NoParentTrace(); } -TEST_F(TestTracing, WithParentTrace) { WithParentTrace(); } +#if defined(ARROW_WITH_OPENTELEMETRY) && !defined(ADDRESS_SANITIZER) +// Must define it ourselves to avoid a linker error +constexpr size_t kSpanIdSize = opentelemetry::trace::SpanId::kSize; +constexpr size_t kTraceIdSize = opentelemetry::trace::TraceId::kSize; + +TEST_F(TestTracing, NoParentTrace) { + ASSERT_OK_AND_ASSIGN(auto results, client_->DoAction(Action{})); + + ASSERT_OK_AND_ASSIGN(auto result, results->Next()); + ASSERT_NE(result, nullptr); + ASSERT_NE(result->body, nullptr); + // Span ID should be a valid span ID, i.e. the server must have started a span + ASSERT_EQ(result->body->size(), kSpanIdSize); + opentelemetry::trace::SpanId span_id({result->body->data(), kSpanIdSize}); + ASSERT_TRUE(span_id.IsValid()); + + ASSERT_OK_AND_ASSIGN(result, results->Next()); + ASSERT_NE(result, nullptr); + ASSERT_NE(result->body, nullptr); + ASSERT_EQ(result->body->size(), kTraceIdSize); + opentelemetry::trace::TraceId trace_id({result->body->data(), kTraceIdSize}); + ASSERT_TRUE(trace_id.IsValid()); +} +TEST_F(TestTracing, WithParentTrace) { + auto* tracer = arrow::internal::tracing::GetTracer(); + auto span = tracer->StartSpan("test"); + auto scope = tracer->WithActiveSpan(span); + + auto span_context = span->GetContext(); + auto current_trace_id = span_context.trace_id().Id(); + + ASSERT_OK_AND_ASSIGN(auto results, client_->DoAction(Action{})); + + ASSERT_OK_AND_ASSIGN(auto result, results->Next()); + ASSERT_NE(result, nullptr); + ASSERT_NE(result->body, nullptr); + ASSERT_EQ(result->body->size(), kSpanIdSize); + opentelemetry::trace::SpanId span_id({result->body->data(), kSpanIdSize}); + ASSERT_TRUE(span_id.IsValid()); + + ASSERT_OK_AND_ASSIGN(result, results->Next()); + ASSERT_NE(result, nullptr); + ASSERT_NE(result->body, nullptr); + ASSERT_EQ(result->body->size(), kTraceIdSize); + opentelemetry::trace::TraceId trace_id({result->body->data(), kTraceIdSize}); + // The server span should have the same trace ID as the client span. + ASSERT_EQ(std::string_view(reinterpret_cast(trace_id.Id().data()), + trace_id.Id().size()), + std::string_view(reinterpret_cast(current_trace_id.data()), + current_trace_id.size())); +} #else TEST_F(TestTracing, NoOp) { // The middleware should not cause any trouble when OTel is not enabled. From 6d9f60cad11b0e306afa8a4ce1a22e4012d010dd Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Sun, 1 Jun 2025 21:00:10 -0700 Subject: [PATCH 24/28] Revert useless changes --- cpp/build-support/asan-suppressions.txt | 2 -- cpp/build-support/run-test.sh | 5 ----- cpp/build-support/sanitizer-disallowed-entries.txt | 4 ---- cpp/src/arrow/telemetry/telemetry_test.cc | 14 +++----------- cpp/src/arrow/util/tracing_internal.cc | 3 ++- 5 files changed, 5 insertions(+), 23 deletions(-) diff --git a/cpp/build-support/asan-suppressions.txt b/cpp/build-support/asan-suppressions.txt index fc5f89e11f50..245692337bc3 100644 --- a/cpp/build-support/asan-suppressions.txt +++ b/cpp/build-support/asan-suppressions.txt @@ -15,5 +15,3 @@ # specific language governing permissions and limitations # under the License. -# OpenTelemetry. -interceptor_via_fun:*opentelemetry::v1::trace::NoopTracer* \ No newline at end of file diff --git a/cpp/build-support/run-test.sh b/cpp/build-support/run-test.sh index 0aa421728119..f176586a0fd9 100755 --- a/cpp/build-support/run-test.sh +++ b/cpp/build-support/run-test.sh @@ -75,11 +75,6 @@ function setup_sanitizers() { UBSAN_OPTIONS="$UBSAN_OPTIONS suppressions=$ROOT/build-support/ubsan-suppressions.txt" export UBSAN_OPTIONS - # Enable leak detection even under LLVM 3.4, where it was disabled by default. - # This flag only takes effect when running an ASAN build. - # ASAN_OPTIONS="$ASAN_OPTIONS detect_leaks=1" - # export ASAN_OPTIONS - # Set up suppressions for AddressSanitizer ASAN_OPTIONS="$ASAN_OPTIONS suppressions=$ROOT/build-support/asan-suppressions.txt" export ASAN_OPTIONS diff --git a/cpp/build-support/sanitizer-disallowed-entries.txt b/cpp/build-support/sanitizer-disallowed-entries.txt index 7a4d368fa737..636cfda233ad 100644 --- a/cpp/build-support/sanitizer-disallowed-entries.txt +++ b/cpp/build-support/sanitizer-disallowed-entries.txt @@ -23,7 +23,3 @@ fun:*testing*internal*InvokeWith* # Workaround for RapidJSON https://github.com/Tencent/rapidjson/issues/1724 src:*/rapidjson/internal/* - -# Workaround for heap-use-after-free error for OpenTelemetry. -# fun:*opentelemetry::v1::trace::NoopTracer* -src:*/sdk/src/trace/tracer.cc diff --git a/cpp/src/arrow/telemetry/telemetry_test.cc b/cpp/src/arrow/telemetry/telemetry_test.cc index 1db2f5caf7d5..d9215053822f 100644 --- a/cpp/src/arrow/telemetry/telemetry_test.cc +++ b/cpp/src/arrow/telemetry/telemetry_test.cc @@ -94,23 +94,15 @@ class TestLogging : public ::testing::Test { otel::trace::Scope MakeScope() { return tracer_->WithActiveSpan(span_); } - ARROW_DISABLE_UBSAN("address") - void TestBasics() { - auto scope = MakeScope(); - Log(LogLevel::ARROW_ERROR, "foo bar"); - Log(LogLevel::ARROW_WARNING, "baz bal"); - } - protected: otel::trace::Tracer* tracer_; otel::nostd::shared_ptr span_; }; TEST_F(TestLogging, Basics) { - TestBasics(); - // auto scope = MakeScope(); - // Log(LogLevel::ARROW_ERROR, "foo bar"); - // Log(LogLevel::ARROW_WARNING, "baz bal"); + auto scope = MakeScope(); + Log(LogLevel::ARROW_ERROR, "foo bar"); + Log(LogLevel::ARROW_WARNING, "baz bal"); } } // namespace telemetry diff --git a/cpp/src/arrow/util/tracing_internal.cc b/cpp/src/arrow/util/tracing_internal.cc index e001c31291bf..5e28bdb461dd 100644 --- a/cpp/src/arrow/util/tracing_internal.cc +++ b/cpp/src/arrow/util/tracing_internal.cc @@ -93,7 +93,8 @@ class OtlpOStreamExporter final : public sdktrace::SpanExporter { return otel::sdk::common::ExportResult::kSuccess; } - bool Shutdown(std::chrono::microseconds timeout) noexcept override { + bool Shutdown(std::chrono::microseconds timeout = + std::chrono::microseconds(0)) noexcept override { return exporter_.Shutdown(timeout); } // XXX: OTel 1.19 silent breaking change: this must be overridden From 5908118b8ace4cca534691d8b160fb68cb269a2a Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Sun, 1 Jun 2025 21:06:35 -0700 Subject: [PATCH 25/28] Comment about the reason disabling the tests --- cpp/src/arrow/flight/flight_test.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cpp/src/arrow/flight/flight_test.cc b/cpp/src/arrow/flight/flight_test.cc index 4d4a3ed618c1..16a4909828b0 100644 --- a/cpp/src/arrow/flight/flight_test.cc +++ b/cpp/src/arrow/flight/flight_test.cc @@ -70,6 +70,8 @@ // > other API headers. This approach efficiently avoids the conflict // > between the two different versions of Abseil. #include "arrow/util/tracing_internal.h" +// When running with OTel, ASAN reports false-positives that can't be easily suppressed. +// Disable OTel for ASAN. See GH-46509. #if defined(ARROW_WITH_OPENTELEMETRY) && !defined(ADDRESS_SANITIZER) # include # include @@ -95,6 +97,8 @@ const char kAuthHeader[] = "authorization"; class OtelEnvironment : public ::testing::Environment { public: void SetUp() override { +// When running with OTel, ASAN reports false-positives that can't be easily suppressed. +// Disable OTel for ASAN. See GH-46509. #if defined(ARROW_WITH_OPENTELEMETRY) && !defined(ADDRESS_SANITIZER) // The default tracer always generates no-op spans which have no // span/trace ID. Set up a different tracer. Note, this needs to be run @@ -1682,6 +1686,8 @@ class TracingTestServer : public FlightServerBase { auto* middleware = reinterpret_cast(call_context.GetMiddleware("tracing")); if (!middleware) return Status::Invalid("Could not find middleware"); +// When running with OTel, ASAN reports false-positives that can't be easily suppressed. +// Disable OTel for ASAN. See GH-46509. #if defined(ARROW_WITH_OPENTELEMETRY) && !defined(ADDRESS_SANITIZER) // Ensure the trace context is present (but the value is random so // we cannot assert any particular value) @@ -1731,6 +1737,8 @@ class TestTracing : public ::testing::Test { std::unique_ptr server_; }; +// When running with OTel, ASAN reports false-positives that can't be easily suppressed. +// Disable OTel for ASAN. See GH-46509. #if defined(ARROW_WITH_OPENTELEMETRY) && !defined(ADDRESS_SANITIZER) // Must define it ourselves to avoid a linker error constexpr size_t kSpanIdSize = opentelemetry::trace::SpanId::kSize; From f453cbfcde7432cedf051f960a1ff74bb647412e Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Sun, 1 Jun 2025 21:11:10 -0700 Subject: [PATCH 26/28] Empty line --- cpp/build-support/asan-suppressions.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cpp/build-support/asan-suppressions.txt b/cpp/build-support/asan-suppressions.txt index 245692337bc3..d216be4ddc94 100644 --- a/cpp/build-support/asan-suppressions.txt +++ b/cpp/build-support/asan-suppressions.txt @@ -13,5 +13,4 @@ # "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY # KIND, either express or implied. See the License for the # specific language governing permissions and limitations -# under the License. - +# under the License. \ No newline at end of file From 3b3e9def2bf14938d532925c7db560133fffc34d Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Sun, 1 Jun 2025 21:15:42 -0700 Subject: [PATCH 27/28] Use latest otel-cpp 1.21.0 and otel-proto 1.7.0 --- cpp/thirdparty/versions.txt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/thirdparty/versions.txt b/cpp/thirdparty/versions.txt index 39852c5b7cf7..3a8ad73e1371 100644 --- a/cpp/thirdparty/versions.txt +++ b/cpp/thirdparty/versions.txt @@ -86,10 +86,10 @@ ARROW_MIMALLOC_BUILD_VERSION=v2.0.6 ARROW_MIMALLOC_BUILD_SHA256_CHECKSUM=9f05c94cc2b017ed13698834ac2a3567b6339a8bde27640df5a1581d49d05ce5 ARROW_NLOHMANN_JSON_BUILD_VERSION=v3.12.0 ARROW_NLOHMANN_JSON_BUILD_SHA256_CHECKSUM=4b92eb0c06d10683f7447ce9406cb97cd4b453be18d7279320f7b2f025c10187 -ARROW_OPENTELEMETRY_BUILD_VERSION=v1.20.0 -ARROW_OPENTELEMETRY_BUILD_SHA256_CHECKSUM=4b6eeb852f075133c21b95948017f13a3e21740e55b921d27e42970a47314297 -ARROW_OPENTELEMETRY_PROTO_BUILD_VERSION=v1.6.0 -ARROW_OPENTELEMETRY_PROTO_BUILD_SHA256_CHECKSUM=92682778affe8d00cd36f68308b49295db34fce379bef0a781c50837eccbc3c0 +ARROW_OPENTELEMETRY_BUILD_VERSION=v1.21.0 +ARROW_OPENTELEMETRY_BUILD_SHA256_CHECKSUM=98e5546f577a11b52a57faed1f4cc60d8c1daa44760eba393f43eab5a8ec46a2 +ARROW_OPENTELEMETRY_PROTO_BUILD_VERSION=v1.7.0 +ARROW_OPENTELEMETRY_PROTO_BUILD_SHA256_CHECKSUM=11330d850f5e24d34c4246bc8cb21fcd311e7565d219195713455a576bb11bed ARROW_ORC_BUILD_VERSION=2.1.2 ARROW_ORC_BUILD_SHA256_CHECKSUM=55451e65dea6ed42afb39fe33a88f9dcea8928dca0a0c9c23ef5545587810b4c ARROW_PROTOBUF_BUILD_VERSION=v21.3 From 61dae9af3b9a96aaa7c2e976a36a7e9181c39372 Mon Sep 17 00:00:00 2001 From: Rossi Sun Date: Sun, 1 Jun 2025 23:34:25 -0700 Subject: [PATCH 28/28] Address comments --- cpp/build-support/asan-suppressions.txt | 5 ++++- cpp/cmake_modules/ThirdpartyToolchain.cmake | 3 +-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/cpp/build-support/asan-suppressions.txt b/cpp/build-support/asan-suppressions.txt index d216be4ddc94..553706e045f4 100644 --- a/cpp/build-support/asan-suppressions.txt +++ b/cpp/build-support/asan-suppressions.txt @@ -13,4 +13,7 @@ # "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY # KIND, either express or implied. See the License for the # specific language governing permissions and limitations -# under the License. \ No newline at end of file +# under the License. + +# Note this file is merely a placeholder that contains no suppressions for now. +# But it may become useful in the future. \ No newline at end of file diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index a57a1f0123cc..29dde4071691 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -4849,7 +4849,6 @@ macro(build_opentelemetry) version) set(OPENTELEMETRY_BUILD_BYPRODUCTS) set(OPENTELEMETRY_LIBRARIES) - set(OPENTELEMETRY_CXX_FLAGS "${EP_CXX_FLAGS}") foreach(_OPENTELEMETRY_LIB ${_OPENTELEMETRY_APIS}) add_library(opentelemetry-cpp::${_OPENTELEMETRY_LIB} INTERFACE IMPORTED) @@ -4896,7 +4895,7 @@ macro(build_opentelemetry) set(OPENTELEMETRY_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} "-DCMAKE_INSTALL_PREFIX=${OPENTELEMETRY_PREFIX}" - -DCMAKE_CXX_FLAGS=${OPENTELEMETRY_CXX_FLAGS} -DWITH_EXAMPLES=OFF) + -DWITH_EXAMPLES=OFF) set(OPENTELEMETRY_PREFIX_PATH_LIST) # Don't specify the DEPENDS unless we actually have dependencies, else