-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[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 1 commit
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,99 @@ | ||
#ifndef LVM_TELEMETRY_TELEMETRY_H | ||
#define LVM_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 { | ||
|
||
using SteadyTimePoint = std::chrono::time_point<std::chrono::steady_clock>; | ||
|
||
struct TelemetryConfig { | ||
oontvoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// If true, telemetry will be enabled. | ||
bool enable_telemetry; | ||
|
||
// Additional destinations to send the logged entries. | ||
// Could be stdout, stderr, or some local paths. | ||
// Note: these are destinations are __in addition to__ whatever the default | ||
// destination(s) are, as implemented by vendors. | ||
std::vector<std::string> additional_destinations; | ||
oontvoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
struct TelemetryEventStats { | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// REQUIRED: Start time of event | ||
SteadyTimePoint m_start; | ||
// OPTIONAL: End time of event - may be empty if not meaningful. | ||
std::optional<SteadyTimePoint> m_end; | ||
// TBD: could add some memory stats here too? | ||
|
||
TelemetryEventStats() = default; | ||
TelemetryEventStats(SteadyTimePoint start) : m_start(start) {} | ||
TelemetryEventStats(SteadyTimePoint start, SteadyTimePoint end) | ||
: m_start(start), m_end(end) {} | ||
|
||
std::string ToString() const; | ||
oontvoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
struct ExitDescription { | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
int exit_code; | ||
std::string description; | ||
|
||
std::string ToString() const; | ||
}; | ||
|
||
// The base class contains the basic set of data. | ||
// Downstream implementations can add more fields as needed. | ||
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? |
||
// A "session" corresponds to every time the tool starts. | ||
// All entries emitted for the same session will have | ||
// the same session_uuid | ||
std::string session_uuid; | ||
|
||
TelemetryEventStats stats; | ||
|
||
std::optional<ExitDescription> exit_description; | ||
|
||
// Counting number of entries. | ||
// (For each set of entries with the same session_uuid, this value should | ||
// be unique for each entry) | ||
size_t counter; | ||
|
||
TelemetryInfo() = default; | ||
~TelemetryInfo() = default; | ||
virtual std::string ToString() const; | ||
}; | ||
|
||
// Where/how to send the telemetry entries. | ||
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. The mixing of the "Where" and the "How" is a red-flag to me, because it suggests that the class is trying to do two things at once. It's related to a comment I made previously: these two concepts are somewhat orthogonal. It might be that we want to send data to a file or a string or a pipe or ... , and we might want to send it as JSON/YAML/some kind of structured messages etc. It's certainly true as noted before that mixing some "hows" with some "wheres" doesn't make sense, but there are enough cases where it does make sense that I don't think we should risk having to have a combinatorial explosion of classes e.g. 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 the comment on the usage for the class , which is that it acts as a data sink for the framework :
|
||
class TelemetryDestination { | ||
public: | ||
virtual ~TelemetryDestination() = default; | ||
virtual Error EmitEntry(const TelemetryInfo *entry) = 0; | ||
virtual std::string name() const = 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. Do we expect names to always be constant strings? If so we can have this return an 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 - seems a bit restrictive for a general API, though. |
||
}; | ||
|
||
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: | ||
virtual ~Telemeter() = default; | ||
|
||
// Invoked upon tool startup | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
virtual void LogStartup(llvm::StringRef tool_path, TelemetryInfo *entry) = 0; | ||
|
||
// Invoked upon tool exit. | ||
virtual void LogExit(llvm::StringRef tool_path, TelemetryInfo *entry) = 0; | ||
|
||
virtual void AddDestination(TelemetryDestination *destination) = 0; | ||
}; | ||
|
||
} // namespace telemetry | ||
} // namespace llvm | ||
|
||
#endif // LVM_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,32 @@ | ||
#include "llvm/Telemetry/Telemetry.h" | ||
|
||
namespace llvm { | ||
namespace telemetry { | ||
|
||
std::string TelemetryEventStats::ToString() const { | ||
std::string result; | ||
llvm::raw_string_ostream os(result); | ||
os << "start_timestamp: " << m_start.time_since_epoch().count() | ||
<< ", end_timestamp: " | ||
<< (m_end.has_value() ? std::to_string(m_end->time_since_epoch().count()) | ||
: "<NONE>"); | ||
return result; | ||
} | ||
|
||
std::string ExitDescription::ToString() const { | ||
return "exit_code: " + std::to_string(exit_code) + | ||
", description: " + description + "\n"; | ||
} | ||
|
||
std::string TelemetryInfo::ToString() const { | ||
return "[TelemetryInfo]\n" + (" session_uuid:" + session_uuid + "\n") + | ||
(" stats: " + stats.ToString() + "\n") + | ||
(" exit_description: " + | ||
(exit_description.has_value() ? exit_description->ToString() | ||
: "<NONE>") + | ||
"\n") + | ||
(" counter: " + std::to_string(counter) + "\n"); | ||
} | ||
|
||
} // namespace telemetry | ||
} // namespace llvm |
Uh oh!
There was an error while loading. Please reload this page.