Define Telemetry plugin for LLDB.

[oontvoo]

Details:

Make LLDB's TelemetryManager a plugin so that vendor can supply appropriate implementation.
The rest of LLDB code will simply call TelemetryManager::getInstance

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Vy Nguyen (oontvoo)

Changes

Details:

Upstream in LLDB, we will have a default TelemetryVendor plugin will provide a default Config and NULL TelemetryManager. Downstream vendors can extend this to provide a vendor-specific Config along with a functional TelemetryManager instance.

---

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

2 Files Affected:

• (added) lldb/include/lldb/Core/TelemetryVendor.h (+39)
• (added) lldb/source/Core/TelemetryVendor.cpp (+43)

diff --git a/lldb/include/lldb/Core/TelemetryVendor.h b/lldb/include/lldb/Core/TelemetryVendor.h
new file mode 100644
index 000000000000000..a2ab3b69fde4225
--- /dev/null
+++ b/lldb/include/lldb/Core/TelemetryVendor.h
@@ -0,0 +1,39 @@
+//===-- TelemetryVendor.h -------------------------------------------------===//
+//
+// 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_TELEMETRYVENDOR_H
+#define LLDB_CORE_TELEMETRYVENDOR_H
+
+#include "lldb/Core/PluginInterface.h"
+#include "lldb/Core/Telemetry.h"
+#include "llvm/Telemetry/Telemetry.h"
+
+#include <memory>
+
+namespace lldb_private {
+
+class TelemetryVendor : public PluginInterface {
+public:
+  TelemetryVendor() = default;
+
+  llvm::StringRef GetPluginName() override;
+
+  static void Initialize();
+
+  static void Terminate();
+
+  static std::unique_ptr<llvm::telemetry::Config> GetTelemetryConfig();
+  static void
+  SetTelemetryConfig(std::unique_ptr<llvm::telemetry::Config> config);
+
+  static lldb::TelemetryManagerSP GetTelemetryManager();
+  static void SetTelemetryManager(const lldb::TelemetryManagerSP &manager_sp);
+};
+
+} // namespace lldb_private
+#endif // LLDB_CORE_TELEMETRYVENDOR_H
diff --git a/lldb/source/Core/TelemetryVendor.cpp b/lldb/source/Core/TelemetryVendor.cpp
new file mode 100644
index 000000000000000..520a01b9b1c7a3e
--- /dev/null
+++ b/lldb/source/Core/TelemetryVendor.cpp
@@ -0,0 +1,43 @@
+//===-- TelemetryVendor.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/TelemetryVendor.h"
+
+namespace lldb_private {
+
+llvm::StringRef TelemetryVendor::GetPluginName() {
+  return "UpstreamTelemetryVendor";
+}
+
+void TelemetryVendor::Initialize() {
+  // The default (upstream) impl will have telemetry disabled by default.
+  SetTelemetryConfig(
+      std::make_unique<llvm::telemetry::Config>(/*enable_telemetry*/ false));
+  SetTelemetryManager(nullptr);
+}
+
+static std::unique_ptr<llvm::telemetry::Config> current_config;
+std::unique_ptr<llvm::telemetry::Config> TelemetryVendor::GetTelemetryConfig() {
+  return current_config;
+}
+
+void TelemetryVendor::SetTelemetryConfig(
+    std::unique_ptr<llvm::telemetry::Config> config) {
+  current_config = std::move(config);
+}
+
+lldb::TelemetryManagerSP TelemetryVendor::GetTelemetryManager() {
+  static TelemteryManagerSP g_telemetry_manager_sp;
+  return g_telemetry_manager_sp;
+}
+
+void SetTelemetryManager(const lldb::TelemetryManagerSP &manager_sp) {
+  GetTelemetryManager() = manager_sp;
+}
+
+} // namespace lldb_private

[github-actions]

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

[labath]

This is definitely better than what you had before, but I still think it's more complicated than it needs to be. For one, I'd like to understand why is there a need for separate TelemetryManager and TelemetryConfig fields. If the downstream implementation is going to be in charge of creating the telemetry manager, why does it need to bother with calling SetTelemetryConfig?

A good way to demonstrate how this is supposed to be used in practice (and also provide some test coverage) would be to write a unit test which creates a simple telemetry vendor plugin and goes through the motions of creating and registering it. It doesn't have to be big -- I'm only interested in the mechanics of registration here, not data collection -- but it should show how one goes about to create and then access the telemetry manager.

