Re: [PATCH] D78429: [clangd] Metric tracking through Tracer

thanks Hans! On Mon, May 4, 2020 at 12:38 PM Hans Wennborg via Phabricator < revi...@reviews.llvm.org> wrote: > hans added a comment. > > For anyone running into the same problem, this broke the build with GCC 5: > > /work/llvm.monorepo/clang-tools-extra/clangd/ClangdServer.cpp:374:75: >

[PATCH] D78429: [clangd] Metric tracking through Tracer

hans added a comment. For anyone running into the same problem, this broke the build with GCC 5: /work/llvm.monorepo/clang-tools-extra/clangd/ClangdServer.cpp:374:75: error: could not convert ‘(const char*)""’ from ‘const char*’ to ‘llvm::StringLiteral’

[PATCH] D78429: [clangd] Metric tracking through Tracer

This revision was automatically updated to reflect the committed changes. kadircet marked 2 inline comments as done. Closed by commit rGe64f99c51a8e: [clangd] Metric tracking through Tracer (authored by kadircet). Changed prior to commit: https://reviews.llvm.org/D78429?vs=261514=261686#toc

[PATCH] D78429: [clangd] Metric tracking through Tracer

sammccall accepted this revision. sammccall added a comment. Still LG Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:1047 Server, FooHPath, RenamePos, NewName, {/*CrossFile=*/true})); + EXPECT_THAT(Tracer.takeMetric("rename_files"), SizeIs(1));

[PATCH] D78429: [clangd] Metric tracking through Tracer

kadircet updated this revision to Diff 261514. kadircet added a comment. - Track renamed files instead of occurences Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78429/new/ https://reviews.llvm.org/D78429 Files:

[PATCH] D78429: [clangd] Metric tracking through Tracer

kadircet updated this revision to Diff 261441. kadircet marked 4 inline comments as done. kadircet added a comment. - Change IsEmpty with SizeIs - Make TestTracer thread-safe Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78429/new/

[PATCH] D78429: [clangd] Metric tracking through Tracer

sammccall accepted this revision. sammccall added a comment. Thanks! In D78429#2013635 , @kadircet wrote: > Also while writing the test for rename I've noticed we were actually counting > renamed files rather than occurrences, had to put a loop that

[PATCH] D78429: [clangd] Metric tracking through Tracer

kadircet updated this revision to Diff 261329. kadircet marked 4 inline comments as done. kadircet added a comment. Herald added a subscriber: mgorny. - Test real metrics Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78429/new/

[PATCH] D78429: [clangd] Metric tracking through Tracer

kadircet added a comment. In D78429#2012873 , @sammccall wrote: > Awesome, ship it! > > ... though, how do you feel about testing the actual metrics we export? > > Suggest a slightly generalized TestTracer that installs itself (RAII), and > has a > >

[PATCH] D78429: [clangd] Metric tracking through Tracer

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Awesome, ship it! ... though, how do you feel about testing the actual metrics we export? Suggest a slightly generalized TestTracer that installs itself (RAII), and has a // Returns

[PATCH] D78429: [clangd] Metric tracking through Tracer

kadircet added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:46 +// Tracks end-to-end latency of high level lsp calls. +constexpr trace::Metric LSPLatencies("lsp_latencies", sammccall wrote: > sammccall wrote: > > comment should

[PATCH] D78429: [clangd] Metric tracking through Tracer

kadircet updated this revision to Diff 261208. kadircet marked 23 inline comments as done. kadircet added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78429/new/ https://reviews.llvm.org/D78429 Files:

[PATCH] D78429: [clangd] Metric tracking through Tracer

sammccall added a comment. I like this a lot! Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:46 +// Tracks end-to-end latency of high level lsp calls. +constexpr trace::Metric LSPLatencies("lsp_latencies", comment should mention the label schema if

[PATCH] D78429: [clangd] Metric tracking through Tracer

kadircet marked 8 inline comments as done. kadircet added inline comments. Comment at: clang-tools-extra/clangd/Trace.h:35 +/// itself to active tracer on destruction. +struct Metric { + enum Type { sammccall wrote: > Conceptually there's a difference between a

[PATCH] D78429: [clangd] Metric tracking through Tracer

kadircet updated this revision to Diff 261156. kadircet marked an inline comment as done. kadircet added a comment. - Separate concept of a metric and measurement - Tie latency tracking to trace::Spans. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D78429: [clangd] Metric tracking through Tracer

sammccall added a comment. Thanks for tackling this! Mostly API stuff, obviously :-) Comment at: clang-tools-extra/clangd/Trace.h:35 +/// itself to active tracer on destruction. +struct Metric { + enum Type { Conceptually there's a difference between a metric

[PATCH] D78429: [clangd] Metric tracking through Tracer

kadircet updated this revision to Diff 258595. kadircet added a comment. Herald added a subscriber: ormris. - Add tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78429/new/ https://reviews.llvm.org/D78429 Files:

[PATCH] D78429: [clangd] Metric tracking through Tracer

kadircet updated this revision to Diff 258590. kadircet added a comment. - Report metrics on destruction. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78429/new/ https://reviews.llvm.org/D78429 Files: clang-tools-extra/clangd/TUScheduler.cpp

[PATCH] D78429: [clangd] Metric tracking through Tracer

kadircet updated this revision to Diff 258591. kadircet added a comment. - Introduce latency tracking through context. - Add metrics for code actions and rename. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78429/new/