-
Notifications
You must be signed in to change notification settings - Fork 327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(kuma-cp): improved xds logging #4436
Conversation
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
if statusTrackerLog.V(1).Enabled() { | ||
log = log.WithValues("node", req.Node()) | ||
} | ||
log.Info("proxy connected") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe instead of
proxy connected {"proxyName": "redis"...
it can be
connected {"type": "dataplane", "name": "redis"...
so we could easily see if this is dataplane or zoneingress or zoneegress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already log the type "type", md.GetProxyType(),
in 156 line. I'd rather keep "proxy connected" instead of "connected".
It's proxyName instead of name for consistency, see #4436 (comment)
Summary
This PR improves XDS logging. Right now you have 0 logs about XDS on INFO and a massive amount of logs on DEBUG.
Logs on debug are "cryptic" and require knowledge about the CP implementation (what is
OnStreamRequest
etc).Logging entire DiscoveryRequest/Response even on debug was too much. Especially with gigantic
Node
object for every log.I was trying to pick the minimal amount of information that could inform the user what is going on with the XDS exchange without knowledge about implementation.
I was also considering implementing full resource logging on trace but
Any
toListener
, butHttpConnectionManager
is again marshaled toAny
After this change
Issues resolved
Fix #4033
Documentation
No docs.
Testing
Backwards compatibility
- [ ] UpdateUPGRADE.md
with any steps users will need to take when upgrading.- [ ] Addbackport-to-stable
label if the code follows our backporting policy