Skip to content

[llvm][telemetry]Change Telemetry-disabling mechanism. #128534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Feb 26, 2025
Merged

[llvm][telemetry]Change Telemetry-disabling mechanism. #128534

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
236ab76
[llvm][telemetr]Change Telemetry-disabling mechanism.
oontvoo Feb 24, 2025
45869d7
formatting + more checks
oontvoo Feb 24, 2025
b199854
more clean up
oontvoo Feb 24, 2025
9f5cfcd
Update llvm/include/llvm/Telemetry/Telemetry.h
oontvoo Feb 25, 2025
361a564
Update llvm/include/llvm/Config/llvm-config.h.cmake
oontvoo Feb 25, 2025
013637a
Update llvm/include/llvm/Telemetry/Telemetry.h
oontvoo Feb 25, 2025
409636e
Merge branch 'main' into telemetry_flag
oontvoo Feb 25, 2025
c0a8d47
make field constexpr
oontvoo Feb 25, 2025
19badf7
removed deprecated flag directly
oontvoo Feb 25, 2025
e6fa138
skip test if disabled
oontvoo Feb 25, 2025
785cd8c
Merge branch 'llvm:main' into telemetry_flag
oontvoo Feb 25, 2025
36ffad7
fix build failures
oontvoo Feb 25, 2025
c02bb0c
Update utils/bazel/llvm_configs/llvm-config.h.cmake
oontvoo Feb 26, 2025
e21a8b1
Update llvm/cmake/modules/LLVMConfig.cmake.in
oontvoo Feb 26, 2025
c265467
define test-macro helper to skip tests when telemetry is disabled
oontvoo Feb 26, 2025
9124299
Merge branch 'telemetry_flag' of github.com:oontvoo/llvm-project into…
oontvoo Feb 26, 2025
a72c7db
formatting
oontvoo Feb 26, 2025
60924f7
formatting
oontvoo Feb 26, 2025
bf24522
Merge branch 'main' into telemetry_flag
oontvoo Feb 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions lldb/source/Core/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,6 @@ if (LLDB_ENABLE_CURSES)
endif()
endif()

if (LLVM_BUILD_TELEMETRY)
set(TELEMETRY_DEPS Telemetry)
endif()

# TODO: Add property `NO_PLUGIN_DEPENDENCIES` to lldbCore
add_lldb_library(lldbCore
Address.cpp
Expand Down Expand Up @@ -84,7 +80,7 @@ add_lldb_library(lldbCore
Support
Demangle
TargetParser
${TELEMETRY_DEPS}
Telemetry
)

add_dependencies(lldbCore
Expand Down
13 changes: 5 additions & 8 deletions lldb/source/Core/Telemetry.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,6 @@
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#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"
Expand Down Expand Up @@ -67,13 +62,15 @@ llvm::Error TelemetryManager::preDispatch(TelemetryInfo *entry) {
}

std::unique_ptr<TelemetryManager> TelemetryManager::g_instance = nullptr;
TelemetryManager *TelemetryManager::getInstance() { return g_instance.get(); }
TelemetryManager *TelemetryManager::getInstance() {
if (!Config::BuildTimeEnableTelemetry)
return nullptr;
return g_instance.get();
}

void TelemetryManager::setInstance(std::unique_ptr<TelemetryManager> manager) {
g_instance = std::move(manager);
}

} // namespace telemetry
} // namespace lldb_private

#endif // LLVM_BUILD_TELEMETRY
5 changes: 1 addition & 4 deletions lldb/unittests/Core/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
if (LLVM_BUILD_TELEMETRY)
set(TELEMETRY_DEPS Telemetry)
endif()

