Skip to content

GH-46508: [C++] Upgrade OpenTelemetry cpp to avoid build error on recent Clang#46509

Merged
zanmato1984 merged 28 commits into
apache:mainfrom
zanmato1984:fix/otel-build-on-recent-clang
Jun 2, 2025
Merged

GH-46508: [C++] Upgrade OpenTelemetry cpp to avoid build error on recent Clang#46509
zanmato1984 merged 28 commits into
apache:mainfrom
zanmato1984:fix/otel-build-on-recent-clang

Conversation

@zanmato1984
Copy link
Copy Markdown
Contributor

@zanmato1984 zanmato1984 commented May 20, 2025

Rationale for this change

A warning is introduced in Clang 19.1.0 and AppleClang 17.0.0 that won't be demoted by -Wno-error, causing opentelemetry-cpp build failure.

What changes are included in this PR?

Upgrade opentelemetry-cpp (and opentelemetry-proto correspondingly) to the most recent version, which has addressed this issue in open-telemetry/opentelemetry-cpp#3133.

With this upgrade, we found several false-positives in sanitizers. The reason seems to be that we build bundled third-party dependencies as static libraries and don't instrument their code. This is known to cause false-positives as per https://github.com/google/sanitizers/wiki/threadsanitizercppmanual#non-instrumented-code . So some suppressions and disablements are also made in this PR.

Are these changes tested?

Manually build pass.

Are there any user-facing changes?

None.

@zanmato1984 zanmato1984 requested a review from kou May 20, 2025 04:06
@github-actions
Copy link
Copy Markdown

⚠️ GitHub issue #46508 has been automatically assigned in GitHub to PR creator.

@zanmato1984
Copy link
Copy Markdown
Contributor Author

@github-actions crossbow submit -g cpp

@github-actions
Copy link
Copy Markdown

Revision: 39582b65c86e0f2b613b412d045b4078c90e837b

Submitted crossbow builds: ursacomputing/crossbow @ actions-0137efa559

Task Status
example-cpp-minimal-build-static GitHub Actions
example-cpp-minimal-build-static-system-dependency GitHub Actions
example-cpp-tutorial GitHub Actions
test-alpine-linux-cpp GitHub Actions
test-build-cpp-fuzz GitHub Actions
test-conda-cpp GitHub Actions
test-conda-cpp-meson GitHub Actions
test-conda-cpp-valgrind GitHub Actions
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1 GitHub Actions
test-debian-12-cpp-amd64 GitHub Actions
test-debian-12-cpp-i386 GitHub Actions
test-fedora-39-cpp GitHub Actions
test-ubuntu-22.04-cpp GitHub Actions
test-ubuntu-22.04-cpp-20 GitHub Actions
test-ubuntu-22.04-cpp-bundled GitHub Actions
test-ubuntu-22.04-cpp-emscripten GitHub Actions
test-ubuntu-22.04-cpp-no-threading GitHub Actions
test-ubuntu-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-bundled-offline GitHub Actions
test-ubuntu-24.04-cpp-gcc-13-bundled GitHub Actions
test-ubuntu-24.04-cpp-gcc-14 GitHub Actions
test-ubuntu-24.04-cpp-minimal-with-formats GitHub Actions
test-ubuntu-24.04-cpp-thread-sanitizer GitHub Actions

Comment thread cpp/cmake_modules/ThirdpartyToolchain.cmake Outdated
@github-actions github-actions Bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting review Awaiting review awaiting changes Awaiting changes awaiting change review Awaiting change review labels May 20, 2025
Comment on lines 4854 to 4855
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this with the latest OpenTelemetry C++?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we don't, as long as we all agree with to upgrade.

Please see #46509 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, OK. We can upgrade bundled OpenTelemetry C++ if our CI passes with it. So let's remove this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Also triggered a crossbow build. Let's see the result. Thanks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that recent opentelemetry introduces sanitizer failures, e.g.: https://github.com/ursacomputing/crossbow/actions/runs/15148842343/job/42590903382 https://github.com/apache/arrow/actions/runs/15148814357/job/42590835285?pr=46509

@kou Shall we keep investigating these failures or fallback to the original fix? Thanks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For TSAN I'm not sure if this is the case. I did debug a bit today but still no clue. It's much more difficult than I expected. Plus we have new emerging ASAN errors after suppressing the leak error.

I'll keep looking but this will take quite some longer.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For TSAN I'm not sure if this is the case. I did debug a bit today but still no clue. It's much more difficult than I expected.

How about asking OpenTelemetry C++ developers at https://github.com/open-telemetry/opentelemetry-cpp/discussions ?

Plus we have new emerging ASAN errors after suppressing the leak error.

https://github.com/apache/arrow/actions/runs/15152202496/job/42600231311?pr=46509#step:7:4072

==16624==ERROR: AddressSanitizer: heap-use-after-free on address 0x604000010a18 at pc 0x556f354d1d0f bp 0x7fff3cb44c80 sp 0x7fff3cb44c78
WRITE of size 4 at 0x604000010a18 thread T0
    #0 0x556f354d1d0e in __exchange_and_add /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/ext/atomicity.h:66:12
    #1 0x556f354d1d0e in __exchange_and_add_dispatch /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/ext/atomicity.h:101:14
    #2 0x556f354d1d0e in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr_base.h:165:6
    #3 0x556f354d197e in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr_base.h:705:11
    #4 0x556f35500534 in std::__shared_ptr<opentelemetry::v1::trace::NoopTracer, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr_base.h:1154:31
    #5 0x556f354f84c0 in std::shared_ptr<opentelemetry::v1::trace::NoopTracer>::~shared_ptr() /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:122:11
    #6 0x7f9743791a55 in __cxa_finalize (/lib/x86_64-linux-gnu/libc.so.6+0x45a55) (BuildId: cd410b710f0f094c6832edd95931006d883af48e)
    #7 0x7f974f0a9ac6 in __do_global_dtors_aux crtstuff.c

It seems that this is also related to https://github.com/open-telemetry/opentelemetry-cpp/blob/v1.20.0/sdk/src/trace/tracer.cc#L41-L42 .
I think that we can suppress this too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we can suppress this too.

@kou Do you happen to know how to suppress this one? I did some search but couldn't find en effective way to do it.

How about asking OpenTelemetry C++ developers at open-telemetry/opentelemetry-cpp/discussions ?

Yeah, I'll try to look into it for some more time. If it doesn't work out, I'll ask for help from opentelemetry community.

Thanks for the suggestion.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you try the following?

Add suppressions option for ASAN:

diff --git a/cpp/build-support/run-test.sh b/cpp/build-support/run-test.sh
index 55e3fe0980..f176586a0f 100755
--- a/cpp/build-support/run-test.sh
+++ b/cpp/build-support/run-test.sh
@@ -75,10 +75,9 @@ 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
 
   # Set up suppressions for LeakSanitizer
   LSAN_OPTIONS="$LSAN_OPTIONS suppressions=$ROOT/build-support/lsan-suppressions.txt"

Add cpp/build-support/asan-suppressions.txt:

type:opentelemetry::v1::trace::NoopTracer

See also: https://clang.llvm.org/docs/AddressSanitizer.html

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. ASAN suppressions added, let's see.

@github-actions github-actions Bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels May 20, 2025
@zanmato1984 zanmato1984 force-pushed the fix/otel-build-on-recent-clang branch from 4d7f655 to 0650e71 Compare May 20, 2025 22:04
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 20, 2025
@zanmato1984
Copy link
Copy Markdown
Contributor Author

@github-actions crossbow submit -g cpp

@github-actions
Copy link
Copy Markdown

Revision: ac164b5b6714093bae5833bfe93568b90ed86a9d

Submitted crossbow builds: ursacomputing/crossbow @ actions-90fb7c3af6

Task Status
example-cpp-minimal-build-static GitHub Actions
example-cpp-minimal-build-static-system-dependency GitHub Actions
example-cpp-tutorial GitHub Actions
test-alpine-linux-cpp GitHub Actions
test-build-cpp-fuzz GitHub Actions
test-conda-cpp GitHub Actions
test-conda-cpp-meson GitHub Actions
test-conda-cpp-valgrind GitHub Actions
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1 GitHub Actions
test-debian-12-cpp-amd64 GitHub Actions
test-debian-12-cpp-i386 GitHub Actions
test-fedora-39-cpp GitHub Actions
test-ubuntu-22.04-cpp GitHub Actions
test-ubuntu-22.04-cpp-20 GitHub Actions
test-ubuntu-22.04-cpp-bundled GitHub Actions
test-ubuntu-22.04-cpp-emscripten GitHub Actions
test-ubuntu-22.04-cpp-no-threading GitHub Actions
test-ubuntu-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-bundled-offline GitHub Actions
test-ubuntu-24.04-cpp-gcc-13-bundled GitHub Actions
test-ubuntu-24.04-cpp-gcc-14 GitHub Actions
test-ubuntu-24.04-cpp-minimal-with-formats GitHub Actions
test-ubuntu-24.04-cpp-thread-sanitizer GitHub Actions

@zanmato1984 zanmato1984 changed the title GH-46508: [C++] Suppress OpenTelemetry warning (as error) on recent Clang GH-46508: [C++] Upgrade OpenTelemetry cpp to avoid build error on recent Clang May 20, 2025
@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels May 21, 2025
@zanmato1984 zanmato1984 force-pushed the fix/otel-build-on-recent-clang branch from da1dfaa to 61dae9a Compare June 2, 2025 17:14
@zanmato1984
Copy link
Copy Markdown
Contributor Author

CI is good. I'm merging it.

@zanmato1984 zanmato1984 merged commit 35b9a89 into apache:main Jun 2, 2025
38 of 46 checks passed
@zanmato1984 zanmato1984 removed the awaiting merge Awaiting merge label Jun 2, 2025
@zanmato1984 zanmato1984 deleted the fix/otel-build-on-recent-clang branch June 2, 2025 22:11
@conbench-apache-arrow
Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 35b9a89.

There were 71 benchmark results with an error:

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 17 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants