address review comments

Author: oontvoo

lldb/include/lldb/Core/Telemetry.h

@@ -14,6 +14,7 @@
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/StructuredData.h"
 #include "lldb/lldb-forward.h"
+#include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/JSON.h"
@@ -27,9 +28,16 @@
 namespace lldb_private {
 namespace telemetry {
 
+// We expect each (direct) subclass of LLDBTelemetryInfo to
+// have an LLDBEntryKind in the form 0b11xxxxxxxx
+// Specifically:
+//  - Length: 8 bits
+//  - First two bits (MSB) must be 11 - the common prefix
+// If any of the subclass has descendents, those descendents
+// must have their LLDBEntryKind in the similar form (ie., share common prefix)
 struct LLDBEntryKind : public ::llvm::telemetry::EntryKind {
-  static const llvm::telemetry::KindType BaseInfo = 0b11000;
-  static const llvm::telemetry::KindType DebuggerInfo = 0b11001;
+  static const llvm::telemetry::KindType BaseInfo = 0b11000000;
+  static const llvm::telemetry::KindType DebuggerInfo = 0b11000100;
 };
 
 /// Defines a convenient type for timestamp of various events.
@@ -58,15 +66,10 @@ struct LLDBBaseTelemetryInfo : public llvm::telemetry::TelemetryInfo {
   void serialize(llvm::telemetry::Serializer &serializer) const override;
 };
 
-/// Describes the exit status of a debugger.
-struct ExitDescription {
-  int exit_code;
-  std::string description;
-};
-
 struct DebuggerInfo : public LLDBBaseTelemetryInfo {
   std::string lldb_version;
-  std::optional<ExitDescription> exit_desc;
+
+  bool is_exit_entry = false;
 
   DebuggerInfo() = default;
 
@@ -75,7 +78,9 @@ struct DebuggerInfo : public LLDBBaseTelemetryInfo {
   }
 
   static bool classof(const llvm::telemetry::TelemetryInfo *T) {
-    return T->getKind() == LLDBEntryKind::DebuggerInfo;
+    // Subclasses of this is also acceptable
+    return (T->getKind() & LLDBEntryKind::DebuggerInfo) ==
+           LLDBEntryKind::DebuggerInfo;
   }
 
   void serialize(llvm::telemetry::Serializer &serializer) const override;
@@ -91,8 +96,11 @@ class TelemetryManager : public llvm::telemetry::Manager {
   const llvm::telemetry::Config *GetConfig();
 
   virtual llvm::StringRef GetInstanceName() const = 0;
+
   static TelemetryManager *GetInstance();
 
+  static TelemetryManager *GetInstanceIfEnabled();
+
 protected:
   TelemetryManager(std::unique_ptr<llvm::telemetry::Config> config);
 
@@ -110,9 +118,9 @@ class TelemetryManager : public llvm::telemetry::Manager {
 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(std::function<void(Info *info)> callback,
+  ScopedDispatcher(llvm::unique_function<void(Info *info)> callback,
                    Debugger *debugger = nullptr)
-      : m_callback(callback) {
+      : m_callback(std::move(callback)) {
     // Start the timer.
     m_start_time = std::chrono::steady_clock::now();
     m_info.debugger = debugger;
@@ -121,11 +129,12 @@ template <typename Info> struct ScopedDispatcher {
   void SetDebugger(Debugger *debugger) { m_info.debugger = debugger; }
 
   ~ScopedDispatcher() {
-    TelemetryManager *manager = TelemetryManager::GetInstance();
     // If Telemetry is disabled (either at buildtime or runtime),
     // then don't do anything.
-    if (!manager || !manager->GetConfig()->EnableTelemetry)
+    TelemetryManager *manager = TelemetryManager::GetInstanceIfEnabled();
+    if (!manager)
       return;
+
     m_info.start_time = m_start_time;
     // Populate common fields that we can only set now.
     m_info.end_time = std::chrono::steady_clock::now();
@@ -140,7 +149,7 @@ template <typename Info> struct ScopedDispatcher {
 
 private:
   SteadyTimePoint m_start_time;
-  std::function<void(Info *info)> m_callback;
+  llvm::unique_function<void(Info *info)> m_callback;
   Info m_info;
 };
 

lldb/source/Core/Debugger.cpp

@@ -1003,9 +1003,7 @@ void Debugger::Clear() {
         helper(
             [this](lldb_private::telemetry::DebuggerInfo *info) {
               assert(this == info->debugger);
-              // If we are here, then there was no error.
-              // Any abnormal exit will be reported by the crash-handler.
-              info->exit_desc = {0, ""};
+              info->is_exit_entry = true;
             },
             this);
     ClearIOHandlers();

lldb/source/Core/Telemetry.cpp

@@ -64,10 +64,7 @@ void DebuggerInfo::serialize(Serializer &serializer) const {
   LLDBBaseTelemetryInfo::serialize(serializer);
 
   serializer.write("lldb_version", lldb_version);
-  if (exit_desc.has_value()) {
-    serializer.write("exit_code", exit_desc->exit_code);
-    serializer.write("exit_desc", exit_desc->description);
-  }
+  serializer.write("is_exit_entry", is_exit_entry);
 }
 
 TelemetryManager::TelemetryManager(std::unique_ptr<Config> config)
@@ -93,6 +90,16 @@ TelemetryManager *TelemetryManager::GetInstance() {
   return g_instance.get();
 }
 
+TelemetryManager *TelemetryManager::GetInstanceIfEnabled() {
+  // Telemetry may be enabled at build time but disabled at runtime.
+  if (TelemetryManager *ins = TelemetryManager::GetInstance()) {
+    if (ins->GetConfig()->EnableTelemetry)
+      return ins;
+  }
+
+  return nullptr;
+}
+
 void TelemetryManager::SetInstance(std::unique_ptr<TelemetryManager> manager) {
   if (Config::BuildTimeEnableTelemetry)
     g_instance = std::move(manager);

lldb/unittests/Core/TelemetryTest.cpp

@@ -22,31 +22,30 @@ struct FakeTelemetryInfo : public telemetry::LLDBBaseTelemetryInfo {
   std::string msg;
   int num;
 
-  ::llvm::telemetry::KindType getKind() const override { return 0b11111; }
+  ::llvm::telemetry::KindType getKind() const override { return 0b11111111; }
 };
 
 class TestDestination : public llvm::telemetry::Destination {
 public:
-  TestDestination(std::vector<llvm::telemetry::TelemetryInfo *> *entries)
+  TestDestination(
+      std::vector<std::unique_ptr<llvm::telemetry::TelemetryInfo>> *entries)
       : received_entries(entries) {}
 
   llvm::Error
   receiveEntry(const llvm::telemetry::TelemetryInfo *entry) override {
     // Save a copy of the entry for later verification (because the original
     // entry might have gone out of scope by the time verification is done.
-    if (auto *fake_entry = llvm::dyn_cast<FakeTelemetryInfo>(entry)) {
-      FakeTelemetryInfo *copied = new FakeTelemetryInfo();
-      copied->msg = fake_entry->msg;
-      copied->num = fake_entry->num;
-      received_entries->push_back(copied);
-    }
+    if (auto *fake_entry = llvm::dyn_cast<FakeTelemetryInfo>(entry))
+      received_entries->push_back(
+          std::make_unique<FakeTelemetryInfo>(*fake_entry));
     return llvm::Error::success();
   }
 
   llvm::StringLiteral name() const override { return "TestDestination"; }
 
 private:
-  std::vector<llvm::telemetry::TelemetryInfo *> *received_entries;
+  std::vector<std::unique_ptr<llvm::telemetry::TelemetryInfo>>
+      *received_entries;
 };
 
 class FakePlugin : public telemetry::TelemetryManager {
@@ -92,17 +91,18 @@ TELEMETRY_TEST(TelemetryTest, PluginTest) {
   auto *ins = lldb_private::telemetry::TelemetryManager::GetInstance();
   ASSERT_NE(ins, nullptr);
 
-  std::vector<::llvm::telemetry::TelemetryInfo *> expected_entries;
+  std::vector<std::unique_ptr<::llvm::telemetry::TelemetryInfo>>
+      received_entries;
   ins->addDestination(
-      std::make_unique<lldb_private::TestDestination>(&expected_entries));
+      std::make_unique<lldb_private::TestDestination>(&received_entries));
 
   lldb_private::FakeTelemetryInfo entry;
   entry.msg = "";
 
   ASSERT_THAT_ERROR(ins->dispatch(&entry), ::llvm::Succeeded());
-  ASSERT_EQ(1U, expected_entries.size());
+  ASSERT_EQ(1U, received_entries.size());
   EXPECT_EQ("In FakePlugin",
-            llvm::dyn_cast<lldb_private::FakeTelemetryInfo>(expected_entries[0])
+            llvm::dyn_cast<lldb_private::FakeTelemetryInfo>(received_entries[0])
                 ->msg);
 
   ASSERT_EQ("FakeTelemetryPlugin", ins->GetInstanceName());
@@ -112,9 +112,10 @@ TELEMETRY_TEST(TelemetryTest, ScopedDispatcherTest) {
   lldb_private::FakePlugin::Initialize();
   auto *ins = TelemetryManager::GetInstance();
   ASSERT_NE(ins, nullptr);
-  std::vector<::llvm::telemetry::TelemetryInfo *> expected_entries;
+  std::vector<std::unique_ptr<::llvm::telemetry::TelemetryInfo>>
+      received_entries;
   ins->addDestination(
-      std::make_unique<lldb_private::TestDestination>(&expected_entries));
+      std::make_unique<lldb_private::TestDestination>(&received_entries));
 
   {
     ScopedDispatcher<lldb_private::FakeTelemetryInfo> helper(
@@ -131,10 +132,10 @@ TELEMETRY_TEST(TelemetryTest, ScopedDispatcherTest) {
         [](lldb_private::FakeTelemetryInfo *info) { info->num = 2; });
   }
 
-  EXPECT_EQ(3U, expected_entries.size());
+  EXPECT_EQ(3U, received_entries.size());
   for (int i = 0; i < 3; ++i) {
     EXPECT_EQ(
-        i, llvm::dyn_cast<lldb_private::FakeTelemetryInfo>(expected_entries[i])
+        i, llvm::dyn_cast<lldb_private::FakeTelemetryInfo>(received_entries[i])
                ->num);
   }
 }