[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.
@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
| #ifndef LLDB_CORE_TELEMETRY_H | ||
| #define LLDB_CORE_TELEMETRY_H | ||
|
|
||
| #include <chrono> |
There was a problem hiding this comment.
This isn't the order we use in llvm. Just remove the blank lines and let clang-format order it for you.
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 @@ | |||
|
|
|||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
| #include "lldb/Core/Telemetry.h" |
There was a problem hiding this comment.
fix include order (remove blank lines and reformat)
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)); |
| # 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) |
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.