[Lldb-commits] [lldb] Define Telemetry plugin for LLDB. (PR #126588)

[Pavel Labath via lldb-commits]

labath wrote:

> This was what we were doing in the [initial PR](https://github.com/llvm/llvm-project/pull/98528/files#diff-20a2060f8e87c6742d6f2c7ae97e919f8485995d7808bd9fccbfbede697a9ec7) but Pavel had correctly pointed out that the architecture was unnecessarily "baroque". GIven there will be only one instance of the TelemetrManager at a time and that we will not be creating a new manager on the fly, none of those complex hooks are necessary.

I think you've summarized this correctly. I just want to elaborate/add some colour a bit:

The (baroque) way that the other plugins work serves two purposes:

1. controlling the type (class) of the plugin being created based on some runtime property
2. creating an arbitrary number of plugin instances

For telemetry "plugins", I don't think we want/need (1) because the type of the plugin is determined by the vendor -- at build time. A user can't change that at runtime. They may be able to disable collection, but they can't say "well, today I feel like reporting my telemetry to company <X>" and change a setting or something to make that happen. I mean, it's probably doable, but I don't think anyone has that as a requirement (if you do, let us know).

The second question is I think more interesting, and I think it boils down to "what is the 'scope' of a TelemetryManager object". I can see two options, and I think both of them are reasonable. One (which is what this PR does) is to make it a process-wide object (a singleton). The other is to create one instance per Debugger object. The advantage of the first one is that the object is always available -- there's no problem with accessing the object from parts of code (e.g. everything under the `Module` class) which are not tied to a specific debugger instance (I'm sure you remember the contortions that we go through to report progress events related to debug info parsing). The advantage of the second one is that it fits in better with our object model and the events we do report are automatically organized into a user session.

The reason I think the first one is better is that organizing events into "sessions" is still possible with this model (just add a debugger_id field to the reported event), but it still allows you to report debugger-less events, but I could be convinced otherwise. However, even if we do that, I still don't think we need a *list* of plugins (due to the first item). All we'd need to do is replace `setInstance(std::unique_ptr<TelemetryManager>)` with something like `setInstanceCreateCallback(std::unique_ptr<TelemetryManager> (*)(Debugger&))`.


> More of a question for @labath but do we have an example of another place where we have "semi plugins" like this and if not, what are existing plugins that could be reworked to be lighter weight? 

We sort of do have a precedent for that, but it's in an unlikely place -- the Platform plugins. It sounds hard to believe because Platforms are probably the most complicated kinds of plugins, but part of that complexity comes from the fact that they are doing two mostly separate things:

1. Registering themselves to handle the remote systems of the given kind. This part is pretty much the same as all other plugins and uses the PluginManager, iteration, and whatnot.
2. Registering themselves as the "Host" platform, if they match the current build host. This part is achieved by a call to Platform::SetPlatform (and that's an analogue to TelemetryManager::setInstance).


Here's how the PlatformLinux::Initialize looks like. The first part is just us being "baroque":
```
  PlatformPOSIX::Initialize(); 

  if (g_initialize_count++ == 0) {
```
Next it does the the item (2)
```
#if defined(__linux__) && !defined(__ANDROID__)
    PlatformSP default_platform_sp(new PlatformLinux(true));
    default_platform_sp->SetSystemArchitecture(HostInfo::GetArchitecture());
    Platform::SetHostPlatform(default_platform_sp);
#endif
```
And finally item (1)
```
    PluginManager::RegisterPlugin(
        PlatformLinux::GetPluginNameStatic(false),
        PlatformLinux::GetPluginDescriptionStatic(false),
        PlatformLinux::CreateInstance, nullptr);
  }
```

Another slightly similar mechanism are the process plugins. ProcessLinux and Process***BSD are a sort of a plugin, but they aren't even linked into lldb. They are used by lldb-server, which only links the plugin which matches the current build host (there's no runtime choice). I think it's reasonable to call these "plugins" even though their choice is completely static.

> I think we need to have a sufficiently strong motivation to diverge from the existing way of writing plugins (or have a renaissance where we improve it for all the plugins).

Does this sound compelling enough? 

Another similar mechanism we have is the ["static polymorphism"](https://github.com/llvm/llvm-project/blob/55b0fde20a2ba1c67313cb4c8d6a30316facd6ad/lldb/include/lldb/Host/HostInfo.h#L35) used in the Host classes. I would like to avoid that one because it requires modifying an existing file to add a new implementation. Since this is something that is explicitly meant to be extensible downstream, I think it'd be nice to not need to do that.

FWIW, I actually would like to do a bit of a renaissance in the plugin architecture, to reduce the boilerplate present in their definitions. However, it's not really relevant here since some of that complexity really is necessary.

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