add_lldb_unittest(LLDBCoreTests
CommunicationTest.cpp
Expand Down Expand Up @@ -31,5 +28,5 @@ add_lldb_unittest(LLDBCoreTests
LLVMTestingSupport
LINK_COMPONENTS
Support
${TELEMETRY_DEPS}
Telemetry
)
7 changes: 0 additions & 7 deletions lldb/unittests/Core/TelemetryTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,6 @@
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#include "llvm/Config/llvm-config.h"

#ifdef LLVM_BUILD_TELEMETRY

#include "lldb/Core/PluginInterface.h"
#include "lldb/Core/PluginManager.h"
#include "lldb/Core/Telemetry.h"
Expand Down Expand Up @@ -94,5 +89,3 @@ TEST(TelemetryTest, PluginTest) {

ASSERT_EQ("FakeTelemetryPlugin", ins->GetInstanceName());
}

#endif // LLVM_BUILD_TELEMETRY
3 changes: 2 additions & 1 deletion llvm/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

@labath labath Feb 26, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@oontvoo oontvoo Feb 26, 2025

Choose a reason for hiding this comment

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

This was removed.

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")
Expand Down
1 change: 1 addition & 0 deletions llvm/cmake/modules/LLVMConfig.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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@")
Expand Down
4 changes: 4 additions & 0 deletions llvm/include/llvm/Config/llvm-config.h.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,10 @@
#cmakedefine LLVM_HAS_LOGF128

/* Define if building LLVM with LLVM_BUILD_TELEMETRY */
/* DEPRECATED - use LLVM_ENABLE_TELEMETRY */
Copy link
Member

@JDevlieghere JDevlieghere Feb 25, 2025

Choose a reason for hiding this comment

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

Do we need to support the old define? I don't see anything else like that in this file. I'd just remove it.

Copy link
Member Author

@oontvoo oontvoo Feb 25, 2025

Choose a reason for hiding this comment

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

@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!

#cmakedefine LLVM_BUILD_TELEMETRY ${LLVM_BUILD_TELEMETRY}

/* Define if building LLVM with LLVM_ENABLE_TELEMETRY */
#cmakedefine LLVM_ENABLE_TELEMETRY ${LLVM_ENABLE_TELEMETRY}

#endif
12 changes: 10 additions & 2 deletions llvm/include/llvm/Telemetry/Telemetry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 const bool BuildTimeEnableTelemetry = true;
#else
static const 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; }
};
Expand Down
5 changes: 2 additions & 3 deletions llvm/lib/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions llvm/lib/Telemetry/Telemetry.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ void TelemetryInfo::serialize(Serializer &serializer) const {
}

Error Manager::dispatch(TelemetryInfo *Entry) {
assert(Config::BuildTimeEnableTelemetry &&
"Telemetry should have been enabled");
Comment on lines +24 to +25
Copy link
Collaborator

@labath labath Feb 25, 2025

Choose a reason for hiding this comment

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

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..

Copy link
Member Author

@oontvoo oontvoo Feb 25, 2025

Choose a reason for hiding this comment

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

Is there a way to say "if LLVM_ENABLE_TELEMETRY is not set, then skip the tests)?

Copy link
Member Author

@oontvoo oontvoo Feb 25, 2025

Choose a reason for hiding this comment

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

P.S: How about this? Adding GTEST_SKIP if the static flag is false.

Copy link
Collaborator

@labath labath Feb 26, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@oontvoo oontvoo Feb 26, 2025

Choose a reason for hiding this comment

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

done

if (Error Err = preDispatch(Entry))
return Err;

Expand Down
4 changes: 1 addition & 3 deletions llvm/unittests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=",
Expand Down
4 changes: 4 additions & 0 deletions utils/bazel/llvm_configs/llvm-config.h.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,10 @@
#cmakedefine LLVM_HAS_LOGF128

/* Define if building LLVM with LLVM_BUILD_TELEMETRY */
/* DEPRECATED - Use LLVM_ENABLE_TELEMETRY */
#cmakedefine LLVM_BUILD_TELEMETRY ${LLVM_BUILD_TELEMETRY}

/* Define if building LLVM with LLVM_ENABLE_TELEMETRY */
#cmakedefine LLVM_ENABLE_TELEMETRY ${LLVM_ENABLE_TELEMETRY}

#endif
Loading