-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[LLDB][Telemetry]Define DebuggerTelemetryInfo and related methods #127696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
24e9f78
496c370
11efc09
5aec284
4b6a4bd
83bb7c4
4dd0782
7ad8360
d7b1275
302552a
c00cd54
597185f
41f6649
169449b
7420811
0fb078d
68f95ab
d586956
8b7afd6
21b8bdf
7776ea1
c70bd5d
bcd793e
4ba0431
6027a69
c011531
a9fd8ad
17ced2e
d07dc38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,8 +11,10 @@ | |
|
||
#include "lldb/Core/StructuredDataImpl.h" | ||
#include "lldb/Interpreter/CommandReturnObject.h" | ||
#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" | ||
|
@@ -22,13 +24,20 @@ | |
#include <memory> | ||
#include <optional> | ||
#include <string> | ||
#include <unordered_map> | ||
|
||
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 BaseInfo = 0b11000000; | ||
static const llvm::telemetry::KindType DebuggerInfo = 0b11000100; | ||
}; | ||
|
||
/// Defines a convenient type for timestamp of various events. | ||
|
@@ -41,6 +50,7 @@ struct LLDBBaseTelemetryInfo : public llvm::telemetry::TelemetryInfo { | |
std::optional<SteadyTimePoint> end_time; | ||
// TBD: could add some memory stats here too? | ||
|
||
lldb::user_id_t debugger_id = LLDB_INVALID_UID; | ||
Debugger *debugger; | ||
|
||
// For dyn_cast, isa, etc operations. | ||
|
@@ -56,26 +66,93 @@ struct LLDBBaseTelemetryInfo : public llvm::telemetry::TelemetryInfo { | |
void serialize(llvm::telemetry::Serializer &serializer) const override; | ||
}; | ||
|
||
struct DebuggerInfo : public LLDBBaseTelemetryInfo { | ||
std::string lldb_version; | ||
|
||
bool is_exit_entry = false; | ||
|
||
DebuggerInfo() = default; | ||
|
||
llvm::telemetry::KindType getKind() const override { | ||
return LLDBEntryKind::DebuggerInfo; | ||
} | ||
|
||
static bool classof(const llvm::telemetry::TelemetryInfo *T) { | ||
// Subclasses of this is also acceptable | ||
return (T->getKind() & LLDBEntryKind::DebuggerInfo) == | ||
LLDBEntryKind::DebuggerInfo; | ||
} | ||
|
||
void serialize(llvm::telemetry::Serializer &serializer) const override; | ||
}; | ||
|
||
/// The base Telemetry manager instance in LLDB. | ||
/// This class declares additional instrumentation points | ||
/// applicable to LLDB. | ||
class TelemetryManager : public llvm::telemetry::Manager { | ||
public: | ||
llvm::Error preDispatch(llvm::telemetry::TelemetryInfo *entry) override; | ||
|
||
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); | ||
|
||
static void SetInstance(std::unique_ptr<TelemetryManager> manger); | ||
|
||
private: | ||
std::unique_ptr<llvm::telemetry::Config> m_config; | ||
// Each instance of a TelemetryManager is assigned a unique ID. | ||
const std::string m_id; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we store this as a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What use case do you have in mind? Right now this field is read-only (copied onto multiple entry objects). So it seems jsut storing a string directly seems more straightforward. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Putting the generation code into the UUID class seems reasonable. I'm fairly ambivalent about the storage format. One reason for storing it in the UUID form might be if we think that someone could be interested in storing/transmitting the UUID in a binary form (because it's more compact?). (One thing complicating that is the fact that you're currently appending the timestamp to the string. I don't think that's necessary because the random data should be unique enough, and the (unlikely) case of a missing random source could be handled by initializing a pseudo-random generator with the timestamp and then using that to generate a UUID) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is the TelemetryManager - it is never transmitted or serialised anywhere. So why does compactness matter? If you mean storing the id as UUID in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I mean that, but yes, the fact that this is in the llvm base class make this tricky. I think this is fine then. |
||
|
||
static std::unique_ptr<TelemetryManager> g_instance; | ||
}; | ||
|
||
/// Helper RAII class for collecting telemetry. | ||
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, | ||
Debugger *debugger = nullptr) | ||
: m_callback(std::move(callback)) { | ||
// Start the timer. | ||
m_start_time = std::chrono::steady_clock::now(); | ||
m_info.debugger = debugger; | ||
} | ||
|
||
void SetDebugger(Debugger *debugger) { m_info.debugger = debugger; } | ||
|
||
~ScopedDispatcher() { | ||
// If Telemetry is disabled (either at buildtime or runtime), | ||
// then don't do anything. | ||
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(); | ||
// The callback will set the remaining fields. | ||
m_callback(&m_info); | ||
// And then we dispatch. | ||
if (llvm::Error er = manager->dispatch(&m_info)) { | ||
LLDB_LOG_ERROR(GetLog(LLDBLog::Object), std::move(er), | ||
"Failed to dispatch entry of type: {0}", m_info.getKind()); | ||
} | ||
} | ||
|
||
private: | ||
SteadyTimePoint m_start_time; | ||
llvm::unique_function<void(Info *info)> m_callback; | ||
Info m_info; | ||
}; | ||
|
||
} // namespace telemetry | ||
} // namespace lldb_private | ||
#endif // LLDB_CORE_TELEMETRY_H |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -17,6 +17,7 @@ | |||||||||||||||||||||||||||
#include "lldb/Core/PluginManager.h" | ||||||||||||||||||||||||||||
#include "lldb/Core/Progress.h" | ||||||||||||||||||||||||||||
#include "lldb/Core/StreamAsynchronousIO.h" | ||||||||||||||||||||||||||||
#include "lldb/Core/Telemetry.h" | ||||||||||||||||||||||||||||
#include "lldb/DataFormatters/DataVisualization.h" | ||||||||||||||||||||||||||||
#include "lldb/Expression/REPL.h" | ||||||||||||||||||||||||||||
#include "lldb/Host/File.h" | ||||||||||||||||||||||||||||
|
@@ -52,6 +53,7 @@ | |||||||||||||||||||||||||||
#include "lldb/Utility/State.h" | ||||||||||||||||||||||||||||
#include "lldb/Utility/Stream.h" | ||||||||||||||||||||||||||||
#include "lldb/Utility/StreamString.h" | ||||||||||||||||||||||||||||
#include "lldb/Version/Version.h" | ||||||||||||||||||||||||||||
#include "lldb/lldb-enumerations.h" | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
#if defined(_WIN32) | ||||||||||||||||||||||||||||
|
@@ -62,13 +64,15 @@ | |||||||||||||||||||||||||||
#include "llvm/ADT/STLExtras.h" | ||||||||||||||||||||||||||||
#include "llvm/ADT/StringRef.h" | ||||||||||||||||||||||||||||
#include "llvm/ADT/iterator.h" | ||||||||||||||||||||||||||||
#include "llvm/Config/llvm-config.h" | ||||||||||||||||||||||||||||
#include "llvm/Support/DynamicLibrary.h" | ||||||||||||||||||||||||||||
#include "llvm/Support/FileSystem.h" | ||||||||||||||||||||||||||||
#include "llvm/Support/Process.h" | ||||||||||||||||||||||||||||
#include "llvm/Support/ThreadPool.h" | ||||||||||||||||||||||||||||
#include "llvm/Support/Threading.h" | ||||||||||||||||||||||||||||
#include "llvm/Support/raw_ostream.h" | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
#include <chrono> | ||||||||||||||||||||||||||||
#include <cstdio> | ||||||||||||||||||||||||||||
#include <cstdlib> | ||||||||||||||||||||||||||||
#include <cstring> | ||||||||||||||||||||||||||||
|
@@ -761,7 +765,14 @@ void Debugger::InstanceInitialize() { | |||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
DebuggerSP Debugger::CreateInstance(lldb::LogOutputCallback log_callback, | ||||||||||||||||||||||||||||
void *baton) { | ||||||||||||||||||||||||||||
lldb_private::telemetry::ScopedDispatcher< | ||||||||||||||||||||||||||||
lldb_private::telemetry::DebuggerInfo> | ||||||||||||||||||||||||||||
helper([](lldb_private::telemetry::DebuggerInfo *entry) { | ||||||||||||||||||||||||||||
entry->lldb_version = lldb_private::GetVersion(); | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
DebuggerSP debugger_sp(new Debugger(log_callback, baton)); | ||||||||||||||||||||||||||||
helper.SetDebugger(debugger_sp.get()); | ||||||||||||||||||||||||||||
Comment on lines
+768
to
+774
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is definitely better, but lambdas tend to add some boilerplate of their own. They may be useful in some cases (if you need to set some field at the very end) but I think that in many cases (this one included) we could just set the field directly, so I want to float this idea: I think this would be shorter if we had the dispatcher object expose the telemetry entry it is managing directly:
Suggested change
(and if we do need a callback, I don't see a reason why this couldn't coexist with that) Any thoughts @JDevlieghere ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer not exposing the managed TelemetryInfo because in the next PR, i'm going to move it to being a local var in the execute function. (Reasons being I need to dispatch/execute more than one callbacks - one at the beginning and one at the end of the function. ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On a second thought, how about we make a "NoOp" TelemetryManager that does not do anything in case when Telemetry is disabled? That way, we wouldn't need to check for the flag/ptr. |
||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
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); | ||||||||||||||||||||||||||||
|
@@ -987,6 +998,12 @@ void Debugger::Clear() { | |||||||||||||||||||||||||||
// static void Debugger::Destroy(lldb::DebuggerSP &debugger_sp); | ||||||||||||||||||||||||||||
// static void Debugger::Terminate(); | ||||||||||||||||||||||||||||
llvm::call_once(m_clear_once, [this]() { | ||||||||||||||||||||||||||||
telemetry::ScopedDispatcher<telemetry::DebuggerInfo> helper( | ||||||||||||||||||||||||||||
[this](lldb_private::telemetry::DebuggerInfo *info) { | ||||||||||||||||||||||||||||
assert(this == info->debugger); | ||||||||||||||||||||||||||||
info->is_exit_entry = true; | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
this); | ||||||||||||||||||||||||||||
ClearIOHandlers(); | ||||||||||||||||||||||||||||
StopIOHandlerThread(); | ||||||||||||||||||||||||||||
StopEventHandlerThread(); | ||||||||||||||||||||||||||||
|
Uh oh!
There was an error while loading. Please reload this page.