-
Notifications
You must be signed in to change notification settings - Fork 10
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
atls: improve logging #1283
base: main
Are you sure you want to change the base?
atls: improve logging #1283
Conversation
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.
Sunds helpful and lgtm overall
@@ -66,6 +75,7 @@ func (c *Credentials) ServerHandshake(rawConn net.Conn) (net.Conn, credentials.A | |||
|
|||
conn := tls.Server(rawConn, serverCfg) | |||
if err := conn.HandshakeContext(ctx); err != nil { | |||
c.logger.DebugContext(ctx, "Handshake error", "error", err) |
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.
Why is this Debug
and not Error
?
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.
Because you get a EOF error here every 5 seconds due to the readiness check. Example message:
time=2025-03-11T12:00:20.766Z level=DEBUG msg="Handshake error" atlscredentials.error=EOF
Since this is also confusing for the user when enabling debug logs, we might want to add an explainer in the message or omit EOFs. Though the second one has a obvious downside of also missing all other EOF errors. Any preference?
972e962
to
3e4f6e2
Compare
During debugging of the latest SNI issue I had to add more logging in my dev state. It would be best if customers could enable this without code changes to make debugging easier.