-
Notifications
You must be signed in to change notification settings - Fork 195
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
libgit2: add contextual logging to subtransports #778
Conversation
@darkowlzz following up from your comments (#776 (comment)), this PR now is only based on contextual logging. If there are no further changes required, I will go on and do the same for HTTP. |
IMO this should print |
Yep, it feels like this is something upstream, as currently we do a very similar approach for both debug and trace, and the former works fine. I will take a look. |
pkg/git/libgit2/managed/ssh.go
Outdated
addr := net.JoinHostPort(u.Hostname(), port) | ||
|
||
if t.ctx == nil { | ||
t.ctx = opts.Context |
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.
The CI seems to be failing because the test doesn't sets the context in
AddTransportOptions(transportOptsURL, TransportOptions{ |
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.
Fixed this, now both ctx
and logger
have safe defaults.
@stefanprodan looks like it's the same in IAC:
After some investigation, it looks like this is because zap doesn't know what -2 level means https://github.com/uber-go/zap/blob/e06e09a6d396031c89b87383eef3cad6f647cf2c/zapcore/level.go#L88 . And looks like it was known at the time of implementing it based on https://github.com/fluxcd/pkg/blob/3357f39663bad8cb6315edb0d2a6b97f560bdba9/runtime/logger/logger.go#L32-L35 . |
6a2d56f
to
6a4741c
Compare
b877bc2
to
e66548f
Compare
Debugging connection issues can be extremely difficult, even more so at scale or when concurrent connections are required to trigger specific issues. Changes: - Add a correlation identifier for each reconciliation, which allows for greater traceability when going through all the reconciliation operations - including at transport level. - Add transportType to segregate HTTP and SSH transport logging. - SSH operations are now enriched with addr containing server address, and HTTP url. Signed-off-by: Paulo Gomes <[email protected]>
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.
LGTM 🚀
"transportType", "http", | ||
"url", opts.TargetURL) | ||
} | ||
}) |
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.
This is fine how it is, but I'd expect such one time configurations to be done at the very beginning of the function when we have all the things we need to do it.
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.
LGTM!
It'd be good to add the correlation ID to other reconcilers as well.
@darkowlzz added an additional commit that rolls out correlation ID across the other reconcilers. |
How does the roll out of the CID combine with the upstream controller-runtime changes around logging that's still ahead of us? As this adds a reconcile ID. |
8d14f0c
to
628a9e3
Compare
GitRepository introduced correlation ID to improve transport level logging. This change aligns the other reconcilers to the same approach. Signed-off-by: Paulo Gomes <[email protected]>
Debugging connection issues can be extremely difficult, even more so at scale or when concurrent connections are required to trigger specific issues.
Changes:
reconcileID
) for each reconciliation, which allows for greater traceability when going through all the reconciliation operations - including at transport level.addr
containing server address, and HTTPurl
.Example of stdout output when grepping by a specific TransportID: