sammccall added a comment.

Couple of notes on this patch.

There's no actual request logging here yet. My plan is something like `[public] 
request v1/Relations => SUCCESS: 3 results in 3ms`. Wanted to keep the patch a 
reasonable size.

There's also no testing, testing is definitely important here. I want to build 
something on top of Kirill's python tests once that lands. Probably in the 
patch that adds request logging.

The `[public]` thing is gross, but I think worse is better here.
I went through lots of iterations, including with explicit "Sensitivity" enum 
passed around, but it was pretty invasive. 
Moreover explicitly supporting it in `Logger.h` made me feel like all the 
callsites in all our libraries should consider whether info is public or not. I 
think spreading that cognitive load around the whole project is a net negative.
With the request-context detection (which was a last-minute realization 
actually), maybe we could even get away without `[public]`, but I didn't want 
to commit to contorting our code so that the request logging happens outside 
the request scope. Happy to drop this part if you think we can make it work.

Weird extension idea: we could have a completely synthetic request ID to stick 
on the request log lines and also the pattern-only elog()s to tie them together.

Changing the Logger API is a bit annoying, but has one side-benefit: error 
count grouped by format string is a nice thing to be able to monitor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90526

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to