[PATCH] D79678: [clangd] Add CSV export for trace metrics

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet marked 2 inline comments as done.
kadircet added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM!



================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:210
+    assert(!needsQuote(Metric.Name));
+    std::string QuotedLabel;
+    if (needsQuote(Label))
----------------
do we ever expect to have any of `\r \n , "` in a label? I think it would be OK to just assert notNeedsQuote on Label.

Up to you though.


================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:214
+    uint64_t Micros = timestamp();
+    std::string Scratch;
+    Out << llvm::formatv("{0},{1},{2},{3:e},{4}.{5:3}\r\n",
----------------
unused var


================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:215
+    std::string Scratch;
+    Out << llvm::formatv("{0},{1},{2},{3:e},{4}.{5:3}\r\n",
+                         typeName(Metric.Type), Metric.Name, Label, Value,
----------------
acquire `Mu` before printing ?


================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:221
+private:
+  llvm::StringRef typeName(Metric::MetricType T) {
+    switch (T) {
----------------
why not use integers instead?


================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:251
+    using namespace std::chrono;
+    return MicrosT0SinceEpoch +
+           duration<double, std::micro>(steady_clock::now() - SteadyT0).count();
----------------
what are the benefits for making these absolute apart from being able to merge streams coming from different runs?
I am not sure if we'll ever merge data from multiple users, or even multiple runs from the same user.

This would help decrease output size only by a couple percents though, so not that important. Just wondering if there are any other reasons.


================
Comment at: clang-tools-extra/clangd/unittests/support/TraceTests.cpp:183
+  llvm::SmallVector<llvm::StringRef, 4> Lines;
+  llvm::StringRef(Output).split(Lines, "\r\n");
+  EXPECT_THAT(Lines, ElementsAre(_, StartsWith(R"(d,dist,",",1)"),
----------------
nit: maybe have a `getLines()` in the fixture.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79678/new/

https://reviews.llvm.org/D79678