[llvm] [llvm]Add a simple Telemetry framework (PR #102323)
Vy Nguyen via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 20 10:18:08 PDT 2024
oontvoo wrote:
Summary of review feedbacks and responses
(Since it's been a long thread, I thought it might be helpful to collect all the topics into one thread here for book-keeping)
- Cosmetics changes:
- capitalization: This has been addressed (all functions, types and variables names have been redone to conform with LLVM style)
- naming of types: drop `Telemetry` from various structs/classes to avoid redundancy (since the namespace `llvm::telemetry` already has it)
- headers license and docs: Done (doxygen style comments plus additional docs added)
- Naming of `Telemeter`: requested to be renamed to something else. Status: NOT done yet - not clear what reviewer would prefer (alternative includes : "TelemetryCollector")
- Discussions on the design and implementations:
- Testing: functional/end-to-end tests was requested by reviewer.
- Response: we cannot have an end-to-end test here because this patch proposed a support library. As such, in accordance with the LLVM coding guidelines, unit tests are more appropriate. (There is no tool using it yet, hence a lit test isn't helpful)
- Concrete implementation: a concrete implementation of this library (which should just forward data to a local file) was requested by reviewer (partly to demonstrate how it's supposed to be used).
- Response: a concrete impl was NOT provided intentionally due to various reasons
- There is not enough common patterns between tools to make a shared impl useful
- More importantly, privacy and security models of different organizations are very different, which makes it impractical to try to satisfy all.
- For eg., some might find it OK to forward telemetry data to local file, while others do not allow that since "local file" has no auditable access mechanism.
- Furthermore, w.r.t how to use the framework, this issue was addressed by:
- Docs have been added to give example
- LLDB Telemetry patch also demonstrates clearly how the framework would be used.
- Purposes of the `Destination` class (and data serialization)
- Reviewer suggested there might be some confusion with how this class is supposed to be used. (Is it a data serialization class? what not?)
- Response: This class can be thought of as a data-sink to which the telemetry framework send the data. (Where the data eventually ends up is transparent to the framework - up to vendor's implementation). Relatedly, there is a `serializeToJson()` method added on the data courier class `TelemetryInfo`.
TL;DR: I think I've addressed all of the feedbacks except for one naming point - awaiting response rather than having to do multiple renaming round.
https://github.com/llvm/llvm-project/pull/102323
More information about the llvm-commits
mailing list