log: improvements to sparse macros, fancy logger.#13223
log: improvements to sparse macros, fancy logger.#13223htuch merged 3 commits intoenvoyproxy:masterfrom
Conversation
Inspired by a log statement needed for envoyproxy#10815, this patch: * Adds _MISC variants of sparse macros. * Sends _MISC log messages to fancy logger (these were previously ignored). This is a bit weird, as "misc" is also a logger and we use file-specific loggers in fancy logger, but in reality, most folks treat this macro as the generic Envoy log statement. * Avoid evaluating expressions in ENVOY_LOG* with fancy logger when beneath the configured log level. Risk level: Low Testing: Mostly covered by log_macros_test already, added some _MISC coverage in SparseLogMacrosTest. Signed-off-by: Harvey Tuch <htuch@google.com>
oschaaf
left a comment
There was a problem hiding this comment.
a nit and a question, otherwise looks good to me.
source/common/common/fancy_logger.cc
Outdated
| } | ||
| } | ||
|
|
||
| FancyLogLevelMap FancyContext::getAllFancyLogLevels() ABSL_LOCKS_EXCLUDED(fancy_log_lock_) { |
There was a problem hiding this comment.
I lack some context here, but if this is a production code utility, would this deserve a unit test of its own, and maybe add caching to it?
If not, would it be worth considering moving this into test/ as right now it's only used there?
I don't want to bloat this PR, just curious.
There was a problem hiding this comment.
fancy_logger.h comments say this is test only, I don't mind renaming to getAllFancyLogLevelsForTest() if that makes it clearer. Not sure how to move it out of this file as it needs access to internal fields.
There was a problem hiding this comment.
Yes, please name test-only code as such. Makes it much easier to understand.
oschaaf
left a comment
There was a problem hiding this comment.
Thanks for the explanations. LGTM
ggreenway
left a comment
There was a problem hiding this comment.
LGTM, modulo renaming ....ForTest
Signed-off-by: Harvey Tuch <htuch@google.com>
Inspired by a log statement needed for #10815, this patch:
Adds _MISC variants of sparse macros.
Sends _MISC log messages to fancy logger (these were previously
ignored). This is a bit weird, as "misc" is also a logger and
we use file-specific loggers in fancy logger, but in reality, most
folks treat this macro as the generic Envoy log statement.
Avoid evaluating expressions in ENVOY_LOG* with fancy logger when
beneath the configured log level.
Risk level: Low
Testing: Mostly covered by log_macros_test already, added some _MISC
coverage in SparseLogMacrosTest.
Signed-off-by: Harvey Tuch htuch@google.com