[llvm][telemetry]Change Telemetry-disabling mechanism. #128534
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Details: - Previously, we used the LLVM_BUILD_TELEMETRY flag to control whether any Telemetry code will be built. This has proven to cause more nuisance to both users of the Telemetry and any further extension of it. (Eg., we needed to put #ifdef around caller/user code) - So the new approach is to: + Remove this flag and introduce LLVM_ENABLE_TELEMETRY which would be true by default. + If LLVM_ENABLE_TELEMETRY is set to FALSE (at buildtime), the library would still be built BUT Telemetry cannot be enabled. And no data can be collected. The benefit of this is that it simplifies user (and extension) code since we just need to put the check on Config::EnableTelemetry. Besides, the Telemetry library itself is very small, hence the additional code to be built would not cause any difference in build performance.
oontvoo
requested review from
rupprecht,
keith and
aaronmondal
as code owners
oontvoo
changed the title
llvmbot
added
cmake
|
@llvm/pr-subscribers-lldb Author: Vy Nguyen (oontvoo) ChangesDetails:
The benefit of this is that it simplifies user (and extension) code since we just need to put the check on Config::EnableTelemetry. Besides, the Telemetry library itself is very small, hence the additional code to be built would not cause any difference in build performance. Full diff: https://github.com/llvm/llvm-project/pull/128534.diff 11 Files Affected:
diff --git a/lldb/source/Core/CMakeLists.txt b/lldb/source/Core/CMakeLists.txt
index 82fb5f42f9f4b..a3c12e4c1bdbc 100644
--- a/lldb/source/Core/CMakeLists.txt
+++ b/lldb/source/Core/CMakeLists.txt
@@ -16,9 +16,7 @@ if (LLDB_ENABLE_CURSES)
endif()
endif()
-if (LLVM_BUILD_TELEMETRY)
- set(TELEMETRY_DEPS Telemetry)
-endif()
+set(TELEMETRY_DEPS Telemetry)
# TODO: Add property `NO_PLUGIN_DEPENDENCIES` to lldbCore
add_lldb_library(lldbCore
diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp
index 5222f76704f91..51a860ea5313b 100644
--- a/lldb/source/Core/Telemetry.cpp
+++ b/lldb/source/Core/Telemetry.cpp
@@ -8,8 +8,6 @@
#include "llvm/Config/llvm-config.h"
-#ifdef LLVM_BUILD_TELEMETRY
-
#include "lldb/Core/Telemetry.h"
#include "lldb/Core/Debugger.h"
#include "lldb/Utility/LLDBLog.h"
@@ -57,7 +55,8 @@ void LLDBBaseTelemetryInfo::serialize(Serializer &serializer) const {
}
TelemetryManager::TelemetryManager(std::unique_ptr<Config> config)
- : m_config(std::move(config)) {}
+ : m_config(std::move(config))
+}
llvm::Error TelemetryManager::preDispatch(TelemetryInfo *entry) {
// Do nothing for now.
@@ -75,5 +74,3 @@ void TelemetryManager::setInstance(std::unique_ptr<TelemetryManager> manager) {
} // namespace telemetry
} // namespace lldb_private
-
-#endif // LLVM_BUILD_TELEMETRY
diff --git a/lldb/unittests/Core/CMakeLists.txt b/lldb/unittests/Core/CMakeLists.txt
index d4d3764b67ae3..e8df299631e2e 100644
--- a/lldb/unittests/Core/CMakeLists.txt
+++ b/lldb/unittests/Core/CMakeLists.txt
@@ -1,6 +1,6 @@
-if (LLVM_BUILD_TELEMETRY)
- set(TELEMETRY_DEPS Telemetry)
-endif()
+
+set(TELEMETRY_DEPS Telemetry)
+
add_lldb_unittest(LLDBCoreTests
CommunicationTest.cpp
diff --git a/llvm/CMakeLists.txt b/llvm/CMakeLists.txt
index 88512d0f1dd96..9188fb93b7846 100644
--- a/llvm/CMakeLists.txt
+++ b/llvm/CMakeLists.txt
@@ -835,7 +835,8 @@ option (LLVM_ENABLE_DOXYGEN "Use doxygen to generate llvm API documentation." OF
option (LLVM_ENABLE_SPHINX "Use Sphinx to generate llvm documentation." OFF)
option (LLVM_ENABLE_OCAMLDOC "Build OCaml bindings documentation." ON)
option (LLVM_ENABLE_BINDINGS "Build bindings." ON)
-option (LLVM_BUILD_TELEMETRY "Build the telemetry library. This does not enable telemetry." ON)
+option (LLVM_BUILD_TELEMETRY "[DEPRECATED - use LLVM_ENABLE_TELEMTRY]Enable the telemetry library. If set to OFF, library cannot be enabled after build (eg., at runtime)" ON)
+option (LLVM_ENABLE_TELEMETRY "Enable the telemetry library. If set to OFF, library cannot be enabled after build (eg., at runtime)" ON)
set(LLVM_INSTALL_DOXYGEN_HTML_DIR "${CMAKE_INSTALL_DOCDIR}/llvm/doxygen-html"
CACHE STRING "Doxygen-generated HTML documentation install directory")
diff --git a/llvm/cmake/modules/LLVMConfig.cmake.in b/llvm/cmake/modules/LLVMConfig.cmake.in
index 28655ee3ab87d..134d107ce79ba 100644
--- a/llvm/cmake/modules/LLVMConfig.cmake.in
+++ b/llvm/cmake/modules/LLVMConfig.cmake.in
@@ -101,6 +101,7 @@ set(LLVM_ENABLE_PIC @LLVM_ENABLE_PIC@)
set(LLVM_BUILD_32_BITS @LLVM_BUILD_32_BITS@)
set(LLVM_BUILD_TELEMETRY @LLVM_BUILD_TELEMETRY@)
+set(LLVM_EANBLE_TELEMETRY @LLVM_ENABLE_TELEMETRY@)
if (NOT "@LLVM_PTHREAD_LIB@" STREQUAL "")
set(LLVM_PTHREAD_LIB "@LLVM_PTHREAD_LIB@")
diff --git a/llvm/include/llvm/Config/llvm-config.h.cmake b/llvm/include/llvm/Config/llvm-config.h.cmake
index 239f9dd3f38db..13db9935870f8 100644
--- a/llvm/include/llvm/Config/llvm-config.h.cmake
+++ b/llvm/include/llvm/Config/llvm-config.h.cmake
@@ -202,6 +202,8 @@
#cmakedefine LLVM_HAS_LOGF128
/* Define if building LLVM with LLVM_BUILD_TELEMETRY */
+/* DEPRECATED - use LLVM_ENABLE_TELEMETRY */
#cmakedefine LLVM_BUILD_TELEMETRY ${LLVM_BUILD_TELEMETRY}
+#cmakedefine LLVM_ENABLE_TELEMETRY ${LLVM_ENABLE_TELEMETRY}
#endif
diff --git a/llvm/include/llvm/Telemetry/Telemetry.h b/llvm/include/llvm/Telemetry/Telemetry.h
index 42319f3ef51f2..f60c4c762ca7b 100644
--- a/llvm/include/llvm/Telemetry/Telemetry.h
+++ b/llvm/include/llvm/Telemetry/Telemetry.h
@@ -64,11 +64,19 @@ class Serializer {
/// This struct can be extended as needed to add additional configuration
/// points specific to a vendor's implementation.
struct Config {
- virtual ~Config() = default;
+#ifdef LLVM_ENABLE_TELEMETRY
+ static bool BuildTimeEnableTelemetry = true;
+#else
+ static bool BuildTimeEnableTelemetry = false;
+#endif
+ virtual ~Config() : EnableTelemetry(BuildTimeEnableTelemetry) {}
// If true, telemetry will be enabled.
const bool EnableTelemetry;
- Config(bool E) : EnableTelemetry(E) {}
+
+ // Telemetry can only be enabled if both the runtime and buildtime flag
+ // are set.
+ Config(bool E) : EnableTelemetry(E && BuildTimeEnableTelemetry) {}
virtual std::optional<std::string> makeSessionId() { return std::nullopt; }
};
diff --git a/llvm/lib/CMakeLists.txt b/llvm/lib/CMakeLists.txt
index d0a2bc9294381..37b6fcf1236e3 100644
--- a/llvm/lib/CMakeLists.txt
+++ b/llvm/lib/CMakeLists.txt
@@ -41,9 +41,8 @@ add_subdirectory(ProfileData)
add_subdirectory(Passes)
add_subdirectory(TargetParser)
add_subdirectory(TextAPI)
-if (LLVM_BUILD_TELEMETRY)
- add_subdirectory(Telemetry)
-endif()
+add_subdirectory(Telemetry)
+
add_subdirectory(ToolDrivers)
add_subdirectory(XRay)
if (LLVM_INCLUDE_TESTS)
diff --git a/llvm/unittests/CMakeLists.txt b/llvm/unittests/CMakeLists.txt
index 12e229b1c3498..81abce51b8939 100644
--- a/llvm/unittests/CMakeLists.txt
+++ b/llvm/unittests/CMakeLists.txt
@@ -63,9 +63,7 @@ add_subdirectory(Support)
add_subdirectory(TableGen)
add_subdirectory(Target)
add_subdirectory(TargetParser)
-if (LLVM_BUILD_TELEMETRY)
- add_subdirectory(Telemetry)
-endif()
+add_subdirectory(Telemetry)
add_subdirectory(Testing)
add_subdirectory(TextAPI)
add_subdirectory(Transforms)
diff --git a/llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn b/llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
index 5a13545a15b13..8f6c431a713af 100644
--- a/llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
+++ b/llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
@@ -293,6 +293,7 @@ write_cmake_config("llvm-config") {
"LLVM_BUILD_LLVM_DYLIB=",
"LLVM_BUILD_SHARED_LIBS=",
"LLVM_BUILD_TELEMETRY=",
+ "LLVM_ENABLE_TELEMETRY=",
"LLVM_DEFAULT_TARGET_TRIPLE=$llvm_target_triple",
"LLVM_ENABLE_DUMP=",
"LLVM_ENABLE_HTTPLIB=",
diff --git a/utils/bazel/llvm_configs/llvm-config.h.cmake b/utils/bazel/llvm_configs/llvm-config.h.cmake
index 239f9dd3f38db..315ddfb37362c 100644
--- a/utils/bazel/llvm_configs/llvm-config.h.cmake
+++ b/utils/bazel/llvm_configs/llvm-config.h.cmake
@@ -202,6 +202,8 @@
#cmakedefine LLVM_HAS_LOGF128
/* Define if building LLVM with LLVM_BUILD_TELEMETRY */
+/* DEPRECATED - Use LLVM_ENABLE_TELEMETRY */
#cmakedefine LLVM_BUILD_TELEMETRY ${LLVM_BUILD_TELEMETRY}
+#cmakedefine LLVM_ENABLE_TELEMETRY ${LLVM_ENABLE_TELEMETRY}
#endif
|
JDevlieghere
approved these changes
Feb 25, 2025
There was a problem hiding this comment.
Thanks @oontvoo. When I introduced the variable I didn't fully consider the implications on the client side, specifically how this introduces the need for a bunch of ifdefs. I think the new approach is worth the trade-off: it significantly simplifies using the telemetry framework while still providing a build time guarantee that no information is collected.
| @@ -202,6 +202,10 @@ | |||
| #cmakedefine LLVM_HAS_LOGF128 | |||
|
|
|||
| /* Define if building LLVM with LLVM_BUILD_TELEMETRY */ | |||
| /* DEPRECATED - use LLVM_ENABLE_TELEMETRY */ | |||
There was a problem hiding this comment.
Do we need to support the old define? I don't see anything else like that in this file. I'd just remove it.
There was a problem hiding this comment.
@rupprecht Hi, do you know if there's any other place (bazel builds?) that need updating? Or can I just remove the flag directly w/o deprecating it? Thanks!
labath
reviewed
Feb 25, 2025
There was a problem hiding this comment.
For me the biggest question is what to do with tests when the setting is turned off, as this makes them run but not succeed. Not running them is one option, but maybe there's a way to disable this in a sufficiently decisive manner while leaving a back door open for tests? (question both for you and @JDevlieghere)
Comment on lines
+24
to
+25
| assert(Config::BuildTimeEnableTelemetry && | ||
| "Telemetry should have been enabled"); |
There was a problem hiding this comment.
Won't this be a problem for unit tests? I see you're enabling them, but I suspect they won't actually succeed with this setting turned off..
There was a problem hiding this comment.
Is there a way to say "if LLVM_ENABLE_TELEMETRY is not set, then skip the tests)?
There was a problem hiding this comment.
P.S: How about this? Adding GTEST_SKIP if the static flag is false.
There was a problem hiding this comment.
That would work. Since this is a build-time condition another possibility would be to disable the test (gtest doesn't run tests whose name starts with DISABLED_ with some preprocessor trickery
#if LLVM_ENABLE_TELEMETRY
#define TELEMETRY_TEST(suite, test) TEST(suite, test)
#else
#define TELEMETRY_TEST(suite, test) TEST(DISABLED_##suite, test)
#end
.. though for just two tests, that doesn't really matter.
jh7370
reviewed
Feb 25, 2025
There was a problem hiding this comment.
I'm fairly ambivalent about the mechanism used to disable telemetry at build time, so no complaints here, though I do wonder what the point is: if there's no concrete implementation of the framework in the build tree, then telemetry is disabled at build time anyway. I suppose there could be downstream cases with a telemetry implementation that want to have the option not to build it at build-time, but in that situation, I don't think we need upstream support. Maybe I'm missing something though?
My only other observation is that we need to be careful about testing, in that if telemetry is disabled at build time, a class of tests become impossible to implement.
Co-authored-by: Pavel Labath <[email protected]>
Co-authored-by: Pavel Labath <[email protected]>
I agree! In current state, the upstream telemetry is mostly empty and doesn't collect anything.
It is disabled but still built. Can we simply not run those tests? |
Co-authored-by: Pavel Labath <[email protected]>
labath
approved these changes
Feb 26, 2025
llvm/CMakeLists.txt
Outdated
| @@ -835,7 +835,8 @@ option (LLVM_ENABLE_DOXYGEN "Use doxygen to generate llvm API documentation." OF | |||
| option (LLVM_ENABLE_SPHINX "Use Sphinx to generate llvm documentation." OFF) | |||
| option (LLVM_ENABLE_OCAMLDOC "Build OCaml bindings documentation." ON) | |||
| option (LLVM_ENABLE_BINDINGS "Build bindings." ON) | |||
| option (LLVM_BUILD_TELEMETRY "Build the telemetry library. This does not enable telemetry." ON) | |||
| option (LLVM_BUILD_TELEMETRY "[DEPRECATED - use LLVM_ENABLE_TELEMTRY]" ON) | |||
There was a problem hiding this comment.
What's up with that? I don't think we usually deprecate options -- we just remove them. cmake will ignore (and warn about) unused/unknown user-specified options on the command line.
Comment on lines
+24
to
+25
| assert(Config::BuildTimeEnableTelemetry && | ||
| "Telemetry should have been enabled"); |
There was a problem hiding this comment.
That would work. Since this is a build-time condition another possibility would be to disable the test (gtest doesn't run tests whose name starts with DISABLED_ with some preprocessor trickery
#if LLVM_ENABLE_TELEMETRY
#define TELEMETRY_TEST(suite, test) TEST(suite, test)
#else
#define TELEMETRY_TEST(suite, test) TEST(DISABLED_##suite, test)
#end
.. though for just two tests, that doesn't really matter.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/11553 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/10441 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/72/builds/8614 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/145/builds/5321 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details:
Previously, we used the LLVM_BUILD_TELEMETRY flag to control whether any Telemetry code will be built. This has proven to cause more nuisance to both users of the Telemetry and any further extension of it. (Eg., we needed to put #ifdef around caller/user code)
So the new approach is to:
The benefit of this is that it simplifies user (and extension) code since we just need to put the check on Config::EnableTelemetry. Besides, the Telemetry library itself is very small, hence the additional code to be built would not cause any difference in build performance.