[PATCH] D43377: [clangd] Attach more information about Sema completion to traces

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks really useful! Main suggestion is to drop the added span and attach kind to the main span instead. (It's relevant to index too, not just to sema)



================
Comment at: clangd/CodeComplete.cpp:403
 
+StringRef printCompletionKind(enum CodeCompletionContext::Kind Kind) {
+  using CCKind = enum CodeCompletionContext::Kind;
----------------
hmm, this really looks like it belongs next to CodeCompletionContext::Kind?


================
Comment at: clangd/CodeComplete.cpp:503
 
+    trace::Span Span("Process sema results");
+    SPAN_ATTACH(Span, "total_sema_items", NumResults);
----------------
I think this span is going to be tiny (hard to see in some UIs!) and generally not add a lot of value itself.
The nicest thing would be to just grab the kind out of the CCcontext and attach it to the span in CodeCompleteFlow::run()

My instinct is that the #items can be dropped entirely - we already log the number of items from Sema considered (before Limit) so all we'd be losing is the number hidden/ineligible/destructor completions we drop here, which seems uninteresting for tracing purposes.
(But exposing that from the recorder is another option)


================
Comment at: clangd/CodeComplete.cpp:504
+    trace::Span Span("Process sema results");
+    SPAN_ATTACH(Span, "total_sema_items", NumResults);
+    SPAN_ATTACH(Span, "sema_completion_kind",
----------------
sema_results_prefilter?
the filtering is the only difference between this and sema_results logged below


================
Comment at: clangd/CodeComplete.cpp:918
 
+    SPAN_ATTACH(Tracer, "file", SemaCCInput.FileName);
     SPAN_ATTACH(Tracer, "sema_results", NSema);
----------------
FWIW, I have a (delayed, sorry) patch to add a span to all actions run by TUscheduler, which would log the filename. If that lands, *maybe* we don't want it in both places (it'd be the parent span of this one)? But not sure.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43377