-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[llvm]Add a simple Telemetry framework #102323
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 42 commits
dbb8b15
6e43f67
e25f5fc
ad3906c
24d07d6
0057bcf
750e4ac
1378ed4
02e750e
63e99fc
fa88512
0866f64
a8523f7
47e8b06
690c6ab
db668f4
0290a14
14b5234
6ca87e5
4155add
11ead4a
48228ee
ed8a3f1
990e1ca
479cd13
6d8aee2
3a17dbb
a6a6c7b
f30dec6
fd3da20
60616ef
8c0ac5a
c8be829
1393c1f
eb07577
0376abc
e2f7c23
17dfac7
1ea0906
39fd0e7
67b7688
efd25d8
4a8276f
a16344b
26ee5eb
b766e3c
c8cddab
5f8d65f
dffeacd
398ed3e
e462908
df8a9af
70f4742
8eab77a
f9e1a65
2a1fbe5
6e3a85d
f9b1cce
0f38a74
ad57099
a2fcd4d
628e7e9
6ea4e92
08cf32f
3c52401
07f06c0
06e746e
2476294
5ce406c
fd57d06
a8a878b
54eeaba
32dfc6a
0893c5b
155f243
b255e3a
14d1c92
bf2172d
c2dcb7d
bbe9009
e2036c6
05947d5
f1100bb
2f64b29
a5df7a0
585fee8
3812cda
0cdc240
da8056f
c116074
a4e2799
2763d46
75c1b4b
9780ec7
4715743
d231bf2
1941b1f
bd3df5e
4351b94
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 |
---|---|---|
@@ -0,0 +1,214 @@ | ||
=========================== | ||
Telemetry framework in LLVM | ||
=========================== | ||
|
||
.. contents:: | ||
:local: | ||
|
||
.. toctree:: | ||
:hidden: | ||
|
||
=========================== | ||
Telemetry framework in LLVM | ||
=========================== | ||
|
||
Objective | ||
========= | ||
|
||
Provides a common framework in LLVM for collecting various usage and performance | ||
metrics. | ||
It is located at `llvm/Telemetry/Telemetry.h` | ||
|
||
Characteristics | ||
--------------- | ||
* Configurable and extensible by: | ||
|
||
* Tools: any tool that wants to use Telemetry can extend and customize it. | ||
* Vendors: Toolchain vendors can also provide custom implementation of the | ||
library, which could either override or extend the given tool's upstream | ||
implementation, to best fit their organization's usage and privacy models. | ||
* End users of such tool can also configure Telemetry (as allowed by their | ||
vendor). | ||
|
||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Important notes | ||
---------------- | ||
oontvoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
* There is no concrete implementation of a Telemetry library in upstream LLVM. | ||
We only provide the abstract API here. Any tool that wants telemetry will | ||
implement one. | ||
|
||
The rationale for this is that, all the tools in LLVM are very different in | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
what they care about (what/where/when to instrument data). Hence, it might not | ||
be practical to have a single implementation. | ||
However, in the future, if we see enough common pattern, we can extract them | ||
into a shared place. This is TBD - contributions are welcomed. | ||
oontvoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
* No implementation of Telemetry in upstream LLVM shall store any of the | ||
collected data due to privacy and security reasons: | ||
|
||
* Different organizations have different privacy models: | ||
|
||
* Which data is sensitive, which is not? | ||
* Whether it is acceptable for instrumented data to be stored anywhere? | ||
(to a local file, what not?) | ||
|
||
* Data ownership and data collection consents are hard to accommodate from | ||
LLVM developers' point of view: | ||
|
||
* Eg., data collected by Telemetry is not neccessarily owned by the user | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
of an LLVM tool with Telemetry enabled, hence the user's consent to data | ||
collection is not meaningful. On the other hand, LLVM developers have no | ||
reasonable ways to request consent from the "real" owners. | ||
|
||
|
||
High-level design | ||
================= | ||
|
||
Key components | ||
-------------- | ||
|
||
The framework consists of four important classes: | ||
|
||
* `llvm::telemetry::Telemeter`: The class responsible for collecting and | ||
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. RST uses double backticks for inline literals. If you click on "Display the rich diff" GitHub will show you the rendered version of the RST file. 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. done |
||
transmitting telemetry data. This is the main point of interaction between the | ||
framework and any tool that wants to enable telemery. | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* `llvm::telemetry::TelemetryInfo`: Data courier | ||
* `llvm::telemetry::Destination`: Data sink to which the Telemetry framework | ||
sends data. | ||
Its implementation is transparent to the framework. | ||
It is up to the vendor to decide which pieces of data to forward and where | ||
to forward them to their final storage. | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* `llvm::telemetry::Config`: Configurations on the `Telemeter`. | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
.. image:: llvm_telemetry_design.png | ||
|
||
How to implement and interact with the API | ||
------------------------------------------ | ||
|
||
To use Telemetry in your tool, you need to provide a concrete implementation of the `Telemeter` class and `Destination`. | ||
|
||
1) Define a custom `Telemeter` and `Destination` | ||
|
||
.. code-block:: c++ | ||
|
||
// This destination just prints the given entry to a stdout. | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// In "real life", this would be where you forward the data to your | ||
// custom data storage. | ||
class MyStdoutDestination : public llvm::telemetry::Destination { | ||
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. Nit: you qualify |
||
public: | ||
Error emitEntry(const TelemetryInfo* Entry) override { | ||
return sendToBlackBox(Entry); | ||
|
||
} | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
private: | ||
Error sendToBlackBox(const TelemetryInfo* Entry) { | ||
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'm not following the benefit of the extra function here? Could the contents of this function just be inlined? Also, since this is code in our documentation, it should probably follow LLVM coding standards (specifically here, 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. Sure - i was just trying to demonstrate that the data can be forwarded to somewhere else ... 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 think if the comment is inlined into 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. done - simplified the example |
||
// This could send the data anywhere. | ||
// But we're simply sending it to stdout for the example. | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
llvm::outs() << entryToString(Entry) << "\n"; | ||
return llvm::success(); | ||
} | ||
|
||
std::string entryToString(const TelemetryInfo* Entry) { | ||
// make a string-representation of the given entry. | ||
} | ||
}; | ||
|
||
// This defines a custom TelemetryInfo that has an addition Msg field. | ||
struct MyTelemetryInfo : public llvm::telemetry::TelemetryInfo { | ||
std::string Msg; | ||
|
||
json::Object serializeToJson() const { | ||
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 should have 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. done -(the function will actually be removed ) |
||
json::Object Ret = TelemeteryInfo::serializeToJson(); | ||
Ret.emplace_back("MyMsg", Msg); | ||
return std::move(Ret); | ||
} | ||
|
||
// TODO: implement getKind() and classof() to support dyn_cast operations. | ||
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. Is this intended as additional instructions to someone using this class as a base, or something to come back to before merging? 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. Yes, it is supposed to be instructions to users to define these. 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. Okay, thanks. To me "TODO" seemed more like something to tackle before merging. I'm okay with you leaving it like this, if you want, but would personally write the function definition, but leave the body blank with a comment like this or something: |
||
}; | ||
|
||
class MyTelemeter : public llvm::telemery::Telemeter { | ||
public: | ||
static std::unique_ptr<MyTelemeter> createInstatnce(llvm::telemetry::Config* config) { | ||
// If Telemetry is not enabled, then just return null; | ||
if (!config->EnableTelemetry) return nullptr; | ||
|
||
std::make_unique<MyTelemeter>(); | ||
} | ||
MyTelemeter() = default; | ||
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. Is this actually needed? |
||
|
||
void logStartup(llvm::StringRef ToolName, TelemetryInfo* Entry) override { | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (MyTelemetryInfo* M = dyn_cast<MyTelemetryInfo>(Entry)) { | ||
M->Msg = "Starting up tool with name: " + ToolName; | ||
emitToAllDestinations(M); | ||
} else { | ||
emitToAllDestinations(Entry); | ||
} | ||
} | ||
|
||
void logExit(llvm::StringRef ToolName, TelemetryInfo* Entry) override { | ||
if (MyTelemetryInfo* M = dyn_cast<MyTelemetryInfo>(Entry)) { | ||
M->Msg = "Exitting tool with name: " + ToolName; | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
emitToAllDestinations(M); | ||
} else { | ||
emitToAllDestinations(Entry); | ||
} | ||
} | ||
|
||
void addDestination(Destination* dest) override { | ||
destinations.push_back(dest); | ||
} | ||
|
||
// You can also define additional instrumentation points.) | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
void logAdditionalPoint(TelemetryInfo* Entry) { | ||
// .... code here | ||
} | ||
private: | ||
void emitToAllDestinations(const TelemetryInfo* Entry) { | ||
// Note: could do this in parallel, if needed. | ||
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'd suggest dropping this note: ultimately the end implementation will determine parallelism or not, and I think that's somewhat obvious, given that there isn't really any real upstream implementation involved here. |
||
for (Destination* Dest : Destinations) | ||
Dest->emitEntry(Entry); | ||
} | ||
std::vector<Destination> Destinations; | ||
}; | ||
|
||
2) Use the library in your tool. | ||
|
||
Logging the tool init-process: | ||
|
||
.. code-block:: c++ | ||
|
||
// At tool's init code | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
auto StartTime = std::chrono::time_point<std::chrono::steady_clock>::now(); | ||
llvm::telemetry::Config MyConfig = makeConfig(); // build up the appropriate Config struct here. | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
auto Telemeter = MyTelemeter::createInstance(&MyConfig); | ||
std::string CurrentSessionId = ...; // Make some unique ID corresponding to the current session here. | ||
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. Honestly, I think this should be 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. Giving this some more thought, perhaps there should be a default implementation upstream to generate the session ID that is extensible in some manner. I imagine that the session IDs for some people might want to be something different to UUIDs. What do you think? 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. Something like 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. Yeah, or even have 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. done - Added this on the Config |
||
|
||
// Any other tool's init code can go here | ||
oontvoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// ... | ||
|
||
// Finally, take a snapshot of the time now so we know how long it took the | ||
// init process to finish | ||
oontvoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
auto EndTime = std::chrono::time_point<std::chrono::steady_clock>::now(); | ||
MyTelemetryInfo Entry; | ||
Entry.SessionId = CurrentSessionId ; // Assign some unique ID here. | ||
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. Related to my previous comment, I think this should be done within 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. done - the |
||
Entry.Stats = {StartTime, EndTime}; | ||
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. There is no |
||
Telemeter->logStartup("MyTool", &Entry); | ||
|
||
Similar code can be used for logging the tool's exit. | ||
|
||
oontvoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Additionall, at any other point in the tool's lifetime, it can also log telemetry: | ||
labath marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
.. code-block:: c++ | ||
|
||
// At some execution point: | ||
auto StartTime = std::chrono::time_point<std::chrono::steady_clock>::now(); | ||
|
||
// ... other events happening here | ||
|
||
auto EndTime = std::chrono::time_point<std::chrono::steady_clock>::now(); | ||
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. Nit: indentation here and the lines below is inconsistent. |
||
MyTelemetryInfo Entry; | ||
Entry.SessionId = CurrentSessionId ; // Assign some unique ID here. | ||
Entry.Stats = {StartTime, EndTime}; | ||
Telemeter->logAdditionalPoint(&Entry); |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,136 @@ | ||||||||||
//===- llvm/Telemetry/Telemetry.h - Telemetry -------------------*- C++ -*-===// | ||||||||||
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. Nit: coding standards have very recently changed such that 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. what does the new header look like? (I've only seen this format ....) 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. 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. done |
||||||||||
// | ||||||||||
// 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 | ||||||||||
// | ||||||||||
//===----------------------------------------------------------------------===// | ||||||||||
/// | ||||||||||
/// \file | ||||||||||
/// This file provides the basic framework for Telemetry | ||||||||||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
oontvoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
/// | ||||||||||
/// It comprises of three important structs/classes: | ||||||||||
/// | ||||||||||
/// - Telemeter: The class responsible for collecting and forwarding | ||||||||||
/// telemery data. | ||||||||||
/// - TelemetryInfo: data courier | ||||||||||
/// - TelemetryConfig: this stores configurations on Telemeter. | ||||||||||
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 omit this and make 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. done |
||||||||||
/// | ||||||||||
/// Refer to its documentation at llvm/docs/Telemetry.rst for more details. | ||||||||||
//===---------------------------------------------------------------------===// | ||||||||||
|
||||||||||
#ifndef LLVM_TELEMETRY_TELEMETRY_H | ||||||||||
#define LLVM_TELEMETRY_TELEMETRY_H | ||||||||||
|
||||||||||
oontvoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
#include <chrono> | ||||||||||
#include <ctime> | ||||||||||
#include <memory> | ||||||||||
#include <optional> | ||||||||||
#include <string> | ||||||||||
|
||||||||||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
#include "llvm/ADT/StringExtras.h" | ||||||||||
#include "llvm/ADT/StringRef.h" | ||||||||||
#include "llvm/Support/Error.h" | ||||||||||
#include "llvm/Support/JSON.h" | ||||||||||
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've not looked at the other headers, but I believe at least JSON.h isn't needed now? 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. done |
||||||||||
|
||||||||||
namespace llvm { | ||||||||||
namespace telemetry { | ||||||||||
|
||||||||||
/// Configuration for the Telemeter class. | ||||||||||
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 comment needs updating for the new class name. 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. done |
||||||||||
/// This stores configurations from both users and vendors and is passed | ||||||||||
/// to the Telemeter upon construction. (Any changes to the config after | ||||||||||
/// the Telemeter's construction will not have effect on it). | ||||||||||
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.
Suggested change
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. done |
||||||||||
/// | ||||||||||
/// This struct can be extended as needed to add additional configuration | ||||||||||
/// points specific to a vendor's implementation. | ||||||||||
struct Config { | ||||||||||
// If true, telemetry will be enabled. | ||||||||||
bool EnableTelemetry; | ||||||||||
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. Related to my comment below about the mechanism for setting this being vendor-specific, perhaps this should be an abstract method 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.
Short answer to your question: Yes, definitely. The idea is to have a In the Does that address your concern? 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. Sort of, but not entirely. Two thoughts:
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.
Eg.:
|
||||||||||
|
||||||||||
// Implementation-defined names of additional destinations to send | ||||||||||
// telemetry data (Could be stdout, stderr, or some local paths, etc). | ||||||||||
// | ||||||||||
// These strings will be interpreted by the vendor's code. | ||||||||||
// So the users must pick the from their vendor's pre-defined | ||||||||||
// set of Destinations. | ||||||||||
std::vector<std::string> AdditionalDestinations; | ||||||||||
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. Shouldn't this contain a list of 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. No, this field intended to contain the description of the Destinations. In other words, the Because 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'm beginning to think that this field doesn't belong in the generic upstream implementation. I think we can all agree that a field for enabling telemetry is common, though I also think it's fair to say that the mechanism for setting it will be vendor-specific. However, the destinations feel entirely like they should be vendor side, given we're not providing any concrete destinations in the upstream implementation. For each destination a vendor defines, they'll either have it enabled by default, or disabled by default, with a user-facing option to enable it. It may be the case that they have NO destinations that are user-configurable, in which case this field is entirely dead weight. Even if it is configurable by the user somehow, it would seem like the logical way of doing this will be implementation dependent and therefore should be part of the vendor's 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. +1 to @jh7370 comment. I don't think this belongs here. 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. sure, can remove it. |
||||||||||
}; | ||||||||||
|
||||||||||
/// For isa, dyn_cast, etc operations on TelemetryInfo. | ||||||||||
typedef unsigned KindType; | ||||||||||
/// This struct is used by TelemetryInfo to support isa<>, dyn_cast<> | ||||||||||
/// operations. | ||||||||||
/// It is defined as a struct (rather than an enum) because it is | ||||||||||
/// expectend to be extended by subclasses which may have | ||||||||||
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.
Suggested change
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. done |
||||||||||
/// additional TelemetryInfo types defined to describe different events. | ||||||||||
struct EntryKind { | ||||||||||
static const KindType Base = 0; | ||||||||||
}; | ||||||||||
|
||||||||||
/// TelemetryInfo is the data courier, used to move instrumented data | ||||||||||
/// from the tool being monitored to the Telemery framework. | ||||||||||
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.
Suggested change
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. done |
||||||||||
/// | ||||||||||
/// This base class contains only the basic set of telemetry data. | ||||||||||
/// Downstream implementations can add more fields as needed to describe | ||||||||||
/// additional events. | ||||||||||
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. How about this:
Suggested change
I'm suggesting this, because in our downstream implementation, we have lots of event types, each of which would be a variation of TelemtryInfo but with different data, whereas the way the comment is currently phrased implies there should be one event class, with lots of different fields, to cover all the different events. 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. Good point - yes, the intent is to allow for different event classes , not one class with a bunch of fields. |
||||||||||
/// | ||||||||||
/// For example, The LLDB debugger can define a DebugCommandInfo subclass | ||||||||||
/// which has additional fields about the debug-command being instrumented, | ||||||||||
/// such as `CommandArguments` or `CommandName`. | ||||||||||
struct TelemetryInfo { | ||||||||||
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 know we've discussed time points before and I've been giving it a bit more thought today. I think there are three broad time-related variations of a piece of telemetry data:
For a given event type, I don't think it makes sense for it to support more than one of these variations. So I think that implies either three subclasses, or probably simpler, three constructors for TelemetryInfo, like in the following pseudo-code.
I feel like this is generic enough that it deserves to be in the base framework, rather than having most/all downstream vendors having to implement the same things. 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 strongly prefer to not include time in the base implementation. Our downstream implementation exclusively uses counts and has a different mechanism for collecting performance telemetry. 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 think you're looking at an outdated snapshot of the patch somehow. Would that resolve your concern here? |
||||||||||
// This represents a unique-id, conventionally corresponding to | ||||||||||
// a tool's session - i.e., every time the tool starts until it exits. | ||||||||||
// | ||||||||||
// Note: a tool could have multiple sessions running at once, in which | ||||||||||
// case, these shall be multiple sets of TelemetryInfo with multiple unique | ||||||||||
// ids. | ||||||||||
oontvoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
// | ||||||||||
// Different usages can assign different types of IDs to this field. | ||||||||||
std::string SessionId; | ||||||||||
|
||||||||||
TelemetryInfo() = default; | ||||||||||
~TelemetryInfo() = default; | ||||||||||
labath marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
|
||||||||||
virtual json::Object serializeToJson() const; | ||||||||||
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 function still feels wrong to me. I reckon it should look like this:
Concrete implementations populate it with functions like this:
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'd like to clarify the rationale of this a bit. Previously(see comments on Sept 3), you raised an issue with the design of Second point, w.r.t your concern about the explosion of Third point, your proposal for having the
This was also why I initially wanted to "hide" the serialization step inside Does this sound reasonable? 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'm wondering if we've got slightly different visions of things and it's driving the way we're looking at the design differently. Reading between the lines of some of the comments and responses you've made, I get the impression you expect either all concrete In my mind, unless we have concrete instances of at least Regarding the details, I'd expect the theoretical 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 think we have very similar ideas of how this ought to work, but there's some miscommunication probably. (ie., , your second paragraph describes pretty much how it is implemented, at least for LLDB's telemetry)
I want to point out that we should not conflate vendor-specific code with tool-specific code. So - there are two levels of (potential) overrides here:
I would expect, the bulk of code implementing subclasses of Then, different vendors (for the same tool) will provide the concrete implementation of In other words, the only vendor-defined pieces are the concrete implementation of 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. P.S: Concretely, it could look something like this:
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.
Let me try to elaborate on this (I was discussing this with Vy last week). Protobufs are more strongly typed than json (you can basically think of them as structs which know how to serialize themselves to a byte stream). The natural way to work with them is to work with named accessors ( The second source of confusion (now I'm moving on to the "right key string" part) is that this (*) we assume that to be the case since the vendor needs to decide (based on its use case, data collection policy, etc.) whether a particular field can be collected or not 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. Thanks for the explanations - I now see what you're thinking.
This is a fair point. The key thing this means is that code in the top-level framework that is designed to be common needs to be able to work with subclasses defined in tool-level and vendor code. In particular, I think it's reasonable to expect there to be TelemetryInfo subclasses defined in common LLVM code (so for example a I think it's also reasonable to assume that more event types will be written over time (and existing events will be extended). What I think we want to avoid though is for the need for every Sorry, that last paragraph is a bit rambly, but I'm leaving it as-is, so that you can see some of the thoughts going through my head. Ultimately, I think my point is that the advantage of having something like I'll acknowledge that something strongly typed like protobuf doesn't work well with the
FWIW, in our downstream implementation, our 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. Ok, I've thought about this for awhile, I think we can go with James's 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. (NOTE: to be implemented ... this is not resolved yet) |
||||||||||
|
||||||||||
// For isa, dyn_cast, etc, operations. | ||||||||||
virtual KindType getKind() const { return EntryKind::Base; } | ||||||||||
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. Do you ever intend to create instances of the base class? Should the class be abstract (perhaps through making this method abstract ( 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. Not immediately in LLDB, but some simpler use case might just need the base class? 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. Maybe, though I find that unlikely, since this entry carries essentially no information. And one could easily define a concrete empty class, if that's what one really needed. One advantage of making this abstract is that you then don't need the |
||||||||||
static bool classof(const TelemetryInfo *T) { | ||||||||||
if (T == nullptr) | ||||||||||
return false; | ||||||||||
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 don't see null checks in the other classof implementations (I think that because we have 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. Also, for a base class, I think the proper implementation of this function is 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 still believe there shouldn't be a null check here. we have llvm::(dyn_)cast*_if_present* for that. |
||||||||||
return T->getKind() == EntryKind::Base; | ||||||||||
} | ||||||||||
}; | ||||||||||
|
||||||||||
/// This class presents a data sink to which the Telemetry framework | ||||||||||
/// sends data. | ||||||||||
/// | ||||||||||
/// Its implementation is transparent to the framework. | ||||||||||
/// It is up to the vendor to decide which pieces of data to forward | ||||||||||
/// and where to forward them. | ||||||||||
class Destination { | ||||||||||
public: | ||||||||||
virtual ~Destination() = default; | ||||||||||
virtual Error emitEntry(const TelemetryInfo *Entry) = 0; | ||||||||||
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 don't think 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. (I'll respond to this in the same comment w.r.t the serializer because these two are related and it's easier to group comments into one) 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. done - renamed to |
||||||||||
virtual llvm::StringLiteral name() const = 0; | ||||||||||
oontvoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
}; | ||||||||||
|
||||||||||
/// This class is the main interaction point between any LLVM tool | ||||||||||
/// and this framework. | ||||||||||
/// It is responsible for collecting telemetry data from the tool being | ||||||||||
/// monitored and transmitting the data elsewhere. | ||||||||||
class Telemeter { | ||||||||||
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 a weird name. Could you try to use English names only, please. 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. It is English, AFAIK. https://www.oxfordlearnersdictionaries.com/us/definition/english/telemeter_2 P.S: Basically means a device for recording & sending telemetry, which is the appropriate name for this class 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. Shows what I know 😄 Not sure it's a well-known word though, so it doesn't really help convey meaning though, which is the key thing. 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. Please don't resolve conversations I've been involved with (see https://discourse.llvm.org/t/rfc-github-pr-resolve-conversation-button/73178 for a lengthy discussion). I'll resolve these when I'm happy with the resolution. In particular, as noted, I don't think the name "Telemeter" is going to convey the meaning clearly enough. Furthermore, as noted earlier, having all the classes prefixed with 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've renamed the Destiantion, Config, etc to get rid of the prefix. What are your suggestions for alternatives for 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. P.S: How about 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. FWIW, I had a similar reaction as @jh7370 when I saw the class name. Without 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 would go with I see this class as being the thing that a) receives the user-created configuration data, b) receives the telemetry data, c) does things related to setup and teardown of the telemetry system. It then forwards that data onto the Destinations and generally manages things to do with telemetry. I therefore could be persuaded by 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 like 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 believe this is resolved - we've agreed to name it |
||||||||||
public: | ||||||||||
// Invoked upon tool startup | ||||||||||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
virtual void atStartup(llvm::StringRef ToolPath, TelemetryInfo *Entry) = 0; | ||||||||||
|
||||||||||
// Invoked upon tool exit. | ||||||||||
virtual void atExit(llvm::StringRef ToolPath, TelemetryInfo *Entry) = 0; | ||||||||||
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'm not sure I follow why Our telemetry Also, is 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'll try to paraphase the 3 questions (I think) posed in this comment:
It could go either way. My preference for having a separate 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. Okay, thanks for the explanation. I'm happy with the 2 and 3 responses. For the 1 case, how might 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.
Do you mean, how would the implementation of
The subclass of These functions would be specific to each tool, hence not shared 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'm not sure if I expressed this in the past, but I still take issue with these two methods, I don't think they belong in the base implementation and should be specific instance of 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. Can you explain what you'd like to be in this class? Right now, it only has 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 still think a generic As well as the
Then in the tool code, you might have something like:
Finally, I think it makes sense for 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. done - changed this to |
||||||||||
|
||||||||||
virtual void addDestination(Destination *Destination) = 0; | ||||||||||
}; | ||||||||||
|
||||||||||
} // namespace telemetry | ||||||||||
} // namespace llvm | ||||||||||
|
||||||||||
#endif // LLVM_TELEMETRY_TELEMETRY_H |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
add_llvm_component_library(LLVMTelemetry | ||
Telemetry.cpp | ||
|
||
ADDITIONAL_HEADER_DIRS | ||
"${LLVM_MAIN_INCLUDE_DIR}/llvm/Telemetry" | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
#include "llvm/Telemetry/Telemetry.h" | ||
|
||
namespace llvm { | ||
namespace telemetry { | ||
|
||
llvm::json::Object TelemetryInfo::serializeToJson() const { | ||
return json::Object{ | ||
{"SessionId", SessionId}, | ||
}; | ||
}; | ||
|
||
} // namespace telemetry | ||
} // namespace llvm |
Uh oh!
There was an error while loading. Please reload this page.