If we can reduce the size this class (which I'm 80% certain we can), then I am also thinking that maybe it doesn't need to exist at all, as the remaining (static) functions could be moved into the TelemetryManager class (so that TelemetryManager (not vendor) is the plugin). I think that would also be more consistent with the lldb plugin concept as e.g. ObjectFile instances are created and managed by ObjectFile subclasses -- there isn't a special ObjectFileVendor hierarchy to do that (the SymbolVendor, which you may be modelling this after, is a bit of an exception here, but I don't want to use it as a model precisely because it's so simple that it might not need to exist).

[labath]

Why does this have to be a concrete class? You're saying that the no-op case will use a NULL telemetry manager, which makes me thing that we shouldn't need to be able to instantiate this class.

[oontvoo]

From my understanding, this class is the main (and only) way for the rest of LLDB to obtain a TelemetryManager - that is, via TelemetryVendor::GetTelemetryManager() function, without worrying about picking the the right plugin class.

The downstream plugin will just call this class TelemetryVendor::SetTelemetryManager .

[labath]

Yes, but TelemetryVendor::GetTelemetryManager is a static (non-member) function, which means this class doesn't need to be instantiatable, which means that you don't need to define/override the GetPluginName function. Most, if not all of our base classes for plugin hierarchies are abstract classes.

[oontvoo]

Ah, ok. gotcha. done

[oontvoo]

(done)

[labath]

Could we make these protected (as they're only meant to be called by subclasses)?

[oontvoo]

Yes

[labath]

Could we make it so that we don't need this method? So that the values of these two objects are initialized to this by default?

[oontvoo]

done

[labath]

That's better, but can we now remove the method altogether (instead of just leaving it empty)? There should be no need to "initialize" the base class if it's not doing anything. Ideally, only the concrete vendor subclasses should need initialization (that is also how (most) our other plugins work, though it may not be the case with SymbolVendor, which I suspect you're drawing inspiration from))

[oontvoo]

Done. removed this part.

[labath]

Does this have to be a shared pointer? AIUI, the telemetry manager should be valid for the entirety of the application lifetime, and it shouldn't change, which makes me thing this should just return a raw pointer.

[labath]

This doesn't actually work, as it assigns the copy of the pointer returned by GetTelemetryManager. Given that shared_ptr (and unique_ptr, which I think would be a better choice) are trivially constructible, the simplest way to implement this would be

static std::some_ptr<TelemetryManager> g_manager_sp;
void TelemetryVendor::SetTelemetryManager(std::some_ptr<TelemetryManager> manager_sp) { g_manager_sp = std::move(manager_sp); }

[oontvoo]

done (will use unique_ptr)

[oontvoo]

This is definitely better than what you had before, but I still think it's more complicated than it needs to be. For one, I'd like to understand why is there a need for separate TelemetryManager and TelemetryConfig fields. If the downstream implementation is going to be in charge of creating the telemetry manager, why does it need to bother with calling SetTelemetryConfig?

Sorry, missed this comment some how.
Yeah, that's a good point. I've removed the new class and just define the static TelemetryManager::getInstance

[labath]

I like this. Now it's so simple that a test might not have even been necessary, but since you've already started it, let's keep it, and we can later extend it to test sending of telemetry events.

[labath]

Our usual pattern is to have the unit test tree mirror the source tree, which would put these tests at unittests/Core/TelemetryTest.cpp. The debugserver directory is an exception I'd rather not emulate. I can imagine doing it differently if the tests get rather big, or there are some other issues which make collocating this with the rest of Core tests difficult, but I don't think we're there yet.

[labath]

(That means wrapping whole of TelemetryTest.cpp in #ifdef TELEMETRY)

[oontvoo]

done

[labath]

I think this will actually cause the plugin to be linked into the main lldb library. We definitely don't want that. There's no need to emulate the plugin setup so elaborately. Just define the fake plugin inside TelemetryTest.cpp

[oontvoo]

done

[labath]

This isn't the public API for sending a telemetry event, is it? (Why is it even a public method?) I think we could delete this for now. After you add some actual events, we can extend this to test sending of events using the real API.

[oontvoo]

done - switched to using the public ::dispatch()

[labath]

Okay, but is there a reason for preDispatch to be public or did we just miss this in the original review. Could it be protected (or possibly even private -- one of the interesting properties of C++ is that it allows classes to override private methods) instead?

[oontvoo]

the preDispatch() should be at least accessible by the subclasses (Since the idea was that we moved the bulk of the work of sending TelemetryEntries to destinations to the llvm::telemetry::Manager (ie., the base class).
And the preDispatch() was a way to let the subclasses have a chance to add or remove data from the entries before them being sent off)

[labath]

the preDispatch() should be at least accessible by the subclasses

That's ok, but there's big a difference between "begin able to access (call) a method" and "being able to override a method". C++ methods are always overridable. Access specifiers only control accessibility. So you don't need to make it public if all you want is overridability (customizability of behavior).

And the preDispatch() was a way to let the subclasses have a chance to add or remove data from the entries before them being sent off

That makes sense, but then I also wonder why is dispatch virtual, because preDispatch is basically equivalent to overriding dispatch to do the custom thing and then delegate to base class. There is actually a school of OO design which says that public methods should never be virtual (the idea is to separate the "interface for the outside world (users)" from the "interface for inheritance"). I'm not saying we're particularly strong adherents of this philosophy, but this (pre)dispatch certainly looks like it was taken from one of their books.

[oontvoo]

So you don't need to make it public if all you want is overridability (customizability of behavior).
Ok, i'll send a separate PR to move preDispatch to protected.

That makes sense, but then I also wonder why is dispatch virtual, because preDispatch is basically equivalent to overriding dispatch to do the custom thing and then delegate to base class.

The reason we have dispatch do most of the work was to avoid having subclasses repeating pretty much the same code (ie., iterating over the list of Destinations ... ). But we made it virtual because it's possible downstream implementation want to do the dispatch in some different way (i., parallel ? whatnot)

[labath]

Okay, I think we're ready to summon @JDevlieghere now. :)

[labath]

I think this is fine for what this patch does, but this is a bit of a strange test, as it's checking a side effect that the caller should not really care about (I am somewhat surprised that the manager can even modify the callers telemetry entry, as I think of it more like "consuming" it). For the event tests, I'd like to create slightly more realistic setup, where the fake/mock manager actually send the entries to a Destination object (which then saves it to some array or something) and then we check the events that were received there.

[oontvoo]

I've updated the test to have a Destination that received the entry. Then assert on that received entry.

[JDevlieghere]

Based on the title of this PR I was expecting something slightly different, so maybe I'm missing something. But if we want to make this an LLDB plugin (which I agree we should ), I would expect the PluginManager to manage the instance rather than the plugin doing itself. With the current patch, I don't see how you're taking advantage of it being a plugin.

In the PluginInterface, I would expect a Create instance, something like this:

class TelemetryManager : public PluginInterface {
public:
  static lldb::TelemetryManagerUP Create(const llvm::telemetry::Config& config);

  TelemetryManager() = default;

  virtual <methods you need for TelemetryManager> = 0;
};

And then the PluginManager manages the plugin's lifetime:

typedef PluginInstance<TelemetryManagerCreateInstance> TelemetryManagerInstance;
typedef PluginInstances<TelemetryManager> TelemetryManagerInstances; 

static TelemetryManager &GetTelemetryManagerInstance() {
  static TelemetryManager g_instances;
  return g_instances;
}

bool PluginManager::RegisterPlugin(
    llvm::StringRef name, llvm::StringRef description,
    TelemetryManagerCreateInstance create_callback, const llvm::telemetry::Config& config) {
  return GetTelemetryManagerInstances().RegisterPlugin(name, description, create_callback,
                                             config);
}

bool PluginManager::UnregisterPlugin(
    TelemetryManagerCreateInstance create_callback) {
  return GetTelemetryManagerInstances().UnregisterPlugin(create_callback);
}

TelemetryManagerCreateInstance
PluginManager::GetTelemetryManagerCreateCallbackAtIndex(uint32_t idx) {
  return GetTelemetryManagerInstances().GetCallbackAtIndex(idx);
}

And then Create would iterate over the plugins:

lldb::TelemetryManagerUP TelemetryManager::Create() {
  uint32_t idx = 0;

  while (TelemetryManagerCreateInstance create_instance =
             PluginManager::GetTelemetryManagerCreateCallbackAtIndex(idx++)) {
    if (auto device_up = (*create_instance)())
      return device_up;
  }
  return {};
}

And then GetInstance would look like this:

TelemetryManager *TelemetryManager::GetInstance() {
  if (!m_instance)
    m_instance = TelemetryManager::Create();
  return m_instance.get();
}

[oontvoo]

Based on the title of this PR I was expecting something slightly different, so maybe I'm missing something. But if we want to make this an LLDB plugin (which I agree we should ), I would expect the PluginManager to manage the instance rather than the plugin doing itself. With the current patch, I don't see how you're taking advantage of it being a plugin.

In the PluginInterface, I would expect a Create instance, something like this:
....

This was what we were doing in the initial PR 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.

Instead, we just need the downstream implemntation to register its instance (via the setInstance()).
Then the rest of (upstream) LLDB code, which uses the Telemetry, would just do TelemetryManager::getInstance().

[JDevlieghere]

This was what we were doing in the initial PR 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.

Instead, we just need the downstream implemntation to register its instance (via the setInstance()). Then the rest of (upstream) LLDB code, which uses the Telemetry, would just do TelemetryManager::getInstance().

Thanks for the clarification. I had a feeling I had missed something. But then I don't get the point of making it a "plugin" at all. What's the benefit of declaring it as a plugin? As much as I agree that the plugin system is baroque, there is value in consistency and everything that's called a plugin working the same way. Me being surprised by the PR is a concrete example of that.

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? 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).

[labath]

This was what we were doing in the initial PR 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" 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.

[JDevlieghere]

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).

I don't think anyone has that requirement but I could see a few situations where that could be useful. For example we could have a "textual telemetry" fallback plugin that we use for testing and users could turn on the audit what's being reported. Also with the work I did a few years ago, it's also super easy to disable plugins are compile time or do something like:

if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
  add_subdirectory(DarwinTelemetry)
endif()

Anyway, to be clear, I'm not saying we need to support that, but I could see a reason to make it work like that. The overhead/complexity of not preventing that is small (admittedly that's subjective) in exchange for consistency and if this was the only argument I'd feel it would not warrant diverging from the established way of doing this. I think (2) is more compelling.

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&)).

