[LLDB][Telemetry]Define telemetry::CommandInfo #129354

oontvoo · 2025-03-01T04:05:41Z

and collect telemetry about a command's execution.

*NOTE: Please consider this PR a DRAFT ( Waiting on PR/127696 to be submitted. )

…about a command's execution.

llvmbot · 2025-03-01T04:06:13Z

@llvm/pr-subscribers-lldb

Author: Vy Nguyen (oontvoo)

Changes

and collect telemetry about a command's execution.

*NOTE: Please consider this PR a DRAFT ( Waiting on PR/127696 to be submitted. )

Full diff: https://github.com/llvm/llvm-project/pull/129354.diff

3 Files Affected:

(modified) lldb/include/lldb/Core/Telemetry.h (+124-3)
(modified) lldb/source/Core/Telemetry.cpp (+14)
(modified) lldb/source/Interpreter/CommandInterpreter.cpp (+20)

diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h
index b72556ecaf3c9..30b8474156124 100644
--- a/lldb/include/lldb/Core/Telemetry.h
+++ b/lldb/include/lldb/Core/Telemetry.h
@@ -12,11 +12,14 @@
 #include "lldb/Core/StructuredDataImpl.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Utility/StructuredData.h"
+#include "lldb/Utility/LLDBLog.h"
 #include "lldb/lldb-forward.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/FunctionExtras.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Telemetry/Telemetry.h"
+#include <atomic>
 #include <chrono>
 #include <ctime>
 #include <memory>
