[lldb][telemetry] Implement LLDB Telemetry (part 1) #119716
Conversation
|
@llvm/pr-subscribers-lldb Author: Vy Nguyen (oontvoo) ChangesDetails:
Note: Please ignore all changes under llvm/.... These will be reverted after the pending LLVM patch is submitted. Patch is 32.79 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/119716.diff 9 Files Affected:
diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h
new file mode 100644
index 00000000000000..241d957672b6ca
--- /dev/null
+++ b/lldb/include/lldb/Core/Telemetry.h
@@ -0,0 +1,309 @@
+//===-- Telemetry.h ----------------------------------------------*- C++
+//-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_CORE_TELEMETRY_H
+#define LLDB_CORE_TELEMETRY_H
+
+#include <atomic>
+#include <chrono>
+#include <ctime>
+#include <memory>
+#include <optional>
+#include <string>
+#include <unordered_map>
+
+#include "lldb/Core/StructuredDataImpl.h"
+#include "lldb/Interpreter/CommandReturnObject.h"
+#include "lldb/Utility/StructuredData.h"
+#include "lldb/lldb-forward.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Telemetry/Telemetry.h"
+
+namespace lldb_private {
+
+using llvm::telemetry::Destination;
+using llvm::telemetry::KindType;
+using llvm::telemetry::Serializer;
+using llvm::telemetry::TelemetryInfo;
+
+struct LldbEntryKind : public ::llvm::telemetry::EntryKind {
+ static const KindType BaseInfo = 0b11000;
+ static const KindType DebuggerInfo = 0b11001;
+ static const KindType TargetInfo = 0b11010;
+ static const KindType ClientInfo = 0b11100;
+ static const KindType CommandInfo = 0b11101;
+ static const KindType MiscInfo = 0b11110;
+};
+
+/// Defines a convenient type for timestamp of various events.
+/// This is used by the EventStats below.
+using SteadyTimePoint = std::chrono::time_point<std::chrono::steady_clock>;
+
+/// Various time (and possibly memory) statistics of an event.
+struct EventStats {
+ // REQUIRED: Start time of an event
+ SteadyTimePoint start;
+ // OPTIONAL: End time of an event - may be empty if not meaningful.
+ std::optional<SteadyTimePoint> end;
+ // TBD: could add some memory stats here too?
+
+ EventStats() = default;
+ EventStats(SteadyTimePoint start) : start(start) {}
+ EventStats(SteadyTimePoint start, SteadyTimePoint end)
+ : start(start), end(end) {}
+};
+
+/// Describes the exit signal of an event.
+struct ExitDescription {
+ int exit_code;
+ std::string description;
+};
+
+struct LldbBaseTelemetryInfo : public TelemetryInfo {
+ EventStats stats;
+
+ // For dyn_cast, isa, etc operations.
+ KindType getKind() const override { return LldbEntryKind::BaseInfo; }
+
+ static bool classof(const TelemetryInfo *t) {
+ if (t == nullptr)
+ return false;
+ // Subclasses of this is also acceptable.
+ return (t->getKind() & LldbEntryKind::BaseInfo) == LldbEntryKind::BaseInfo;
+ }
+
+ void serialize(Serializer &serializer) const override;
+};
+
+struct DebuggerTelemetryInfo : public LldbBaseTelemetryInfo {
+ std::string username;
+ std::string lldb_git_sha;
+ std::string lldb_path;
+ std::string cwd;
+
+ std::optional<ExitDescription> exit_desc;
+ DebuggerTelemetryInfo() = default;
+
+ // Provide a copy ctor because we may need to make a copy before
+ // sanitizing the data.
+ // (The sanitization might differ between different Destination classes).
+ DebuggerTelemetryInfo(const DebuggerTelemetryInfo &other) {
+ username = other.username;
+ lldb_git_sha = other.lldb_git_sha;
+ lldb_path = other.lldb_path;
+ cwd = other.cwd;
+ };
+
+ KindType getKind() const override { return LldbEntryKind::DebuggerInfo; }
+
+ static bool classof(const TelemetryInfo *T) {
+ if (T == nullptr)
+ return false;
+ return T->getKind() == LldbEntryKind::DebuggerInfo;
+ }
+
+ void serialize(Serializer &serializer) const override;
+};
+
+struct TargetTelemetryInfo : public LldbBaseTelemetryInfo {
+ lldb::ModuleSP exec_mod;
+ Target *target_ptr;
+
+ // The same as the executable-module's UUID.
+ std::string target_uuid;
+ std::string file_format;
+
+ std::string binary_path;
+ size_t binary_size;
+
+ std::optional<ExitDescription> exit_desc;
+ TargetTelemetryInfo() = default;
+
+ TargetTelemetryInfo(const TargetTelemetryInfo &other) {
+ exec_mod = other.exec_mod;
+ target_uuid = other.target_uuid;
+ file_format = other.file_format;
+ binary_path = other.binary_path;
+ binary_size = other.binary_size;
+ exit_desc = other.exit_desc;
+ }
+
+ KindType getKind() const override { return LldbEntryKind::TargetInfo; }
+
+ static bool classof(const TelemetryInfo *T) {
+ if (T == nullptr)
+ return false;
+ return T->getKind() == LldbEntryKind::TargetInfo;
+ }
+
+ void serialize(Serializer &serializer) const override;
+};
+
+// Entry from client (eg., SB-API)
+struct ClientTelemetryInfo : public LldbBaseTelemetryInfo {
+ std::string request_name;
+ std::string error_msg;
+
+ ClientTelemetryInfo() = default;
+
+ ClientTelemetryInfo(const ClientTelemetryInfo &other) {
+ request_name = other.request_name;
+ error_msg = other.error_msg;
+ }
+
+ KindType getKind() const override { return LldbEntryKind::ClientInfo; }
+
+ static bool classof(const TelemetryInfo *T) {
+ if (T == nullptr)
+ return false;
+ return T->getKind() == LldbEntryKind::ClientInfo;
+ }
+
+ void serialize(Serializer &serializer) const override;
+};
+
+struct CommandTelemetryInfo : public LldbBaseTelemetryInfo {
+ Target *target_ptr;
+ CommandReturnObject *result;
+
+ // If the command is/can be associated with a target entry,
+ // this field contains that target's UUID.
+ // <EMPTY> otherwise.
+ std::string target_uuid;
+ std::string command_uuid;
+
+ // Eg., "breakpoint set"
+ std::string command_name;
+
+ // !!NOTE!!: The following fields may be omitted due to PII risk.
+ // (Configurable via the telemery::Config struct)
+ std::string original_command;
+ std::string args;
+
+ std::optional<ExitDescription> exit_desc;
+ lldb::ReturnStatus ret_status;
+
+ CommandTelemetryInfo() = default;
+
+ CommandTelemetryInfo(const CommandTelemetryInfo &other) {
+ target_uuid = other.target_uuid;
+ command_uuid = other.command_uuid;
+ command_name = other.command_name;
+ original_command = other.original_command;
+ args = other.args;
+ exit_desc = other.exit_desc;
+ ret_status = other.ret_status;
+ }
+
+ KindType getKind() const override { return LldbEntryKind::CommandInfo; }
+
+ static bool classof(const TelemetryInfo *T) {
+ if (T == nullptr)
+ return false;
+ return T->getKind() == LldbEntryKind::CommandInfo;
+ }
+
+ void serialize(Serializer &serializer) const override;
+};
+
+/// The "catch-all" entry to store a set of custom/non-standard
+/// data.
+struct MiscTelemetryInfo : public LldbBaseTelemetryInfo {
+ /// If the event is/can be associated with a target entry,
+ /// this field contains that target's UUID.
+ /// <EMPTY> otherwise.
+ std::string target_uuid;
+
+ /// Set of key-value pairs for any optional (or impl-specific) data
+ std::map<std::string, std::string> meta_data;
+
+ MiscTelemetryInfo() = default;
+
+ MiscTelemetryInfo(const MiscTelemetryInfo &other) {
+ target_uuid = other.target_uuid;
+ meta_data = other.meta_data;
+ }
+
+ KindType getKind() const override { return LldbEntryKind::MiscInfo; }
+
+ static bool classof(const TelemetryInfo *T) {
+ if (T == nullptr)
+ return false;
+ return T->getKind() == LldbEntryKind::MiscInfo;
+ }
+
+ void serialize(Serializer &serializer) const override;
+};
+
+/// The base Telemetry manager instance in LLDB
+/// This class declares additional instrumentation points
+/// applicable to LLDB.
+class TelemetryManager : public llvm::telemetry::Manager {
+public:
+ /// Creates an instance of TelemetryManager.
+ /// This uses the plugin registry to find an instance:
+ /// - If a vendor supplies a implementation, it will use it.
+ /// - If not, it will either return a no-op instance or a basic
+ /// implementation for testing.
+ ///
+ /// See also lldb_private::TelemetryVendor.
+ static std::unique_ptr<TelemetryManager>
+ CreateInstance(std::unique_ptr<llvm::telemetry::Config> config,
+ Debugger *debugger);
+
+ /// To be invoked upon LLDB startup.
+ virtual void LogStartup(DebuggerTelemetryInfo *entry);
+
+ /// To be invoked upon LLDB exit.
+ virtual void LogExit(DebuggerTelemetryInfo *entry);
+
+ /// To be invoked upon loading the main executable module.
+ /// We log in a fire-n-forget fashion so that if the load
+ /// crashes, we don't lose the entry.
+ virtual void LogMainExecutableLoadStart(TargetTelemetryInfo *entry);
+ virtual void LogMainExecutableLoadEnd(TargetTelemetryInfo *entry);
+
+ /// To be invoked upon process exit.
+ virtual void LogProcessExit(TargetTelemetryInfo *entry);
+
+ /// Invoked for each command
+ /// We log in a fire-n-forget fashion so that if the command execution
+ /// crashes, we don't lose the entry.
+ virtual void LogCommandStart(CommandTelemetryInfo *entry);
+ virtual void LogCommandEnd(CommandTelemetryInfo *entry);
+
+ /// For client (eg., SB API) to send telemetry entries.
+ virtual void
+ LogClientTelemetry(const lldb_private::StructuredDataImpl &entry);
+
+ virtual std::string GetNextUUID() {
+ return std::to_string(uuid_seed.fetch_add(1));
+ }
+
+ llvm::Error dispatch(TelemetryInfo *entry) override;
+ void addDestination(std::unique_ptr<Destination> destination) override;
+
+protected:
+ TelemetryManager(std::unique_ptr<llvm::telemetry::Config> config,
+ Debugger *debugger);
+ TelemetryManager() = default;
+ virtual void CollectMiscBuildInfo();
+
+private:
+ std::atomic<size_t> uuid_seed = 0;
+ std::unique_ptr<llvm::telemetry::Config> m_config;
+ Debugger *m_debugger;
+ const std::string m_session_uuid;
+ std::vector<std::unique_ptr<Destination>> m_destinations;
+};
+
+} // namespace lldb_private
+#endif // LLDB_CORE_TELEMETRY_H
diff --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h
index 938f6e3abe8f2a..8015f42c5ffc8c 100644
--- a/lldb/include/lldb/lldb-enumerations.h
+++ b/lldb/include/lldb/lldb-enumerations.h
@@ -257,8 +257,8 @@ enum StopReason {
};
/// Command Return Status Types.
-enum ReturnStatus {
- eReturnStatusInvalid,
+enum ReturnStatus : int {
+ eReturnStatusInvalid = 0,
eReturnStatusSuccessFinishNoResult,
eReturnStatusSuccessFinishResult,
eReturnStatusSuccessContinuingNoResult,
diff --git a/lldb/source/Core/CMakeLists.txt b/lldb/source/Core/CMakeLists.txt
index dbc620b91b1ed1..4a02f7f1fc85e5 100644
--- a/lldb/source/Core/CMakeLists.txt
+++ b/lldb/source/Core/CMakeLists.txt
@@ -51,6 +51,7 @@ add_lldb_library(lldbCore
Section.cpp
SourceLocationSpec.cpp
SourceManager.cpp
+ Telemetry.cpp
StreamAsynchronousIO.cpp
ThreadedCommunication.cpp
UserSettingsController.cpp
@@ -94,6 +95,7 @@ add_lldb_library(lldbCore
Support
Demangle
TargetParser
+ Telemetry
)
add_dependencies(lldbCore
diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp
new file mode 100644
index 00000000000000..5ddad030ef962e
--- /dev/null
+++ b/lldb/source/Core/Telemetry.cpp
@@ -0,0 +1,338 @@
+
+//===-- Telemetry.cpp -----------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#include "lldb/Core/Telemetry.h"
+
+#include <chrono>
+#include <cstdlib>
+#include <ctime>
+#include <fstream>
+#include <memory>
+#include <string>
+#include <typeinfo>
+#include <utility>
+#include <vector>
+
+#include "lldb/API/SBDebugger.h"
+#include "lldb/API/SBProcess.h"
+#include "lldb/Core/Debugger.h"
+#include "lldb/Core/Module.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Interpreter/CommandInterpreter.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Statistics.h"
+#include "lldb/Utility/ConstString.h"
+#include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/LLDBLog.h"
+#include "lldb/Utility/UUID.h"
+#include "lldb/Version/Version.h"
+#include "lldb/lldb-enumerations.h"
+#include "lldb/lldb-forward.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/LineIterator.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/RandomNumberGenerator.h"
+#include "llvm/Support/raw_ostream.h"
+#include "llvm/Telemetry/Telemetry.h"
+
+namespace lldb_private {
+
+using ::llvm::Error;
+using ::llvm::telemetry::Destination;
+using ::llvm::telemetry::TelemetryInfo;
+
+static size_t ToNanosecOrZero(const std::optional<SteadyTimePoint> &Point) {
+ if (!Point.has_value())
+ return 0;
+
+ return Point.value().time_since_epoch().count();
+}
+
+void LldbBaseTelemetryInfo::serialize(Serializer &serializer) const {
+ serializer.writeInt32("EntryKind", getKind());
+ serializer.writeString("SessionId", SessionId);
+}
+
+void DebuggerTelemetryInfo::serialize(Serializer &serializer) const {
+ LldbBaseTelemetryInfo::serialize(serializer);
+ serializer.writeString("username", username);
+ serializer.writeString("lldb_path", lldb_path);
+ serializer.writeString("cwd", cwd);
+ serializer.writeSizeT("start", stats.start.time_since_epoch().count());
+ serializer.writeSizeT("end", ToNanosecOrZero(stats.end));
+}
+
+void ClientTelemetryInfo::serialize(Serializer &serializer) const {
+ LldbBaseTelemetryInfo::serialize(serializer);
+ serializer.writeString("request_name", request_name);
+ serializer.writeString("error_msg", error_msg);
+ serializer.writeSizeT("start", stats.start.time_since_epoch().count());
+ serializer.writeSizeT("end", ToNanosecOrZero(stats.end));
+}
+
+void TargetTelemetryInfo::serialize(Serializer &serializer) const {
+ LldbBaseTelemetryInfo::serialize(serializer);
+ serializer.writeString("target_uuid", target_uuid);
+ serializer.writeString("binary_path", binary_path);
+ serializer.writeSizeT("binary_size", binary_size);
+}
+
+void CommandTelemetryInfo::serialize(Serializer &serializer) const {
+ LldbBaseTelemetryInfo::serialize(serializer);
+ serializer.writeString("target_uuid", target_uuid);
+ serializer.writeString("command_uuid", command_uuid);
+ serializer.writeString("args", args);
+ serializer.writeString("original_command", original_command);
+ serializer.writeSizeT("start", stats.start.time_since_epoch().count());
+ serializer.writeSizeT("end", ToNanosecOrZero(stats.end));
+
+ // If this entry was emitted at the end of the command-execution,
+ // then calculate the runtime too.
+ if (stats.end.has_value()) {
+ serializer.writeSizeT("command_runtime",
+ (stats.end.value() - stats.start).count());
+ if (exit_desc.has_value()) {
+ serializer.writeInt32("exit_code", exit_desc->exit_code);
+ serializer.writeString("exit_msg", exit_desc->description);
+ serializer.writeInt32("return_status", static_cast<int>(ret_status));
+ }
+ }
+}
+
+void MiscTelemetryInfo::serialize(Serializer &serializer) const {
+ LldbBaseTelemetryInfo::serialize(serializer);
+ serializer.writeString("target_uuid", target_uuid);
+ serializer.writeKeyValueMap("meta_data", meta_data);
+}
+
+static std::string MakeUUID(lldb_private::Debugger *debugger) {
+ std::string ret;
+ uint8_t random_bytes[16];
+ if (auto ec = llvm::getRandomBytes(random_bytes, 16)) {
+ LLDB_LOG(GetLog(LLDBLog::Object),
+ "Failed to generate random bytes for UUID: {0}", ec.message());
+ // fallback to using timestamp + debugger ID.
+ ret = std::to_string(
+ std::chrono::steady_clock::now().time_since_epoch().count()) +
+ "_" + std::to_string(debugger->GetID());
+ } else {
+ ret = lldb_private::UUID(random_bytes).GetAsString();
+ }
+
+ return ret;
+}
+
+TelemetryManager::TelemetryManager(
+ std::unique_ptr<llvm::telemetry::Config> config,
+ lldb_private::Debugger *debugger)
+ : m_config(std::move(config)), m_debugger(debugger),
+ m_session_uuid(MakeUUID(debugger)) {}
+
+std::unique_ptr<TelemetryManager> TelemetryManager::CreateInstance(
+ std::unique_ptr<llvm::telemetry::Config> config,
+ lldb_private::Debugger *debugger) {
+
+ TelemetryManager *ins = new TelemetryManager(std::move(config), debugger);
+
+ return std::unique_ptr<TelemetryManager>(ins);
+}
+
+llvm::Error TelemetryManager::dispatch(TelemetryInfo *entry) {
+ entry->SessionId = m_session_uuid;
+
+ for (auto &destination : m_destinations) {
+ llvm::Error err = destination->receiveEntry(entry);
+ if (err) {
+ return std::move(err);
+ }
+ }
+ return Error::success();
+}
+
+void TelemetryManager::addDestination(
+ std::unique_ptr<Destination> destination) {
+ m_destinations.push_back(std::move(destination));
+}
+
+void TelemetryManager::LogStartup(DebuggerTelemetryInfo *entry) {
+ UserIDResolver &resolver = lldb_private::HostInfo::GetUserIDResolver();
+ std::optional<llvm::StringRef> opt_username =
+ resolver.GetUserName(lldb_private::HostInfo::GetUserID());
+ if (opt_username)
+ entry->username = *opt_username;
+
+ entry->lldb_git_sha =
+ lldb_private::GetVersion(); // TODO: find the real git sha?
+
+ llvm::SmallString<64> cwd;
+ if (!llvm::sys::fs::current_path(cwd)) {
+ entry->cwd = cwd.c_str();
+ } else {
+ MiscTelemetryInfo misc_info;
+ misc_info.meta_data["internal_errors"] = "Cannot determine CWD";
+ if (auto er = dispatch(&misc_info)) {
+ LLDB_LOG(GetLog(LLDBLog::Object),
+ "Failed to dispatch misc-info from startup");
+ }
+ }
+
+ if (auto er = dispatch(entry)) {
+ LLDB_LOG(GetLog(LLDBLog::Object), "Failed to dispatch entry from startup");
+ }
+
+ // Optional part
+ CollectMiscBuildInfo();
+}
+
+void TelemetryManager::LogExit(DebuggerTelemetryInfo *entry) {
+ if (auto *selected_target =
+ m_debugger->GetSelectedExecutionContext().GetTargetPtr()) {
+ if (!selected_target->IsDummyTarget()) {
+ const lldb::ProcessSP proc = selected_target->GetProcessSP();
+ if (proc == nullptr) {
+ // no process has been launched yet.
+ entry->exit_desc = {-1, "no process launched."};
+ } else {
+ entry->exit_desc = {proc->GetExitStatus(), ""};
+ if (const char *description = proc->GetExitDescription())
+ entry->exit_desc->description = std::string(description);
+ }
+ }
+ }
+ dispatch(entry);
+}
+
+void TelemetryManager::LogProcessExit(TargetTelemetryInfo *entry) {
+ entry->target_uuid =
+ entry->target_ptr && !entry->target_ptr->IsDummyTarget()
+ ? entry->target_ptr->GetExecutableModule()->GetUUID().GetAsString()
+ : "";
+
+ dispatch(entry);
+}
+
+void TelemetryManager::CollectMiscBuildInfo() {
+ // collecting use-case specific data
+}
+
+void TelemetryManager::LogMainExecutableLoadStart(TargetTelemetryInfo *entry) {
+ entry->binary_path =
+ entry->exec_mod->GetFileSpec().GetPathAsConstString().GetCString();
+ entry->file_format = entry->exec_mod->GetArchitecture().GetArchitectureName();
+ entry->target_uuid = entry->exec_mod->GetUUID().GetAsString();
+ if (auto err = llvm::sys::fs::file_size(
+ entry->exec_mod->GetFileSpec().GetPath(), entry->binary_size)) {
+ // If there was error obtaining it, just reset the size to 0.
+ // Maybe log the error too?
+ entry->binary_size = 0;
+ }
+ dispatch(entry);
+}
+
+void TelemetryManager::LogMainExecutableLoadEnd(TargetTelemetryInfo *entry) {
+ lldb::ModuleSP exec_mod = entry->exec_mod;
+ entry->binary_path =
+ exec_mod->GetFileSpec().GetPathAsConstString().GetCString();
+ entry->file_format = exec_mod->GetArchitecture().GetArchitectureName();
+ entry->target_uuid = exec_mod->GetUUID().GetAsString();
+ entry->binary_size = exec_mod->GetObjectFile()->GetByteSize();
+
+ dispatch(entry);
+
+ // Collect some more info, might be useful?
+ MiscTelemetryInfo misc_info;
+ misc_info.target_uuid = exec_mod->GetUUID().GetAsString();
+ misc_info.m...
[truncated]
|
There was a problem hiding this comment.
Pavel's suggestion was to split up the patch to make reviewing easier
It was, but this wasn't the kind of separation I had in mind. For me as least, it's very hard to review features in these "horizontal" slices. It's also unclear how (if ever) will this code be tested.
I think it be much better to slice this up vertically, so that (e.g.) the CommandTelemetryInfo was added in the same patch that was actually using it.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
I can split the usage changes to different patches (because they'd be changing differnt files), but spliting the library into different patches by "verticals" seems unnecessarily clustering to me (esp. they're touching the same file, which makes it annoying to resolve conflicts) |
You can test this locally with the following command:git-clang-format --diff 36c34ec967c28c77406fe85ef3237a167a243763 23dd58e37826a1ba300b7eb887104c46ab92c0f9 --extensions cpp,h -- lldb/include/lldb/Core/Telemetry.h lldb/source/Core/Telemetry.cpp llvm/include/llvm/Telemetry/Telemetry.h llvm/lib/Telemetry/Telemetry.cpp lldb/include/lldb/lldb-enumerations.hView the diff from clang-format here.diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp
index 17d677f5387..66ec9ee16d2 100644
--- a/lldb/source/Core/Telemetry.cpp
+++ b/lldb/source/Core/Telemetry.cpp
@@ -145,7 +145,8 @@ std::unique_ptr<TelemetryManager> TelemetryManager::CreateInstance(
std::unique_ptr<llvm::telemetry::Config> config,
lldb_private::Debugger *debugger) {
- return std::unique_ptr<TelemetryManager>(new TelemetryManager(std::move(config), debugger));
+ return std::unique_ptr<TelemetryManager>(
+ new TelemetryManager(std::move(config), debugger));
}
llvm::Error TelemetryManager::dispatch(TelemetryInfo *entry) {
|
P.S: I've updated this patch to remove all of the additional base classes of TelemtryInfo, keeping only the LldbBaseTelemtryInfo. Similarly, removed most of the PTAL. |
| enum ReturnStatus { | ||
| eReturnStatusInvalid, | ||
| enum ReturnStatus : int { | ||
| eReturnStatusInvalid = 0, |
There was a problem hiding this comment.
carry-over comment from previous patch("Why is this needed?")
This is needed to serialize the ReturnStatus object
There was a problem hiding this comment.
I'm not sure I understand that. What exactly happens if you leave this out?
There was a problem hiding this comment.
when i tried to do json::Object({"ret_stat", returnStatus}), I got some build error (ambiguous types or sth like that).
There was a problem hiding this comment.
In that case, I'd suggest working around that locally (by casting the enum to an integer type?). It's kinda weird that this is the only enum which has an explicit type specifier.
There was a problem hiding this comment.
done - removed the :int and =0
There was a problem hiding this comment.
@JDevlieghere, it looks like the llvm part of this is going to land imminently, so maybe this is a good time to take another look at this. In particular, I expect you'll want to say something about the use of the string "lldb" in identifier names.
lldb/include/lldb/Core/Telemetry.h
Outdated
|
|
||
| private: | ||
| std::unique_ptr<llvm::telemetry::Config> m_config; | ||
| Debugger *m_debugger; |
There was a problem hiding this comment.
I thought that we've (in our conversation at the dev meeting) concluded that the telemetry manager should not be tied to a debugger (because some of the pieces of code -- e.g. everything inside a Symbol/ObjectFile class -- does not have access to debugger instance -- by design). Has that changed? If not, then this member shouldn't be here (but it could be attached to event types which can be tied to a specific debugger connection).
In the same breath, I want to make sure you're aware of the effort in this RFC, whose result will be that a single process can end up serving multiple DAP connections (with one Debugger instance for each connection) -- which means that some events may not be able to be tied to a specific DAP connection either.
I'm not really pushing for any particular solution, but I want to make sure you make a conscious decision about this, as it can have big implications for the rest of the implementation. It's basically a tradeoff. If you make the telemetry instance specific to a debugger object, then you cannot send telemetry from parts of the code which do not have access to a debugger. Or you have to do something similar to the Progress events, which send progress reports to all active debuggers. If you don't tie this to a debugger, then you can send telemetry from ~anywhere, but well.. that telemetry will not be tied to a debugger..
There was a problem hiding this comment.
Sorry, this wasn't supposed to be here. :) I've removed it from the Manager class.
The idea is to have each of the sub-classes of TelemetryInfo carry a Debugger pointer, if applicable.
lldb/source/Core/Telemetry.cpp
Outdated
| @@ -0,0 +1,95 @@ | |||
|
|
|||
lldb/source/Core/Telemetry.cpp
Outdated
| using ::llvm::telemetry::Destination; | ||
| using ::llvm::telemetry::TelemetryInfo; | ||
|
|
||
| static unsigned long long ToNanosec(const SteadyTimePoint Point) { |
There was a problem hiding this comment.
unsigned long long is also an unusual type for the number of nanoseconds. The reason for implementing ~all integral types in the generic implementation was so that you can use ~any type here, and things will just work. I'd use uint64_t here, but you can also go with std::chrono::nanoseconds::rep if you want to be fancy.
lldb/source/Core/Telemetry.cpp
Outdated
| if (auto err = destination->receiveEntry(entry)) | ||
| deferredErrs = llvm::joinErrors(std::move(deferredErrs), std::move(err)); |
There was a problem hiding this comment.
| if (auto err = destination->receiveEntry(entry)) | |
| deferredErrs = llvm::joinErrors(std::move(deferredErrs), std::move(err)); | |
| deferredErrs = llvm::joinErrors(std::move(deferredErrs), destination->receiveEntry(entry)); |
lldb/test/CMakeLists.txt
Outdated
| # Enable Telemetry for testing. | ||
| target_compile_definitions(lldb PRIVATE -DTEST_TELEMETRY) | ||
|
|
There was a problem hiding this comment.
This doesn't do what you think it does. It builds the lldb binary with the macro definition if LLDB_INCLUDE_TESTS is defined (because that's the condition that guards cmake entering this directory).
(Or, to put it differently target_compile_definitions always defines a macro for the target it gets as an argument, regardless of where the macro is called from)
Thanks. I think this is better because there's no shortage of things to discuss with the general approach as well. It's true that the vertical slices can cause more merge conflicts, but they should be simple to resolve, and a very big plus is that you can do the reviews (the true bottleneck most of the time) in parallel. |
oontvoo
force-pushed
the
lldb_temetry_s1
branch
from
64b616c to
dbae717
Compare
This contains only the concrete implementation of the framework to be used but no usages yet. This is a subset of PR/98528. I plan to send a few follow-up patches: part2 : includes changes in the plugin-manager to set up the plugin stuff (ie., how to create a default vs vendor impl) part3 (all of the following can be done in parallel): * part 3_a: define DebuggerTelemetryInfo and related methods to collect data about debugger startup/exit * part 3_b: define TargetTelemetryInfo and related methods to collect data about debug target(s) * part 3_c: define CommandTelemetryInfo and related methods to collect data about debug-commands * part 3_d: define ClientTelemtryInfo and related methods to collect data about lldb-dap/any other client
|
(accidentally closed the PR - reopening now) |
|
@labath Hi, I've updated the patch to sync up with the changes from llvm::Telemetry. (Aslo addressed all the open-questions.) Please have another look when you can. Thanks! P.S Looks like github is slow to show the newer commits in the PR. I can see my latest commits here (https://github.com/oontvoo/llvm-project/tree/lldb_temetry_s1) but it's not reflected in this PR diff ... 🤔 |
lldb/include/lldb/Core/Telemetry.h
Outdated
|
|
||
| namespace lldb_private { | ||
|
|
||
| using llvm::telemetry::Destination; |
There was a problem hiding this comment.
This defines a type (alias) called lldb_private::Destination, which is way too generic. If you want to use names like this, then I suggest creating a sub-namespace.
There was a problem hiding this comment.
Ok, I'll just use the full name then - maybe that's better than creating aliases ...
There was a problem hiding this comment.
Maybe, but if you don't want a namespace, then you should be also be careful about other names that you create. For example, lldb_private::LldbEntryKind is not very telling (it doesn't tell you that it has anything to do with telemetry, the string "lldb" in the name is redundant. In this setup I'd probably go with lldb_private::TelemetryEntryKind. Same goes for EventStats which sounds like it has something to do with the Event class in Utility/Event.h (it doesn't) and SteadyTimePoint, which could be generic, but it's suprising to see it defined in Telemetry.h. So, TelemetryEventStats and TelemetryTimePoint ?
You don't have to do that, but after writing all this I'm starting to incline towards putting everything in a namespace.. Then the names can stay pretty much as they are?
There was a problem hiding this comment.
So putting everthng under lldb_private::telemetry?
There was a problem hiding this comment.
Yes, that's what I'd do. Particularly given that this code is going to be conditionally compiled, I think it'd be nice to isolate it as much as possible.
| enum ReturnStatus { | ||
| eReturnStatusInvalid, | ||
| enum ReturnStatus : int { | ||
| eReturnStatusInvalid = 0, |
There was a problem hiding this comment.
I'm not sure I understand that. What exactly happens if you leave this out?
lldb/source/Core/Telemetry.cpp
Outdated
| ret = std::to_string( | ||
| std::chrono::steady_clock::now().time_since_epoch().count()) + | ||
| "_" + std::to_string(debugger->GetID()); |
There was a problem hiding this comment.
| ret = std::to_string( | |
| std::chrono::steady_clock::now().time_since_epoch().count()) + | |
| "_" + std::to_string(debugger->GetID()); | |
| ret = llvm::formatv("{0}_{1}", | |
| std::chrono::steady_clock::now().time_since_epoch().count()), debugger->GetID()); |
- use full names rather than alias - use formatv
Seems to be back to normal now! :) |
There was a problem hiding this comment.
I think this looks okay so far. @JDevlieghere, do you have anything to add?
The interesting part will be wiring this up with initialization and all the call sites (which is now complicated by the fact that this code is conditionally compiled.
Before you get too much into the plugin manager, let me say that I don't think it makes sense emulating the "normal" plugin setup for this functionality. It's true that this is meant to be pluggable in a way, so calling it a plugin sounds reasonable. However, it's a very different kind of a plugin from all of our other plugins. The complexity of those plugins comes from the fact that we have multiple implementations (classes) of those plugins, which can create an arbitrary number of instances (objects) of that class, at arbitrary points in time (e.g. each time we open an object file).
Pretty much none of this applies here. As I understand it, we're going to have at most one implementation of the plugin (at most one active instance at least), this implementation is going to have at most one instance (since we're sharing it between debugger objects), and we're going to be creating it directly at startup.
With that in mind, I think it'd be sufficient to have something like TelemetryManager::SetInstance(), and have that be called from the Initialize() static method of the plugin. (None of the fancy CreateInstance methods, callbacks, etc.)
| enum ReturnStatus { | ||
| eReturnStatusInvalid, | ||
| enum ReturnStatus : int { | ||
| eReturnStatusInvalid = 0, |
There was a problem hiding this comment.
In that case, I'd suggest working around that locally (by casting the enum to an integer type?). It's kinda weird that this is the only enum which has an explicit type specifier.
lldb/include/lldb/Core/Telemetry.h
Outdated
| struct LldbBaseTelemetryInfo : public llvm::telemetry::TelemetryInfo { | ||
| EventStats stats; | ||
|
|
||
| std::optional<ExitDescription> exit_desc; |
There was a problem hiding this comment.
I'm kinda surprised to see this here. I wouldn't expect this to make sense on every (or even most) of the telemetry entries.
There was a problem hiding this comment.
We're going to have 5 types of entries.I think the exit status (or more accurately "return status" for the command-info case) is applicable to the first 4 entries:
- debugger-info (whether LLDB terminates successfully)
- target-info (whether the main executable exits successfully)
- command-info (whether the command executed successfully)
- client-request-info (whether the request completed successfully)
- misc-info (optionall)
So that's 4 out of 5, which is why I thought it made sense to hoist it into the common class.
There was a problem hiding this comment.
Okay, this answers my previous question. It seems like the meaning of the exit code is sufficiently different (debugger and target have a process exit code, which is meaningful based on the OS) but commands and client request infos share nothing with that. It seems like separate enums would be appropriate for the latter. In other words, I think debugger-info and target-info each can have an exit code field (or an ExitDescription) member. The other 3 should have their own dedicated enum.
There was a problem hiding this comment.
ok -ll remove the field from LLDBBaseTelemetry. (But will leave the ExitDescription struct decl since the two of following PRs will use it)
SG?
There was a problem hiding this comment.
I think it'd be better to remove the struct completely for now. It'll be easier to figure out the exact meaning/description for it in the context of the PR which uses it.
There was a problem hiding this comment.
done - removed the ExistDescription
lldb/include/lldb/Core/Telemetry.h
Outdated
|
|
||
| namespace lldb_private { | ||
|
|
||
| using llvm::telemetry::Destination; |
There was a problem hiding this comment.
Maybe, but if you don't want a namespace, then you should be also be careful about other names that you create. For example, lldb_private::LldbEntryKind is not very telling (it doesn't tell you that it has anything to do with telemetry, the string "lldb" in the name is redundant. In this setup I'd probably go with lldb_private::TelemetryEntryKind. Same goes for EventStats which sounds like it has something to do with the Event class in Utility/Event.h (it doesn't) and SteadyTimePoint, which could be generic, but it's suprising to see it defined in Telemetry.h. So, TelemetryEventStats and TelemetryTimePoint ?
You don't have to do that, but after writing all this I'm starting to incline towards putting everything in a namespace.. Then the names can stay pretty much as they are?
Co-authored-by: Pavel Labath <[email protected]>
Co-authored-by: Pavel Labath <[email protected]>
lldb/include/lldb/Core/Telemetry.h
Outdated
| // REQUIRED: Start time of an event | ||
| SteadyTimePoint start; |
There was a problem hiding this comment.
| // REQUIRED: Start time of an event | |
| SteadyTimePoint start; | |
| /// Start time of an event | |
| SteadyTimePoint start; |
This says REQUIRED but there's a default constructor that doesn't initialize it?
There was a problem hiding this comment.
I think what caught Jonas's eye (though he probably doesn't know it) is that this class was very protobuf-y. Let me try to unpack that. We don't have the (understandable if you know the background, but strange if you don't) situation of fields that are optional at the transport layer but required at the application layer elsewhere. And I don't think we need that here either because the transport layer is part of the downstream/vendor implementation, so we can just use regular c++ conventions for the application layer (if a type is std::optional, it's optional; if it's not, it's not).
That's one part of the issue. The second part is question whether the "required-ness" of a field can be enforced syntactically (through a constructor which intializes it). That's usually a nice property though it doesn't work in all situations, and I don't think it's that useful for objects which are plain data carriers (no internal logic). It also only works if every constructor initializes it, so the fact that you've added a constructor which does that, but then also kept the default one which does not (well, it initializes it, but to zero), doesn't really help there.
I'm saying this because this overload set is basically the same as what you'd get if you specified no explicit constructors -- you could still do the same thing, just with curly braces (EventStats{} default initializes everything, EventStats{start} initializes the first member, EventStats{start, end} initializes both). So, if this is the behavior you want, you can just delete all of them. If you want to enforce the requiredness (which might be nice, but I don't think it's necessary -- Jonas may disagree), then you should delete the default constructor -- but then you also need to use these classes differently.
There was a problem hiding this comment.
Sorry for the confusion. Should ahve added more comment on this.
The "REQUIRED" here doesn't mean it has to be set at construction time .It just means it should be set at some point before the TelemetryInfo struct is sent off.
In any case, I've removed the EventStats and moved the start/end time fields directly into the TelemetryInfo struct.
lldb/include/lldb/Core/Telemetry.h
Outdated
| std::chrono::nanoseconds>; | ||
|
|
||
| /// Various time (and possibly memory) statistics of an event. | ||
| struct EventStats { |
There was a problem hiding this comment.
[begin bikeshedding]
Given the current implementation, I don't think there are "stats". How about just EventData or Data. IIUC, this class is meant to represent data that all the LLDB TelemetryInfo will contain. Is there a reason that even needs to be its own class?
[end bikeshedding]
There was a problem hiding this comment.
done (moved the fields into the TelemetryInfo class)
lldb/include/lldb/Core/Telemetry.h
Outdated
| : start(start), end(end) {} | ||
| }; | ||
|
|
||
| /// Describes the exit signal of an event. |
There was a problem hiding this comment.
I don't know what the exit signal of an event means. Do you mean the exit signal of a process? Of LLDB itself?
There was a problem hiding this comment.
it's used by a few subclasses of the TelemetryInfo. (Comment below)
There was a problem hiding this comment.
I'm going to go with Jonas here. "Signal" is a very confusing word here, because a process can die from a (unix) signal (but lldb doesn't do a very good job of differentiating the "death by signal N" and "exit with status N"
There was a problem hiding this comment.
To answer the orignal question, this is used to describe both the exit of LLDB and a process (or main executable).
I've removed the "signal" from the comment.
lldb/include/lldb/Core/Telemetry.h
Outdated
| struct LldbBaseTelemetryInfo : public llvm::telemetry::TelemetryInfo { | ||
| EventStats stats; | ||
|
|
||
| std::optional<ExitDescription> exit_desc; |
There was a problem hiding this comment.
Okay, this answers my previous question. It seems like the meaning of the exit code is sufficiently different (debugger and target have a process exit code, which is meaningful based on the OS) but commands and client request infos share nothing with that. It seems like separate enums would be appropriate for the latter. In other words, I think debugger-info and target-info each can have an exit code field (or an ExitDescription) member. The other 3 should have their own dedicated enum.
Co-authored-by: Jonas Devlieghere <[email protected]>
Co-authored-by: Jonas Devlieghere <[email protected]>
Co-authored-by: Jonas Devlieghere <[email protected]>
|
@JDevlieghere Hi, do you have any further comment/feedback on this? If not, I plan to merge the PR this afternoon. Thanks! |
|
|
||
| Debugger *debugger; | ||
|
|
||
| // For dyn_cast, isa, etc operations. |
There was a problem hiding this comment.
Everywhere else we use /// LLVM RTTI support.
| void serialize(llvm::telemetry::Serializer &serializer) const override; | ||
| }; | ||
|
|
||
| /// The base Telemetry manager instance in LLDB |
| serializer.write("end_time", ToNanosec(end_time.value())); | ||
| } | ||
|
|
||
| static std::string MakeUUID(lldb_private::Debugger *debugger) { |
There was a problem hiding this comment.
s/lldb_private:://, goes for the whole file.
There was a problem hiding this comment.
(all addressed in #126757)
| if (auto ec = llvm::getRandomBytes(random_bytes, 16)) { | ||
| LLDB_LOG(GetLog(LLDBLog::Object), | ||
| "Failed to generate random bytes for UUID: {0}", ec.message()); | ||
| // fallback to using timestamp + debugger ID. |
| } | ||
|
|
||
| TelemetryManager::TelemetryManager( | ||
| std::unique_ptr<llvm::telemetry::Config> config) |
There was a problem hiding this comment.
You're using using ::llvm::telemetry::Foo for some other things but not for the Config. Why not use using namespace llvm::telemetry instead?
|
@oontvoo This PR caused the llvm Standalone bot build to fail, for instance: https://ci.swift.org/view/all/job/llvm.org/view/LLDB/job/lldb-cmake-standalone/1328/consoleText Can someone fix that build problem? |
|
That said, I would personally lean towards putting the whole file around an |
|
Related follow-up : PR/126746 (for excluding telemetry code) |
Details: - This is a subset of PR/98528.( Pavel's suggestion was to split up the patch to make reviewing easier) - This contains only the concrete implementation of the framework to be used but no usages yet. - I plan to send a few follow-up patches: + part2 : includes changes in the plugin-manager to set up the plugin stuff (ie., how to create a default vs vendor impl) + part3 (all of the following can be done in parallel): * part 3_a: define DebuggerTelemetryInfo and related methods to collect data about debugger startup/exit * part 3_b: define TargetTelemetryInfo and related methods to collect data about debug target(s) * part 3_c: define CommandTelemetryInfo and related methods to collect data about debug-commands * part 3_d: define ClientTelemtryInfo and related methods to collect data about lldb-dap/any other client --------- Co-authored-by: Pavel Labath <[email protected]> Co-authored-by: Jonas Devlieghere <[email protected]>
Details: - This is a subset of PR/98528.( Pavel's suggestion was to split up the patch to make reviewing easier) - This contains only the concrete implementation of the framework to be used but no usages yet. - I plan to send a few follow-up patches: + part2 : includes changes in the plugin-manager to set up the plugin stuff (ie., how to create a default vs vendor impl) + part3 (all of the following can be done in parallel): * part 3_a: define DebuggerTelemetryInfo and related methods to collect data about debugger startup/exit * part 3_b: define TargetTelemetryInfo and related methods to collect data about debug target(s) * part 3_c: define CommandTelemetryInfo and related methods to collect data about debug-commands * part 3_d: define ClientTelemtryInfo and related methods to collect data about lldb-dap/any other client --------- Co-authored-by: Pavel Labath <[email protected]> Co-authored-by: Jonas Devlieghere <[email protected]>
Details: - This is a subset of PR/98528.( Pavel's suggestion was to split up the patch to make reviewing easier) - This contains only the concrete implementation of the framework to be used but no usages yet. - I plan to send a few follow-up patches: + part2 : includes changes in the plugin-manager to set up the plugin stuff (ie., how to create a default vs vendor impl) + part3 (all of the following can be done in parallel): * part 3_a: define DebuggerTelemetryInfo and related methods to collect data about debugger startup/exit * part 3_b: define TargetTelemetryInfo and related methods to collect data about debug target(s) * part 3_c: define CommandTelemetryInfo and related methods to collect data about debug-commands * part 3_d: define ClientTelemtryInfo and related methods to collect data about lldb-dap/any other client --------- Co-authored-by: Pavel Labath <[email protected]> Co-authored-by: Jonas Devlieghere <[email protected]>
Details:
* part 3_a: define DebuggerTelemetryInfo and related methods to collect data about debugger startup/exit
* part 3_b: define TargetTelemetryInfo and related methods to collect data about debug target(s)
* part 3_c: define CommandTelemetryInfo and related methods to collect data about debug-commands
* part 3_d: define ClientTelemtryInfo and related methods to collect data about lldb-dap/any other client
Note: Please ignore all changes under llvm/.... These will be reverted after the pending LLVM patch is submitted.