Yes, I like the debugger_id approach we've taken with the progress events and the debugger diagnostics. I agree we should do the same for the telemetry as it has worked well. In other words, I understand we only need a single instance. But that's totally something we can handle through the PluginManager interface, which could return you the same one with the snippet of code I posted earlier.

What I'm hearing is that Telemetry could use the plugin mechanism but that it offers you to do things that we don't need (pick at runtime, have multiple instances) and it adds more complexity to do it that way. I follow both arguments and to be clear, I have the same qualms with the Plugin machinery as everyone else. I'm not really advocating for it beyond "this could be used to make this work".

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);
  }

Given that they also need to support (1) they'd always have to be full blown plugins, right? So it's not like they could be migrated over to a "plugin light" if we came up with something like that?

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?

Yes. To summarize, it sounds like Telemetry shouldn't be a "plugin" at all, and it should just behave like the other singletons we have today (FileSystem, Diagnostics (*), etc). The fact that different vendors will have different implementation remains an implementation detail (as opposed to being formalized by defining it as a Plugin. The usual ::Initialize method will responsible for instantiating the concrete vendor implementation. We don't need to inherit from PluginInterface as it really doesn't give you anything.

How does that sound to you @oontvoo & @labath?

Another similar mechanism we have is the "static polymorphism" 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.