@@ -27,8 +30,16 @@
 namespace lldb_private {
 namespace telemetry {
 
+struct LLDBConfig : public ::llvm::telemetry::Config {
+  const bool m_collect_original_command;
+
+  explicit LLDBConfig(bool enable_telemetry, bool collect_original_command)
+      : ::llvm::telemetry::Config(enable_telemetry), m_collect_original_command(collect_original_command) {}
+};
+
 struct LLDBEntryKind : public ::llvm::telemetry::EntryKind {
-  static const llvm::telemetry::KindType BaseInfo = 0b11000;
+  static const llvm::telemetry::KindType BaseInfo = 0b11000000;
+  static const llvm::telemetry::KindType CommandInfo = 0b11010000;
 };
 
 /// Defines a convenient type for timestamp of various events.
@@ -41,6 +52,7 @@ struct LLDBBaseTelemetryInfo : public llvm::telemetry::TelemetryInfo {
   std::optional<SteadyTimePoint> end_time;
   // TBD: could add some memory stats here too?
 
+  lldb::user_id_t debugger_id = LLDB_INVALID_UID;
   Debugger *debugger;
 
   // For dyn_cast, isa, etc operations.
@@ -56,6 +68,42 @@ struct LLDBBaseTelemetryInfo : public llvm::telemetry::TelemetryInfo {
   void serialize(llvm::telemetry::Serializer &serializer) const override;
 };
 
+
+struct CommandInfo : public LLDBBaseTelemetryInfo {
+
+  // If the command is/can be associated with a target entry this field contains
+  // that target's UUID. <EMPTY> otherwise.
+  std::string target_uuid;
+  // A unique ID for a command so the manager can match the start entry with
+  // its end entry. These values only need to be unique within the same session.
+  // Necessary because we'd send off an entry right before a command's execution
+  // and another right after. This is to avoid losing telemetry if the command
+  // does not execute successfully.
+  int command_id;
+
+  // Eg., "breakpoint set"
+  std::string command_name;
+
+  // !!NOTE!! These two fields are not collected (upstream) due to PII risks.
+  // (Downstream impl may add them if needed).
+  // std::string original_command;
+  // std::string args;
+
+  lldb::ReturnStatus ret_status;
+  std::string error_data;
+
+
+  CommandInfo() = default;
+
+  llvm::telemetry::KindType getKind() const override { return LLDBEntryKind::CommandInfo; }
+
+  static bool classof(const llvm::telemetry::TelemetryInfo *T) {
+    return (T->getKind() & LLDBEntryKind::CommandInfo) == LLDBEntryKind::CommandInfo;
+  }
+
+  void serialize(Serializer &serializer) const override;
+};
+
 /// The base Telemetry manager instance in LLDB.
 /// This class declares additional instrumentation points
 /// applicable to LLDB.
@@ -63,19 +111,92 @@ class TelemetryManager : public llvm::telemetry::Manager {
 public:
   llvm::Error preDispatch(llvm::telemetry::TelemetryInfo *entry) override;
 
+  int MakeNextCommandId();
+
+  LLDBConfig* GetConfig() { return m_config.get(); }
+
   virtual llvm::StringRef GetInstanceName() const = 0;
   static TelemetryManager *getInstance();
 
 protected:
-  TelemetryManager(std::unique_ptr<llvm::telemetry::Config> config);
+  TelemetryManager(std::unique_ptr<LLDBConfig> config);
 
   static void setInstance(std::unique_ptr<TelemetryManager> manger);
 
 private:
-  std::unique_ptr<llvm::telemetry::Config> m_config;
+  std::unique_ptr<LLDBConfig> m_config;
+  const std::string m_id;
+  // We assign each command (in the same session) a unique id so that their
+  // "start" and "end" entries can be matched up.
+  // These values don't need to be unique across runs (because they are
+  // secondary-key), hence a simple counter is sufficent.
+  std::atomic<int> command_id_seed = 0;
   static std::unique_ptr<TelemetryManager> g_instance;
 };
 
+/// Helper RAII class for collecting telemetry.
+template <typename Info> struct ScopedDispatcher {
+  // The debugger pointer is optional because we may not have a debugger yet.
+  // In that case, caller must set the debugger later.
+  ScopedDispatcher(Debugger *debugger = nullptr) {
+    // Start the timer.
+    m_start_time = std::chrono::steady_clock::now();
+    debugger = debugger;
+  }
+  ScopedDispatcher(llvm::unique_function<void(Info *info)> final_callback,
+                   Debugger *debugger = nullptr)
+      : m_final_callback(std::move(final_callback)) {
+    // Start the timer.
+    m_start_time = std::chrono::steady_clock::now();
+    debugger = debugger;
+  }
+
+
+  template typename<T>
+      T GetIfEnable(llvm::unique_function<T(TelemetryManager*)> callable,
+                                   T default_value) {
+    TelemetryManager *manager = TelemetryManager::GetInstanceIfEnabled();
+    if (!manager)
+      return default_value;
+    return callable(manager);
+  }
+
+  void SetDebugger(Debugger *debugger) { debugger = debugger; }
+
+  void SetFinalCallback(llvm::unique_function<void(Info *info)> final_callback) {
+    m_final_callback(std::move(final_callback));
+  }
+
+  void DispatchIfEnable(llvm::unique_function<void(Info *info)> populate_fields_cb) {
+    TelemetryManager *manager = TelemetryManager::GetInstanceIfEnabled();
+    if (!manager)
+      return;
+    Info info;
+    // Populate the common fields we know aboutl
+    info.start_time = m_start_time;
+    info.end_time = std::chrono::steady_clock::now();
+    info.debugger = debugger;
+    // The callback will set the rest.
+    populate_fields_cb(&info);
+    // And then we dispatch.
+    if (llvm::Error er = manager->dispatch(&info)) {
+      LLDB_LOG_ERROR(GetLog(LLDBLog::Object), std::move(er),
+                     "Failed to dispatch entry of type: {0}", m_info.getKind());
+    }
+
+  }
+
+  ~ScopedDispatcher() {
+    // TODO: check if there's a cb to call?
+    DispatchIfEnable(std::move(m_final_callback));
+  }
+
+private:
+  SteadyTimePoint m_start_time;
+  llvm::unique_function<void(Info *info)> m_final_callback;
+  Debugger * debugger;
+};
+
 } // namespace telemetry
 } // namespace lldb_private
 #endif // LLDB_CORE_TELEMETRY_H
diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp
index 5222f76704f91..7fb32f75f474e 100644
--- a/lldb/source/Core/Telemetry.cpp
+++ b/lldb/source/Core/Telemetry.cpp
@@ -43,6 +43,16 @@ void LLDBBaseTelemetryInfo::serialize(Serializer &serializer) const {
     serializer.write("end_time", ToNanosec(end_time.value()));
 }
 
+void CommandInfo::serialize(Serializer &serializer) const {
+  LLDBBaseTelemetryInfo::serializer(serializer);
+
+  serializer.write("target_uuid", target_uuid);
+  serializer.write("command_id", command_id);
+  serializer.write("command_name", command_name);
+  serializer.write("ret_status", ret_status);
+  serializer.write("error_data", error_data);
+}
+
 [[maybe_unused]] static std::string MakeUUID(Debugger *debugger) {
   uint8_t random_bytes[16];
   if (auto ec = llvm::getRandomBytes(random_bytes, 16)) {
@@ -66,6 +76,10 @@ llvm::Error TelemetryManager::preDispatch(TelemetryInfo *entry) {
   return llvm::Error::success();
 }
 
+int TelemetryManager::MakeNextCommandId() {
+
+}
+
 std::unique_ptr<TelemetryManager> TelemetryManager::g_instance = nullptr;
 TelemetryManager *TelemetryManager::getInstance() { return g_instance.get(); }
 
diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp
index c363f20081f9e..aab85145b4c3b 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -47,6 +47,7 @@
 
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Telemetry.h"
 #include "lldb/Host/StreamFile.h"
 #include "lldb/Utility/ErrorMessages.h"
 #include "lldb/Utility/LLDBLog.h"
@@ -88,6 +89,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Telemetry/Telemetry.h"
 
 #if defined(__APPLE__)
 #include <TargetConditionals.h>
@@ -1883,10 +1885,28 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
                                        LazyBool lazy_add_to_history,
                                        CommandReturnObject &result,
                                        bool force_repeat_command) {
+  lldb_private::telemetry::ScopedDispatcher<
+      lldb_private::telemetry:CommandInfo> helper;
+  const int command_id = helper.GetIfEnable<int>([](lldb_private::telemetry::TelemetryManager* ins){
+    return ins->MakeNextCommandId(); }, 0);
+
   std::string command_string(command_line);
   std::string original_command_string(command_string);
   std::string real_original_command_string(command_string);
 
+  helper.DispatchIfEnable([&](lldb_private::telemetry:CommandInfo* info,
+                              lldb_private::telemetry::TelemetryManager* ins){
+    info.command_id = command_id;
+    if (Target* target = GetExecutionContext().GetTargetPtr()) {
+      // If we have a target attached to this command, then get the UUID.
+      info.target_uuid = target->GetExecutableModule() != nullptr
+                         ? GetExecutableModule()->GetUUID().GetAsString()
+                         : "";
+    }
+    if (ins->GetConfig()->m_collect_original_command)
+      info.original_command = original_command_string;
+  });
+
   Log *log = GetLog(LLDBLog::Commands);
   LLDB_LOGF(log, "Processing command: %s", command_line);
   LLDB_SCOPED_TIMERF("Processing command: %s.", command_line);

github-actions · 2025-03-01T04:09:04Z

✅ With the latest revision this PR passed the C/C++ code formatter.

JDevlieghere · 2025-03-04T19:15:18Z

lldb/include/lldb/Core/Telemetry.h

+  // If true, we will collect full details about a debug command (eg., args and
+  // original command). Note: This may contain PII, hence can only be enabled by
+  // the vendor while creating the Manager.
+  const bool m_detailed_command_telemetry;


I like the idea of the Configuration being the one deciding what events we emit. That's easy to audit. Can we make this a bit more generic and maybe have a set of telemetry::KindType that are enabled? We can have a default configuration that enables everything by default upstream (for testing) and then vendors have to override this downstream with the events they're interested in. Originally I imagined doing this in the emitter, but I think the config is a better place to do that. WDYT?

This is for controlling how much data we collect for a given command - not for which type of events we collect.

But we can also add that if you want. Although you could already do taht by overriding the ::dispatch() to simply return/ignore the kind of entries you dont want to record. At that point, the data had been collected but it's not "leaving" LLDB yet - so still fine? (Or do you want something stricter? eg., not collect the data at all?)

Ack, I think this is fine as is. Given that this is a struct, please drop the m_ prefix.

Suggested change

// If true, we will collect full details about a debug command (eg., args and

// original command). Note: This may contain PII, hence can only be enabled by

// the vendor while creating the Manager.

const bool m_detailed_command_telemetry;

// If true, we will collect full details about a debug command (eg., args and

// original command). Note: This may contain PII, hence can only be enabled by

// the vendor while creating the Manager.

const bool detailed_command_telemetry;

Ack, I think this is fine as is. Given that this is a struct, please drop the m_ prefix.

done

lldb/include/lldb/Core/Telemetry.h

JDevlieghere · 2025-03-04T19:27:19Z

lldb/include/lldb/Core/Telemetry.h

+  // !!NOTE!! These two fields are not collected by default due to PII risks.
+  // Vendor may allow them by setting the
+  // LLDBConfig::m_detailed_command_telemetry.
+  std::string original_command;
+  std::string args;


Oh I see, the m_detailed_command_telemetry boolean controls a subset of the command, which means my earlier suggestion doesn't work. I can't think of a way to make this more "generic" without breaking up the event, which you probably don't want to do?

Right - that was the knob for controlling how much data is collected upstream.

(The previous design was to have those DispatchFoo() helper method which could be overriden by vendors to collect more or less data, as needed. There's pros and cons to each ...)

lldb/include/lldb/Core/Telemetry.h

lldb/unittests/Core/TelemetryTest.cpp

lldb/source/Interpreter/CommandInterpreter.cpp

JDevlieghere · 2025-03-04T19:35:41Z

lldb/source/Interpreter/CommandInterpreter.cpp

+  std::string parsed_command_args;
+  CommandObject *cmd_obj = nullptr;


I really dislike that the RAII object forces us to declare variables early like it's C99. Do we need the RAII helper here? Are there early returns we need to account for, or can we just fill in the object at the end of the function?

Yes, unfortunately, there are a bunch of early returns in this function (which is also very long).

Line 1941-1945 : return due to some interruption

Line 1997 - 2020 : nested if statements to set the right return-status.

Co-authored-by: Jonas Devlieghere <[email protected]>

JDevlieghere

A few more nits.

JDevlieghere · 2025-03-05T16:43:39Z

lldb/include/lldb/Core/Telemetry.h

+  // If true, we will collect full details about a debug command (eg., args and
+  // original command). Note: This may contain PII, hence can only be enabled by
+  // the vendor while creating the Manager.
+  const bool m_detailed_command_telemetry;


Ack, I think this is fine as is. Given that this is a struct, please drop the m_ prefix.

Suggested change

// If true, we will collect full details about a debug command (eg., args and

// original command). Note: This may contain PII, hence can only be enabled by

// the vendor while creating the Manager.

const bool m_detailed_command_telemetry;

// If true, we will collect full details about a debug command (eg., args and

// original command). Note: This may contain PII, hence can only be enabled by

// the vendor while creating the Manager.

const bool detailed_command_telemetry;

JDevlieghere · 2025-03-05T16:44:14Z

lldb/include/lldb/Core/Telemetry.h

+  // If the command is/can be associated with a target entry this field contains
+  // that target's UUID. <EMPTY> otherwise.


These should be Doxygen comments (///)

JDevlieghere · 2025-03-05T16:45:42Z

lldb/include/lldb/Core/Telemetry.h

+  uint64_t command_id;
+  // Eg., "breakpoint set"
+  std::string command_name;
+  // !!NOTE!! These two fields are not collected by default due to PII risks.


Drop the !!NOTE!!, or use Note that.

Suggested change

// !!NOTE!! These two fields are not collected by default due to PII risks.

// These two fields are not collected by default due to PII risks.

When comments apply to multiple lines, you can use Doxygen groups:

/// Comment spanning multiple things /// @{ thing one; thing two; /// @}

DavidSpickett · 2025-03-05T16:50:46Z

Defind telemetry...

Define?

oontvoo · 2025-03-05T17:11:15Z

Defind telemetry...

Define?

done

oontvoo · 2025-03-06T20:57:41Z

@JDevlieghere Hi, do you have any other comment for this PR? 🙏

JDevlieghere

Ship it

JDevlieghere · 2025-03-06T21:09:16Z

lldb/include/lldb/Core/Telemetry.h

+  std::string original_command;
+  std::string args;


Would it be worth wrapping these in std::optional to make it more obvious that these fields are optional?

(I thought I left this comment in an earlier comment, but I can't find it so maybe I forgot to submit it)

oontvoo · 2025-03-07T10:58:36Z

Thanks!

and collect telemetry about a command's execution. *NOTE: Please consider this PR a DRAFT ( Waiting on PR/127696 to be submitted. ) --------- Co-authored-by: Jonas Devlieghere <[email protected]>

[LLDB][Telemetry]Defind telemetry::CommandInfo and collect telemetry …

5a992af

…about a command's execution.

oontvoo requested review from labath and cmtice March 1, 2025 04:05

oontvoo requested a review from JDevlieghere as a code owner March 1, 2025 04:05

llvmbot added the lldb label Mar 1, 2025

oontvoo added 10 commits February 28, 2025 23:48

more data

ff1ce49

Merge branch 'main' into command

d7a554d

formatting + renaming

4e9d65e

renaming for clarity

22d15d4

remove empty line

291e923

compile error

2d7d713

fixed test

f875c85

formatting

f025bdb

qual

b859e2d

Merge branch 'main' into command

5655d4c

JDevlieghere reviewed Mar 4, 2025

View reviewed changes

oontvoo and others added 9 commits March 4, 2025 15:46

Update lldb/unittests/Core/TelemetryTest.cpp

017be75

Co-authored-by: Jonas Devlieghere <[email protected]>

Update lldb/source/Interpreter/CommandInterpreter.cpp

9d6997b

Co-authored-by: Jonas Devlieghere <[email protected]>

Update lldb/include/lldb/Core/Telemetry.h

714d803

Co-authored-by: Jonas Devlieghere <[email protected]>

Update lldb/include/lldb/Core/Telemetry.h

481e141

Co-authored-by: Jonas Devlieghere <[email protected]>

addressed review commeht

ec598c6

formatting

46225e8

formatting again

a5c46a1

fix dup definition

ac03cb0

simplified

00bf711

JDevlieghere reviewed Mar 5, 2025

View reviewed changes

addressed review comments

031ee14

oontvoo changed the title ~~[LLDB][Telemetry]Defind telemetry::CommandInfo~~ [LLDB][Telemetry]Define telemetry::CommandInfo Mar 5, 2025

oontvoo added 2 commits March 5, 2025 12:26

remove unused

9a0d4cb

fix build failure

f55ea59

JDevlieghere approved these changes Mar 6, 2025

View reviewed changes

oontvoo added 3 commits March 6, 2025 20:52

make fields optional

ebbda8d

Merge branch 'main' into command

c26b651

build

42e7a62

Merge branch 'main' into command

1d79c13

oontvoo merged commit de9cee1 into llvm:main Mar 7, 2025
5 of 9 checks passed

oontvoo deleted the command branch June 2, 2025 17:17

		std::string parsed_command_args;
		CommandObject *cmd_obj = nullptr;

		// If the command is/can be associated with a target entry this field contains
		// that target's UUID. <EMPTY> otherwise.

	// !!NOTE!! These two fields are not collected by default due to PII risks.
	// These two fields are not collected by default due to PII risks.

[LLDB][Telemetry]Define telemetry::CommandInfo #129354

[LLDB][Telemetry]Define telemetry::CommandInfo #129354

Uh oh!

Conversation

oontvoo commented Mar 1, 2025

Uh oh!

llvmbot commented Mar 1, 2025

Uh oh!

github-actions bot commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oontvoo Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oontvoo Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JDevlieghere left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidSpickett commented Mar 5, 2025

Uh oh!

oontvoo commented Mar 5, 2025

Uh oh!

oontvoo commented Mar 6, 2025

Uh oh!

JDevlieghere left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oontvoo commented Mar 7, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 1, 2025 •

edited

Loading

oontvoo Mar 4, 2025 •

edited

Loading

oontvoo Mar 5, 2025 •

edited

Loading