common: add support for thread local sink overrides#18785
Conversation
This adds support for specifying a SinkDelegate that should be active only on the current thread. This allows for per thread overrides of the logging context, which opens up for overriding the logger for the duration of a dispatcher event as well as overriding the logger for specific function calls. Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
|
logger_benchmarks after this change: Running on main: Invoked via |
|
@mattklein123 @ggreenway First stab at TLS logging override. One thing to note is that using a |
Signed-off-by: Snow Pettersen <snowp@lyft.com>
|
/retest |
|
Retrying Azure Pipelines: |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks this makes sense to me. I have a couple of questions. From my read the perf delta is not very much if any so this is probably fine to add. cc @ggreenway
/wait
source/common/common/logger.cc
Outdated
|
|
||
| void SinkDelegate::setTlsDelegate() { | ||
| assert(previous_delegate_ == nullptr); | ||
| previous_delegate_ = log_sink_->tlsDelegate(); |
There was a problem hiding this comment.
Shouldn't this actually be a previous TLS delegate that is different from the global previous delegate?
source/common/common/logger.h
Outdated
| // This is virtual in order to allow for multiple DelegatingLogSinks to exist and use different | ||
| // thread local overrides. A base class defining a new thread_local would be necessary for each | ||
| // DelegatingLogSink that will coexist. |
There was a problem hiding this comment.
Sorry I'm a little lost on this. How would multiple overrides exist in practice? Don't we want there to be a single logger at a given time? If we don't have a use for this can we make this non-virtual for now?
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
|
@jmarantz @ggreenway either of you mind doing a non-Lyft review? |
|
I'll take a look if you can wait a few hours. |
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
lgtm basically with small nit.
source/common/common/logger.h
Outdated
|
|
||
| SinkDelegate** tlsSink() { | ||
| static thread_local SinkDelegate* tls_sink = nullptr; | ||
|
|
There was a problem hiding this comment.
minor nits: slightly prefer outlining some of these non-trivial impls into logger.cc. Any reason not to do it for these untemplated methods?
also do we need the blank line above here?
|
Also, would you mind augmenting your microbenchmark numbers with the conclusions from that? I was trying to compare the two tables and couldn't figure out a tangible difference. |
Signed-off-by: Snow Pettersen <snowp@lyft.com>
|
My takeaway was also that there was no significant difference. I ran it again and used the benchmark compare.py script to get a comparison: The differences seem to be inline with the differences we're seeing fancy log exhibit between the test runs, which wasn't touched at all by this PR. tl;dr not a big difference |
| return &tls_sink; | ||
| } | ||
|
|
||
| void DelegatingLogSink::setTlsDelegate(SinkDelegate* sink) { *tlsSink() = sink; } |
There was a problem hiding this comment.
FWIW I was OK with these 2 one-liners inlined, but this is fine too.
3 participants
Commit Message:
This adds support for specifying a SinkDelegate that should be active
only on the current thread. This allows for per thread overrides of the
logging context, which opens up for overriding the logger for the
duration of a dispatcher event as well as overriding the logger for
specific function calls.
Additional Description: n/a
Risk Level: Low
Testing: New UTs, benchmarks
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a