Agreed.

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.

Cool. There's definitely a lot of repetition in that class that could be avoided.

(*) I really regret naming this class that as it conflicts with the "Debugger Diagnostics" which are something entirely different.

[oontvoo]

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).

I don't think anyone has that requirement but I could see a few situations where that could be useful. For example we could have a "textual telemetry" fallback plugin that we use for testing and users could turn on the audit what's being reported.

This sounds like it could be done via configuration on the Destination (ie., where the collected data ended up), rather than the TelemetryManager (plugin) itself.

We don't need to inherit from PluginInterface as it really doesn't give you anything.

From my understanding, the benefit of inheriting from the interface is you can re-use the existing cmake and C++ macros magic for defining vendor impl - that is, you just put LLDB_PLUGIN_DEFINE(MyPlugin) in the vendor code, along with the cmake declaration and you're done! The framework takes care of ensuring the ::Initialize() is called to set the instance.

How does that sound to you @oontvoo & @labath?

SG, either way - the simpler the better :)

[labath]

Given that they also need to support (1) they'd always have to be full blown plugins, right? So it's not like they could be migrated over to a "plugin light" if we came up with something like that?

Sort of. There would have to be a full-blown plugin. But there's no reason why setting the host platform must be handled at the same place as registering a (remote) platform plugin. For example, if we had a super-succinct mechanism to register a full-blown plugin, and the host platform wouldn't fit into that, we could put that into completely different place, and everything would be fine. Conversely, we could also make registering the host platform use the same callback iteration code as everything else (we'd pass PlatformLinux::CreateHostPlatformCallback to the PluginManager -- if linux was the host -- and then it would create the host platform by iterating through the callbacks).

But I don't think either of those is likely, so it's probably going to stay the way it is...

We don't need to inherit from PluginInterface as it really doesn't give you anything.

From my understanding, the benefit of inheriting from the interface is you can re-use the existing cmake and C++ macros magic for defining vendor impl - that is, you just put LLDB_PLUGIN_DEFINE(MyPlugin) in the vendor code, along with the cmake declaration and you're done! The framework takes care of ensuring the ::Initialize() is called to set the instance.

Not exactly. All LLDB_PLUGIN_DEFINE does is it sets up a call to YourPlugin::Initialize. Anything that happens after that is up to the plugin code. There is nothing requiring you to inherit PluginInterface in any object that you create. IOW, it's totally feasible to have a "plugin" which does not inherit from "PluginInterface". Whether that's a good idea is a different question. I'm personally fine with that as I never liked the PluginInterface interface much anyway.

How does that sound to you @oontvoo & @labath?

SG, either way - the simpler the better :)

SGTM

[oontvoo]

Thanks, all. Just to wrap up, the only change required here is to remove the PluginInterface inheritance (and renaming the current Manager::getPluginName() to Manager::getInstanceName() to avoid confusion.