[LLDB][Telemetry] Collect telemetry from client when allowed. #129728
Conversation
This patch is slightly different from other impl in that we dispatch client-telemetry via a different helper method. This is to make it easier for vendor to opt-out (simply by overriding the method to do nothing). There is also a configuration option to disallow collecting client telemetry.
|
@llvm/pr-subscribers-lldb Author: Vy Nguyen (oontvoo) ChangesThis patch is slightly different from other impl in that we dispatch client-telemetry via a different helper method. This is to make it easier for vendor to opt-out (simply by overriding the method to do nothing). There is also a configuration option to disallow collecting client telemetry. Full diff: https://github.com/llvm/llvm-project/pull/129728.diff 9 Files Affected:
diff --git a/lldb/include/lldb/API/SBDebugger.h b/lldb/include/lldb/API/SBDebugger.h
index e0819f1684f8b..28f92f2095951 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -13,6 +13,7 @@
#include "lldb/API/SBDefines.h"
#include "lldb/API/SBPlatform.h"
+#include "lldb/API/SBStructuredData.h"
namespace lldb_private {
class CommandPluginInterfaceImplementation;
@@ -249,6 +250,8 @@ class LLDB_API SBDebugger {
lldb::SBTarget GetDummyTarget();
+ void DispatchClientTelemetry(const lldb::SBStructuredData &data);
+
// Return true if target is deleted from the target list of the debugger.
bool DeleteTarget(lldb::SBTarget &target);
diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 6ebc6147800e1..e40666d5ceec7 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -19,6 +19,8 @@
#include "lldb/Core/FormatEntity.h"
#include "lldb/Core/IOHandler.h"
#include "lldb/Core/SourceManager.h"
+#include "lldb/Core/StructuredDataImpl.h"
+#include "lldb/Core/Telemetry.h"
#include "lldb/Core/UserSettingsController.h"
#include "lldb/Host/HostThread.h"
#include "lldb/Host/StreamFile.h"
@@ -31,6 +33,7 @@
#include "lldb/Utility/Diagnostics.h"
#include "lldb/Utility/FileSpec.h"
#include "lldb/Utility/Status.h"
+#include "lldb/Utility/StructuredData.h"
#include "lldb/Utility/UserID.h"
#include "lldb/lldb-defines.h"
#include "lldb/lldb-enumerations.h"
@@ -127,6 +130,8 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
void Clear();
+ void DispatchClientTelemetry(const lldb_private::StructuredDataImpl &entry);
+
bool GetAsyncExecution();
void SetAsyncExecution(bool async);
diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h
index 7d8716f1659b5..cad4a4a6c9048 100644
--- a/lldb/include/lldb/Core/Telemetry.h
+++ b/lldb/include/lldb/Core/Telemetry.h
@@ -19,6 +19,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/JSON.h"
#include "llvm/Telemetry/Telemetry.h"
+#include <atomic>
#include <chrono>
#include <ctime>
#include <memory>
@@ -28,6 +29,23 @@
namespace lldb_private {
namespace telemetry {
+struct LLDBConfig : public ::llvm::telemetry::Config {
+ // 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 telemetry from LLDB's clients (eg., lldb-dap) via
+ // the SB interface. Must also be enabled by the vendor while creating the
+ // manager.
+ const bool m_enable_client_telemetry;
+
+ explicit LLDBConfig(bool enable_telemetry, bool detailed_command_telemetry,
+ bool enable_client_telemetry)
+ : ::llvm::telemetry::Config(enable_telemetry),
+ m_detailed_command_telemetry(detailed_command_telemetry),
+ m_enable_client_telemetry(enable_client_telemetry) {}
+};
+
// We expect each (direct) subclass of LLDBTelemetryInfo to
// have an LLDBEntryKind in the form 0b11xxxxxxxx
// Specifically:
@@ -37,6 +55,7 @@ namespace telemetry {
// must have their LLDBEntryKind in the similar form (ie., share common prefix)
struct LLDBEntryKind : public ::llvm::telemetry::EntryKind {
static const llvm::telemetry::KindType BaseInfo = 0b11000000;
+ static const llvm::telemetry::KindType ClientInfo = 0b11100000;
static const llvm::telemetry::KindType DebuggerInfo = 0b11000100;
};
@@ -86,6 +105,11 @@ struct DebuggerInfo : public LLDBBaseTelemetryInfo {
void serialize(llvm::telemetry::Serializer &serializer) const override;
};
+struct ClientInfo : public LLDBBaseTelemetryInfo {
+ std::string request_name;
+ std::optional<std::string> error_msg;
+};
+
/// The base Telemetry manager instance in LLDB.
/// This class declares additional instrumentation points
/// applicable to LLDB.
@@ -93,24 +117,24 @@ class TelemetryManager : public llvm::telemetry::Manager {
public:
llvm::Error preDispatch(llvm::telemetry::TelemetryInfo *entry) override;
- const llvm::telemetry::Config *GetConfig();
+ const LLDBConfig *GetConfig() { return m_config.get(); }
+
+ void DispatchClientTelemery(const lldb_private::StructuredDataImpl &entry,
+ Debugger *debugger);
virtual llvm::StringRef GetInstanceName() const = 0;
static TelemetryManager *GetInstance();
- static TelemetryManager *GetInstanceIfEnabled();
-
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;
// Each instance of a TelemetryManager is assigned a unique ID.
const std::string m_id;
-
static std::unique_ptr<TelemetryManager> g_instance;
};
@@ -118,39 +142,54 @@ 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(llvm::unique_function<void(Info *info)> callback,
+ ScopedDispatcher(Debugger *debugger = nullptr) {
+ // Start the timer.
+ m_start_time = std::chrono::steady_clock::now();
+ this->debugger = debugger;
+ }
+ ScopedDispatcher(llvm::unique_function<void(Info *info)> final_callback,
Debugger *debugger = nullptr)
- : m_callback(std::move(callback)) {
+ : m_final_callback(std::move(final_callback)) {
// Start the timer.
m_start_time = std::chrono::steady_clock::now();
- m_info.debugger = debugger;
+ this->debugger = debugger;
}
- void SetDebugger(Debugger *debugger) { m_info.debugger = debugger; }
+ void SetDebugger(Debugger *debugger) { this->debugger = debugger; }
- ~ScopedDispatcher() {
- // If Telemetry is disabled (either at buildtime or runtime),
- // then don't do anything.
- TelemetryManager *manager = TelemetryManager::GetInstanceIfEnabled();
- if (!manager)
- return;
+ void DispatchOnExit(llvm::unique_function<void(Info *info)> final_callback) {
+ // We probably should not be overriding previously set cb.
+ assert(!m_final_callback);
+ m_final_callback = std::move(final_callback);
+ }
- 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();
- // The callback will set the remaining fields.
- m_callback(&m_info);
+ void DispatchNow(llvm::unique_function<void(Info *info)> populate_fields_cb) {
+ TelemetryManager *manager = TelemetryManager::GetInstance();
+ if (!manager->GetConfig()->EnableTelemetry)
+ 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(&m_info)) {
+ 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());
+ "Failed to dispatch entry of type: {0}", info.getKind());
}
}
+ ~ScopedDispatcher() {
+ if (m_final_callback)
+ DispatchNow(std::move(m_final_callback));
+ }
+
private:
SteadyTimePoint m_start_time;
- llvm::unique_function<void(Info *info)> m_callback;
- Info m_info;
+ llvm::unique_function<void(Info *info)> m_final_callback;
+ Debugger *debugger;
};
} // namespace telemetry
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index e646b09e05852..7cd2beae94189 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -965,6 +965,17 @@ SBTarget SBDebugger::GetDummyTarget() {
return sb_target;
}
+void SBDebugger::DispatchClientTelemetry(const lldb::SBStructuredData &entry) {
+ LLDB_INSTRUMENT_VA(this);
+ if (lldb_private::Debugger *debugger = this->get()) {
+ debugger->DispatchClientTelemetry(*(entry.m_impl_up.get()));
+ } else {
+ Log *log = GetLog(LLDBLog::API);
+ LLDB_LOGF(log,
+ "Could not send telemetry from SBDebugger - debugger was null.");
+ }
+}
+
bool SBDebugger::DeleteTarget(lldb::SBTarget &target) {
LLDB_INSTRUMENT_VA(this, target);
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index dbe3d72e5efa4..5a04eb876a815 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -781,6 +781,12 @@ DebuggerSP Debugger::CreateInstance(lldb::LogOutputCallback log_callback,
return debugger_sp;
}
+void Debugger::DispatchClientTelemetry(
+ const lldb_private::StructuredDataImpl &entry) {
+ lldb_private::telemetry::TelemeryManager::GetInstance()
+ ->DispatchClientTelemetry(entry);
+}
+
void Debugger::HandleDestroyCallback() {
const lldb::user_id_t user_id = GetID();
// Invoke and remove all the callbacks in an FIFO order. Callbacks which are
diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp
index 367e94d6566de..3edb27e535da9 100644
--- a/lldb/source/Core/Telemetry.cpp
+++ b/lldb/source/Core/Telemetry.cpp
@@ -59,6 +59,12 @@ void LLDBBaseTelemetryInfo::serialize(Serializer &serializer) const {
if (end_time.has_value())
serializer.write("end_time", ToNanosec(end_time.value()));
}
+void ClientInfo::serialize(Serializer &serializer) const {
+ LLDBBaseTelemetryInfo::serialize(serializer);
+ serializer.write("request_name", request_name);
+ if (error_msg.has_value())
+ serializer.write("error_msg", error_msg.value());
+}
void DebuggerInfo::serialize(Serializer &serializer) const {
LLDBBaseTelemetryInfo::serialize(serializer);
@@ -67,7 +73,7 @@ void DebuggerInfo::serialize(Serializer &serializer) const {
serializer.write("is_exit_entry", is_exit_entry);
}
-TelemetryManager::TelemetryManager(std::unique_ptr<Config> config)
+TelemetryManager::TelemetryManager(std::unique_ptr<LLDBConfig> config)
: m_config(std::move(config)), m_id(MakeUUID()) {}
llvm::Error TelemetryManager::preDispatch(TelemetryInfo *entry) {
@@ -79,23 +85,90 @@ llvm::Error TelemetryManager::preDispatch(TelemetryInfo *entry) {
return llvm::Error::success();
}
-const Config *TelemetryManager::GetConfig() { return m_config.get(); }
+void TelemetryManager::DispatchClientTelemetry(
+ const lldb_private::StructuredDataImpl &entry, Debugger *debugger) {
+ if (!m_config->m_enable_client_telemetry)
+ return;
-std::unique_ptr<TelemetryManager> TelemetryManager::g_instance = nullptr;
-TelemetryManager *TelemetryManager::GetInstance() {
- if (!Config::BuildTimeEnableTelemetry)
- return nullptr;
- return g_instance.get();
+ ClientInfo client_info;
+ client_info.debugger = debugger;
+
+ std::optional<llvm::StringRef> request_name = entry.getString("request_name");
+ if (!request_name.has_value())
+ LLDB_LOG(GetLog(LLDBLog::Object),
+ "Cannot determine request name from client-telemetry entry");
+ else
+ client_info.request_name = request_name->str();
+
+ std::optional<int64_t> start_time = entry.getInteger("start_time");
+ std::optional<int64_t> end_time = entry.getInteger("end_time");
+ SteadyTimePoint epoch;
+ if (!start_time.has_value()) {
+ LLDB_LOG(GetLog(LLDBLog::Object),
+ "Cannot determine start-time from client-telemetry entry");
+ client_info.start_time = 0;
+ } else {
+ client_info.start_time =
+ epoch + std::chrono::nanoseconds(static_cast<size_t>(*start_time));
+ }
+
+ if (!end_time.has_value()) {
+ LLDB_LOG(GetLog(LLDBLog::Object),
+ "Cannot determine end-time from client-telemetry entry");
+ } else {
+ client_info.end_time =
+ epoch + std::chrono::nanoseconds(static_cast<size_t>(*end_time));
+ }
+
+ std::optional<llvm::StringRef> error_msg = entry.getString("error");
+ if (error_msg.has_value())
+ client_info.error_msg = error_msg->str();
+
+ if (llvm::Error er = dispatch(&client_info))
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Object), std::move(er),
+ "Failed to dispatch client telemetry");
}
-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;
+class NoOpTelemetryManager : public TelemetryManager {
+public:
+ llvm::Error preDispatch(llvm::telemetry::TelemetryInfo *entry) override {
+ // Does nothing.
+ return llvm::Error::success();
+ }
+
+ explicit NoOpTelemetryManager()
+ : TelemetryManager(std::make_unique<LLDBConfig>(
+ /*EnableTelemetry*/ false, /*DetailedCommand*/ false)) {}
+
+ llvm::StringRef GetInstanceName() const override {
+ return "NoOpTelemetryManager";
+ }
+
+ llvm::Error dispatch(llvm::telemetry::TelemetryInfo *entry) override {
+ // Does nothing.
+ return llvm::Error::success();
}
- return nullptr;
+ void DispatchClientTelemetry(const lldb_private::StructuredDataImpl &entry,
+ Debugger *debugger) override {
+ // Does nothing.
+ }
+
+ static NoOpTelemetryManager *GetInstance() {
+ static std::unique_ptr<NoOpTelemetryManager> g_ins =
+ std::make_unique<NoOpTelemetryManager>();
+ return g_ins.get();
+ }
+};
+
+std::unique_ptr<TelemetryManager> TelemetryManager::g_instance = nullptr;
+TelemetryManager *TelemetryManager::GetInstance() {
+ // If Telemetry is disabled or if there is no default instance, then use the
+ // NoOp manager. We use a dummy instance to avoid having to do nullchecks in
+ // various places.
+ if (!Config::BuildTimeEnableTelemetry || !g_instance)
+ return NoOpTelemetryManager::GetInstance();
+ return g_instance.get();
}
void TelemetryManager::SetInstance(std::unique_ptr<TelemetryManager> manager) {
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 53c514b790f38..257d6cc09dcc6 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -754,16 +754,19 @@ PacketStatus DAP::GetNextObject(llvm::json::Object &object) {
}
bool DAP::HandleObject(const llvm::json::Object &object) {
+ TelemetryDispatcher dispatcher;
+
const auto packet_type = GetString(object, "type");
if (packet_type == "request") {
const auto command = GetString(object, "command");
-
+ dispatcher.set("request_name", command);
auto new_handler_pos = request_handlers.find(command);
if (new_handler_pos != request_handlers.end()) {
(*new_handler_pos->second)(object);
return true; // Success
}
+ dispatcher.set("error", llvm::Twine("unhandled-command:" + command).str());
if (log)
*log << "error: unhandled command \"" << command.data() << "\""
<< std::endl;
diff --git a/lldb/tools/lldb-dap/LLDBUtils.h b/lldb/tools/lldb-dap/LLDBUtils.h
index a9e13bb3678da..a52f8637a9bb4 100644
--- a/lldb/tools/lldb-dap/LLDBUtils.h
+++ b/lldb/tools/lldb-dap/LLDBUtils.h
@@ -16,6 +16,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/JSON.h"
#include "llvm/Support/raw_ostream.h"
+#include <chrono>
#include <string>
namespace lldb_dap {
@@ -154,6 +155,39 @@ uint32_t GetLLDBFrameID(uint64_t dap_frame_id);
lldb::SBEnvironment
GetEnvironmentFromArguments(const llvm::json::Object &arguments);
+class TelemetryDispatcher {
+public:
+ TelemetryDispatcher(SBDebugger *debugger) {
+ m_telemetry_array =
+ ({"start_time",
+ std::chrono::steady_clock::now().time_since_epoch().count()});
+ this->debugger = debugger;
+ }
+
+ void Set(std::string key, std::string value) {
+ m_telemetry_array.push_back(llvm::json::Value{key, value})
+ }
+
+ void Set(std::string key, int64_t value) {
+ m_telemetry_array.push_back(llvm::json::Value{key, value})
+ }
+
+ ~TelemetryDispatcher() {
+ m_telemetry_array.push_back(
+ {"end_time",
+ std::chrono::steady_clock::now().time_since_epoch().count()});
+ lldb::SBStructuredData telemetry_entry;
+ llvm::json::Value val(std::move(telemetry_array));
+ std::string string_rep = lldb_dap::JSONToString(val);
+ telemetry_entry.SetFromJSON(string_rep.c_str());
+ debugger->DispatchClientTelemetry(telemetry_entry);
+ }
+
+private:
+ llvm::json::Array m_telemetry_array;
+ SBDebugger *debugger;
+};
+
} // namespace lldb_dap
#endif
diff --git a/lldb/unittests/Core/TelemetryTest.cpp b/lldb/unittests/Core/TelemetryTest.cpp
index 0e9f329110872..065a57114181e 100644
--- a/lldb/unittests/Core/TelemetryTest.cpp
+++ b/lldb/unittests/Core/TelemetryTest.cpp
@@ -52,7 +52,7 @@ class FakePlugin : public telemetry::TelemetryManager {
public:
FakePlugin()
: telemetry::TelemetryManager(
- std::make_unique<llvm::telemetry::Config>(true)) {}
+ std::make_unique<telemetry::LLDBConfig>(true, true)) {}
// TelemetryManager interface
llvm::Error preDispatch(llvm::telemetry::TelemetryInfo *entry) override {
|
lldb/include/lldb/Core/Telemetry.h
Outdated
| // If true, we will collect telemetry from LLDB's clients (eg., lldb-dap) via | ||
| // the SB interface. Must also be enabled by the vendor while creating the | ||
| // manager. | ||
| const bool m_enable_client_telemetry; |
There was a problem hiding this comment.
Sorry to chime in but I'm just new to this project and I'm wonder why you effectively need a flag. If the client needs to send manually a telemetry event, isn't this redudant?
|
This flag (and the rest of the flags in this struct) alters the behaviour
of lldb:: TelemetryManager, and not that of the code being monitored.
If this flag is false, telemetry is still sent from client to lldb-server
but the data is simply ignored and not forwarded or stored anywhere.
…On Tue, Mar 4, 2025, 5:47 PM Walter Erquinigo ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lldb/include/lldb/Core/Telemetry.h
<#129728 (comment)>:
> @@ -28,6 +28,17 @@
namespace lldb_private {
namespace telemetry {
+struct LLDBConfig : public ::llvm::telemetry::Config {
+ // If true, we will collect telemetry from LLDB's clients (eg., lldb-dap) via
+ // the SB interface. Must also be enabled by the vendor while creating the
+ // manager.
+ const bool m_enable_client_telemetry;
Sorry to chime in but I'm just new to this project and I'm wonder why you
effectively need a flag. If the client needs to send manually a telemetry
event, isn't this redudant?
—
Reply to this email directly, view it on GitHub
<#129728 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANONE7OIJBLZEIODQAI4S32SYUPBAVCNFSM6AAAAABYJ5F34WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMNJZGI3TKNZRG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@labath @JDevlieghere ping? 🔔 Thanks! |
There was a problem hiding this comment.
I'm not really sure what to think about this. You call it "client telemetry", which sounds generic, but the implementation (mainly, the "request" field) sounds very specific to lldb-dap (not every lldb "client" needs to have a "request", or even if it has, the term "request" may mean something very different for it. I understand why you did that, but it kinda goes against the principle that (lib)lldb shouldn't tie itself to any specific user/client.
Maybe that's okay, maybe not, but I don't feel comfortable approving on my own.
lldb/source/Core/Telemetry.cpp
Outdated
| if (dict->GetValueForKeyAsString("error", error_msg)) { | ||
| client_info.error_msg = error_msg.str(); | ||
| } |
There was a problem hiding this comment.
lldb/tools/lldb-dap/LLDBUtils.h
Outdated
| static std::string JSONToString(const llvm::json::Value &json) { | ||
| std::string data; | ||
| llvm::raw_string_ostream os(data); | ||
| os << json; | ||
| return data; | ||
| } |
There was a problem hiding this comment.
delete, use llvm::to_string instead.
Co-authored-by: Pavel Labath <pavel@labath.sk>
Co-authored-by: Pavel Labath <pavel@labath.sk>
Co-authored-by: Pavel Labath <pavel@labath.sk>
Co-authored-by: Pavel Labath <pavel@labath.sk>
How about renaming it to "client_data"? (We'd record both the request's command and the response's command)
|
Yes, I think that's better. Basically, we're saying "You can give me an opaque string, and I'll forward it. I don't care what it is, it's up to you to figure out what to do with it." It might be a good idea to include something like "client_name" field so you can differentiate (assuming people don't lie) messages coming from lldb-dap from other lldb "clients". So, if you're fine with a stringly-typed API like this, then I think I'm okay as well. I would like @JDevlieghere to have a look at it as well though. |
done |
lldb/source/API/SBDebugger.cpp
Outdated
| // Disable client-telemetry for SWIG. | ||
| // This prevent arbitrary python client (pretty printers, whatnot) from sending | ||
| // telemetry without vendors knowing. | ||
| #ifndef SWIG |
There was a problem hiding this comment.
@labath @JDevlieghere Would this be enough to address concerns about "random"/unknown client code sending telemetry to LLDB-server?
There was a problem hiding this comment.
I think the ifndef should go in the header and guard the whole function so it's not exposed from SWIG. That said, downstream I will replace it with an #if 0 or just remove this method altogether because I don't want anyone external to LLDB to call this.
There was a problem hiding this comment.
Ok, I've defined a ENABLE_CLIENT_TELEMETRY macro at the top of SBDebugger and use the ifdef with it.
(It's currently hard-coded to ! SWIG , might be overkill to make that a build option? )
There was a problem hiding this comment.
Hi, is there any other comment/feedback for this patch? Thanks!
There was a problem hiding this comment.
I have a feeling this is recreating a small version of the #ifdef TELEMETRY hell we've escaped only recently. I can definitely understand the desire/need to restrict access to this API to the world outside (lib)lldb. However, does that have to be achieved by deleting the function declaration? Could it be enough to have the declaration unconditionally, but put the implementation behind some build-time flag (or even a runtime flag that's controlled by the telemetry plugin -- and have the downstream implementation never set it)? (this question is mainly for Jonas. I'm aware you didn't ask for support for this to be added upstream, but I think Vy's attempt to do that demonstrates the kind of problems you're setting yourself up for if you choose that strategy. I think it be better to figure out a different solution, as I think you won't be the only one with that concern.)
Vy: note that this question is independent of the question whether to expose the function to python. ifdef SWIG prevents SWIG from seeing a function (so it doesn't generate bindings for it). The function will always be present in the binary. This is why SWIG guard doesn't have to be applied to function bodies as the swig never reads those. I'm pretty sure this is what Jonas meant when he suggested moving the ifdef to the header -- search for other instances of #ifndef SWIG
There was a problem hiding this comment.
Ah, ok - now that you mentioned it - I agree we should avoid the #ifdef thing on function decl (missing function def and whatnot).
Can we consider going back to my previous impl, which put the ifdef in the function's definition so that the function either does nothing (when client-telemetry is disabled) or actually dispatches the data to server?
But yes, we can modify the ifdef there - rather than conditioning on SWIG, we just make it an explicit ENABLE_CLIENT_TELEMETRY.
Why wasn't that sufficient?
There was a problem hiding this comment.
Soo, my reason for suggesting ifndef SWIG is because that hides the API from python (specifically from python data formatters). While I think it might be reasonable to provide data formatters a way to report their health metrics, I don't think that's a problem you're trying to solve here. I'm also aware that it's an imperfect solution as it makes no difference between "python code running in lldb" and "python code driving lldb". I think the "outside vs. inside" distinction is what we'd really want to capture here since in theory one should be able rewrite lldb-dap in python and have everything still work, but I don't know how to forbid one without the other.
Bottom line: I'm not insisting on the ifndef SWIG, I'm just trying to make sure there's a reasonable story for who can call this API. If you can make your use case work without that, even better.
|
Thanks all, for the feedback! I'll merge this EOD today, if there's no objection! :) |
This patch is slightly different from other impl in that we dispatch client-telemetry via a different helper method. This is to make it easier for vendor to opt-out (simply by overriding the method to do nothing). There is also a configuration option to disallow collecting client telemetry.