[lldb]Implement LLDB Telemetry #98528

oontvoo · 2024-07-11T19:36:54Z

Provide some concrete implementation for llvm/Telemetry in lldb/Telemetry

(Dependent on PR/102323

nikic · 2024-07-11T19:58:18Z

Why does any part of this need to be in llvm/ at all?

oontvoo · 2024-07-12T11:44:38Z

Why does any part of this need to be in llvm/ at all?

Please see comments on the rfc at https://discourse.llvm.org/t/rfc-lldb-telemetry-metrics/64588/15

TL;DR : A few people feel that Telemetry is not something unique to LLDB and that other tools might be interested in having it too. So rather than having mutliple Telemetry libs doing similar things, it might be good to have at least a common API

labath

I've made one pass over the PR. I think this would be easier to review if it was split into multiple patches:

core llvm infrastructure
core lldb infrastructure
~one patch per event type

I think the biggest hurdle will be finding someone to approve the addition of the new llvm library (I doubt it's going to be someone from the current reviewer set). I wouldn't be surprised if this ends up needing its own RFC.

labath · 2024-07-24T07:02:59Z

lldb/include/lldb/API/SBDebugger.h

@@ -9,10 +9,12 @@
 #ifndef LLDB_API_SBDEBUGGER_H
 #define LLDB_API_SBDEBUGGER_H

+#include <chrono>


Is this used anywhere?

labath · 2024-07-24T07:06:13Z

lldb/include/lldb/API/SBDebugger.h

@@ -245,6 +247,8 @@ class LLDB_API SBDebugger {

  lldb::SBTarget GetDummyTarget();

+  void SendTelemetry(SBStructuredData *entry);


Suggested change

void SendTelemetry(SBStructuredData *entry);

void SendTelemetry(const SBStructuredData &entry);

lldb/include/lldb/Core/Telemetry.h

labath · 2024-07-24T07:16:26Z

lldb/include/lldb/Core/Telemetry.h

+// Logger configs: LLDB users can also supply their own configs via:
+// $HOME/.lldb_telemetry_config
+//
+// We can propose simple syntax: <field_name><colon><value>


Since this is actual code, it should talk about the (real) syntax, not the proposed one.

lldb/source/Core/Telemetry.cpp

labath · 2024-07-24T08:24:39Z

lldb/source/Core/Telemetry.cpp

+
+  auto *ret = new llvm::telemetry::TelemetryConfig{enable_telemetry,
+                                                   additional_destinations};
+#ifdef HAS_VENDOR_TELEMETRY_CONFIG


I'd probably do this by providing dummy/empty implementations of these functions when there's no vendor customization.

can you clarify what you mean by dummy implementations? (w/o causing either duplicate-symbols errors) I'd tried the weak-def but it wasn't very portable, so I thought this was the simplest.

I just meant that we could conditionally provide an empty implementation of the "vendor" function instead of conditionally calling it. So something like:

#if VENDOR // do something to get the vendor definition #else // provide an empty definition ourselves, e.g: void ApplyVendorSpecificConfigs(...) {} #endif

Then you don't need the ifdef at the call site.

Done - reworked this to use Plugin registry (the default plugin is enabled for test, then vendors can add their own impl as needed)

lldb/source/Interpreter/CommandInterpreter.cpp

lldb/tools/lldb-dap/DAP.cpp

oontvoo · 2024-08-07T14:29:13Z

I've made one pass over the PR. I think this would be easier to review if it was split into multiple patches:

core llvm infrastructure

core lldb infrastructure

~one patch per event type

I think the biggest hurdle will be finding someone to approve the addition of the new llvm library (I doubt it's going to be someone from the current reviewer set). I wouldn't be surprised if this ends up needing its own RFC.

Good point - so here's the plane:

I'll create a separate PR for the first bullet point (llvm core infra).
keep this PR as is - since branching is such a pain (and controversial) in llvm, you can "pretend" to not see the llvm/lib/ changes in this PR . hopefully the diff will disappear once the first PR goes in.

Objective: - Provide a common framework in LLVM for collecting various usage metrics - Characteristics: + Extensible and configurable by: * tools in LLVM that want to use it * vendors in their downstream codebase * tools users (as allowed by vendor) Background: The framework was originally proposed only for LLDB, but there were quite a few requests that it should be moved to llvm/lib given telemetry is a common usage to a lot of tools, not just LLDB. See more details on the design and discussions here on the RFC: https://discourse.llvm.org/t/rfc-lldb-telemetry-metrics/64588/20?u=oontvoo

Provide the concrete implementation for telemetry in LLDB. (This is follow-up patch to PR/102323)

lldb/tools/lldb-dap/DAP.cpp

… testing

labath

I think it'd be good to separate the general mechanics of creating and managing a telemetry (telemeter) instance from the details of logging individual telemetry entries. I.e. one PR for the general infrastructure (maybe include one simple (startup?) telemetry entry, if you think it's necessary to make a point), and then one PR for each of the entries.

The plugin infrastructure seems very.. baroque. Why do we need the intermediate TelemetryVendor class? And why a whole list of them?

The reason we support registering more than one e.g. SymbolFile plugin is that we can choose which one to use at runtime (based on which file the user opens, etc.). Do we want to do something like that here? What would that even mean?

To create a basic indirection (so that the vendor implementation can be decoupled from the rest of lldb), it sounds to me like all one need is one function pointer (the factory function for the "telemeter"), a way to set it from inside the plugin (PluginManager::SetTelemetryPlugin ?) and then a way to call it (another function?)

labath · 2024-09-25T13:15:11Z

lldb/include/lldb/lldb-enumerations.h

@@ -257,8 +257,8 @@ enum StopReason {
 };

 /// Command Return Status Types.
-enum ReturnStatus {
-  eReturnStatusInvalid,
+enum ReturnStatus : int {


How does this relate to the rest of the patch?

This is needed to serialize the ReturnStatus from a command.

labath · 2024-09-25T13:17:47Z

lldb/source/Core/Telemetry.cpp

+static std::string
+ExitDescToString(const llvm::telemetry::ExitDescription *desc) {
+  return ("ExitCode:" + desc->ExitCode) +
+         (" ExixitDescription: " + desc->Description);


Suggested change

(" ExixitDescription: " + desc->Description);

(" ExitDescription: " + desc->Description);

labath · 2024-09-25T13:21:39Z

lldb/source/Core/Telemetry.cpp

+    return std::to_string((stats.End.value() - stats.Start).count()) +
+           "(nanosec)";


Suggested change

return std::to_string((stats.End.value() - stats.Start).count()) +

"(nanosec)";

return llvm::formatv("{0:ns}", stats.End.value() - stats.Start).str();

(or maybe some other unit (ms/us) -- how small do we expect these to get)?

lldb/source/Core/Telemetry.cpp

oontvoo requested review from labath and cmtice July 11, 2024 19:36

oontvoo self-assigned this Jul 11, 2024

oontvoo changed the title ~~[DRAFT][llvm]Added lib/Telemetry~~ [llvm]Added lib/Telemetry Jul 23, 2024

oontvoo assigned labath Jul 23, 2024

labath reviewed Jul 24, 2024

View reviewed changes

oontvoo changed the title ~~[llvm]Added lib/Telemetry~~ [lldb]Implement LLDB Telemetry Aug 7, 2024

oontvoo added 18 commits August 7, 2024 13:21

add header

6e43f67

fixed typo

e25f5fc

Merge branch 'llvm:main' into llvm_telemetry

ad3906c

Merge branch 'llvm:main' into llvm_telemetry

24d07d6

add tests and addressed review comments

0057bcf

formatting changes

750e4ac

more formatting

1378ed4

more formatting changes

02e750e

Added header comment to describe the package

63e99fc

reformat header

fa88512

addressed review comments and added separate docs in llvm/docs/

0866f64

updated field names in tests

a8523f7

reformated doc and added additional details

47e8b06

fix formatting

690c6ab

details

db668f4

formatted TelemetryTest.cpp

0290a14

fix formatting in docs

14b5234

convert comments to doxygen style

6ca87e5

oontvoo mentioned this pull request Sep 4, 2024

[llvm]Add a simple Telemetry framework #102323

Merged

oontvoo added 6 commits September 4, 2024 15:14

fix doc toc

4155add

group all the testing-params into a Context struct

11ead4a

fix conversion warnings

48228ee

Merge branch 'llvm:main' into llvm_telemetry

ed8a3f1

finish up todos

990e1ca

[lldb]Implement LLDB Telemetry :

42781f7

Provide the concrete implementation for telemetry in LLDB. (This is follow-up patch to PR/102323)

oontvoo force-pushed the telemetry_shared branch from d290895 to 42781f7 Compare September 13, 2024 12:06

change the data type from json to structureddata

04b7da8

oontvoo commented Sep 17, 2024

View reviewed changes

lldb/tools/lldb-dap/DAP.cpp Show resolved Hide resolved

oontvoo added 2 commits September 23, 2024 20:49

Rework the default impl of Telemetry in LLDB as "Plugin" to allow for…

0e49b93

… testing

Merge branch 'llvm:main' into telemetry_shared

10b5f46

oontvoo marked this pull request as ready for review September 24, 2024 14:33

oontvoo requested a review from JDevlieghere as a code owner September 24, 2024 14:33

labath reviewed Sep 25, 2024

View reviewed changes

UPdate LLDB implementation to match changes in LLVM's telemetry

f85900d

oontvoo force-pushed the telemetry_shared branch from dbef2be to f85900d Compare December 11, 2024 16:19

oontvoo added 5 commits December 11, 2024 15:02

fix vtable issue

66b77aa

more renaming for consistency

67a0ed0

remove unused

f7f7004

addressed review comments

949b994

formatting

1eeb6f9

		@@ -245,6 +247,8 @@ class LLDB_API SBDebugger {

		lldb::SBTarget GetDummyTarget();

		void SendTelemetry(SBStructuredData *entry);

	void SendTelemetry(SBStructuredData *entry);
	void SendTelemetry(const SBStructuredData &entry);

	(" ExixitDescription: " + desc->Description);
	(" ExitDescription: " + desc->Description);

		return std::to_string((stats.End.value() - stats.Start).count()) +
		"(nanosec)";

	return std::to_string((stats.End.value() - stats.Start).count()) +
	"(nanosec)";
	return llvm::formatv("{0:ns}", stats.End.value() - stats.Start).str();

[lldb]Implement LLDB Telemetry #98528

Are you sure you want to change the base?

[lldb]Implement LLDB Telemetry #98528

Uh oh!

Conversation

oontvoo commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Jul 11, 2024

Uh oh!

oontvoo commented Jul 12, 2024

Uh oh!

labath left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oontvoo commented Aug 7, 2024

Uh oh!

Uh oh!

labath left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oontvoo commented Jul 11, 2024 •

edited

Loading