[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