[Lldb-commits] [lldb] [LLDB][Telemetry]Define DebuggerTelemetryInfo and related methods (PR #127696)

[Pavel Labath via lldb-commits]

================
@@ -761,12 +767,29 @@ void Debugger::InstanceInitialize() {
 
 DebuggerSP Debugger::CreateInstance(lldb::LogOutputCallback log_callback,
                                     void *baton) {
+#ifdef LLVM_BUILD_TELEMETRY
+  lldb_private::telemetry::SteadyTimePoint start_time =
+      std::chrono::steady_clock::now();
+#endif
   DebuggerSP debugger_sp(new Debugger(log_callback, baton));
   if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) {
     std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr);
     g_debugger_list_ptr->push_back(debugger_sp);
   }
   debugger_sp->InstanceInitialize();
+
+#ifdef LLVM_BUILD_TELEMETRY
+  if (auto *telemetry_manager = telemetry::TelemetryManager::getInstance()) {
+    if (telemetry_manager->getConfig()->EnableTelemetry) {
+      lldb_private::telemetry::DebuggerTelemetryInfo entry;
+      entry.start_time = start_time;
+      entry.end_time = std::chrono::steady_clock::now();
+      entry.debugger = debugger_sp.get();
+      telemetry_manager->atDebuggerStartup(&entry);
+    }
+  }
+#endif
----------------
labath wrote:

I would actually argue that the telemetry library (before the cmake flag) was very similar to the signposts setup (and all of the other cases where we have an external dependency), with the library *itself* being the wrapper: it exists, and can be used unconditionally, but unless some other component is also available, then it amounts to a slightly sophisticated noop.

Sure, in this case the subject matter is a lot more sensitive, and there are some differences in how it works. For example, that this "other component" takes the form of a subclass, and that no support for an actual implementation exists in this repo, but is rather meant to be added downstream. However, I think the principle is the same.

The last point (no actual concrete implementation) also makes me feel like any cmake flag would be mostly a placebo (as the only way to make this do something is to do it downstream, and at that point, you can always disable/override any protections we add here), but if that's what it takes, then that's fine. I think anything would be better than `#ifdef`ing everything out, or building another wrapper on top of that. I just don't know what kind of a form should that take. For example would something like
```
TelemetryManager* getInstance() {
#ifdef TELEMETRY
  return g_instance;
#else
  return nullptr;
#endif
}
```
be sufficient? (if we put that in a header, we should also have a decent chance of the compiler dead-stripping all the code which depends on this)

https://github.com/llvm/llvm-project/pull/127696