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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 6 inline comments as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:210
+    assert(!needsQuote(Metric.Name));
+    std::string QuotedLabel;
+    if (needsQuote(Label))
----------------
kadircet wrote:
> 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.
I don't think it's terribly likely but I can certainly imagine:
 - using a filename as a label, and some people have weird filenames
 - wanting a compound label, and choosing comma as a separator

I was torn between:
 - doing the escaping (seems a little silly)
 - asserting (a bit of a time bomb in cases above)
 - just ignoring this and outputting garbage

Chose A because it's pretty short, and I wrote a test :-)


================
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();
----------------
kadircet wrote:
> 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.
Yeah no good reason I think. Removed.
Was thinking about converting into walltimes in analysis but it seems unlikely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79678