common: introduce stable logger /w examples in DNS #17772
common: introduce stable logger /w examples in DNS #17772snowp merged 9 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Snow Pettersen <snowp@lyft.com>
|
Note that this does not support log levels as it bypasses spdlog. spdlog does not support any form of structured logging, so going through spdlog would mean either 1) extracting out the stable name from the formatted log string (not much better than having a log sink itself try to parse out information) or 2) by having different spdlog loggers set up for each call site (which seemed very heavy handed). It is also always on as written, even when no log sink is consuming this data. The consequence of this is mostly that the fmt::format is evaluated. I imagine we want to avoid this, but figured I'd put up the PR first before investing the time into figuring out a good solution there. |
|
An alternative API might be something like
|
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks this looks like it's on the right track. Flushing out some initial comments.
/wait
source/common/common/logger.h
Outdated
| virtual ~SinkDelegate(); | ||
|
|
||
| virtual void log(absl::string_view msg) PURE; | ||
| virtual void logStable(absl::string_view, absl::string_view) {} |
There was a problem hiding this comment.
I think I would probably call this something like logWithStableId or something like that.
Also, I think I would pass the level and component to this function in addition to the stable ID (also can you add parameter names)
source/common/common/logger.h
Outdated
| */ | ||
| #define ENVOY_LOG(LEVEL, ...) ENVOY_LOG_TO_LOGGER(ENVOY_LOGGER(), LEVEL, ##__VA_ARGS__) | ||
|
|
||
| #define ENVOY_LOG_STABLE(STABLE_NAME, ...) \ |
There was a problem hiding this comment.
Per above I still think we want component, level, etc. Also I think for these logs we want them forked to both functions with the stable ID removed for the normal one potentially?
Signed-off-by: Snow Pettersen <snowp@lyft.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks this looks right to me. @envoyproxy/maintainers thoughts on this? This feels like something we can ship and iterate on.
/wait
|
|
||
| ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, | ||
| DnsLookupFamily dns_lookup_family, ResolveCb callback) { | ||
| ENVOY_LOG_EVENT(debug, "cares_dns_resolution_started", "dns resolution for {} started", dns_name); |
There was a problem hiding this comment.
inconsistent use of "cares" in this event name vs the other names.
|
What's an example output from one of these logs? I'm not sure I 100% understand the goal, but it seems like not having log-levels will quickly become troublesome. |
|
@ggreenway As written there is no new sink handling built in for these logs - the logs are currently sent both to What we're trying to do in EM is allow hooking up a custom sink that can intercept these log events and emit analytic events, allowing us to incorporate Envoy events into debugging. This would allow us to correlate sequences of events where we see requests fail with Envoy events, e.g. failing connection setup, DNS timing out etc. This could also be built up into a more sophisticated event tracing library where we can associated events with each other, so that more sophisticated tooling can be built to analyze or render entire stream lifetimes, but for now this simple "log the event with a stable name" would be a good start for the EM use case. |
|
Got it; that makes sense. SGTM, but please put a big doc comment above the new top-level macro so people know what it is for and when to use it. |
|
I updated this to check the log level for these new events as well, which ended up being pretty simple since can just check the log level of the associated component. |
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
* main: (32 commits) Stop processing pending H/2 frames if connection transitioned to the closed state http2: limit use of deferred resets in the http2 codec to server-side connections Abort filter chain iteration on local reply Reject or strip fragment from request URI ext-authz: merge duplicate headers from client request in check request common: introduce stable logger /w examples in DNS (envoyproxy#17772) route: fast return when route matches failed (envoyproxy#17769) owners: add owners for dubbo proxy network filter (envoyproxy#17820) config/router/tcp_proxy/options: v2 API, boosting and --bootstrap-version CLI removal. (envoyproxy#17724) coverage: revert the limit http/cache to 92.6. (envoyproxy#17817) network: rename SocketAddressProvider as ConnectionInfoProvider (envoyproxy#17717) test: bumping coverage (envoyproxy#17757) conn_pool: Minor cleanups to ConnPoolBaseImpl (envoyproxy#17710) Split VaryHeader into VaryAllowList and VaryUtils to organize vary-related logic (envoyproxy#17728) ext_proc: Make tests more resilient to IPv6 support (envoyproxy#17784) Remove invlaid backquote from doc (envoyproxy#17797) rocketmq: move to contrib (envoyproxy#17796) kafka: upstream kafka facade in mesh-filter (envoyproxy#17783) ecds: create shared base class for DynamicFilterConfigProviderImpl (envoyproxy#17735) Change log level from debug to trace (envoyproxy#17774) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>
3 participants
Adds a new logging macro that allows for logging with a stable identifier. This is intended to allow
a log sink to be able to key off the identifier, simplifying the process of having a log sink react to specific
log messages (e.g. to emit specific events when certain log lines are hit).
The main alternative to this is using some kind of log parsing to understand when a certain log is hit, which ends up being brittle as code is refactored. Using a stable identifier we can ensure via code review that the stable id remains as log lines are moved around, refactored, etc.
This PR includes applying the new log macro to the c-ares DNS resolver to demonstrate it's usage.
Signed-off-by: Snow Pettersen snowp@lyft.com
Risk Level: Medium
Testing: Manual testing, will add coverage
Docs Changes: n/a
Release Notes: n/a (will add)
Platform Specific Features: n/a