Skip to content
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

feat(logs): Add error logs for io.Copy if error isn't EOF #143

Closed
wants to merge 2 commits into from

Conversation

yquansah
Copy link
Contributor

@yquansah yquansah commented Jul 24, 2024

It seems as though on io.Copy, an EOF constitutes the normal pattern of a successful copy, but I am checking for io.EOF anyway just to be safe.

If the error isn't an io.EOF, I think having a log message indicating what went wrong might be good for the user experience.

Let me know your thoughts, what do you think? @andydunstall

Right now in k8s as we are using piko in TCP mode, we are seeing the connection close on the agent side for some peculiar reason (not sure what it is yet), but the error log would be good to have as general diagnostics.

Also if the log messages can be better, let me know. I just went with something pretty general:

failure to copy...

@andydunstall
Copy link
Owner

andydunstall commented Jul 24, 2024

Hi @yquansah, thanks for the contribution! As mentioned in another thread, TCP support is still quite new so great to find issues (at some point I'm hoping to look at TCP error handling, such as opening the connection to the forward address before considering the tunnel created)

What kinds of errors are you expecting to see other than net.ErrClosed? Trying this out I'm getting a lot of "error":"read tcp 127.0.0.1:8000->127.0.0.1:56166: use of closed network connection" which probably shouldn't be logged, as that's expected behaviour? So I'm just wondering what other errors this could be useful to catch? (Maybe a debug log would make more sense?)

@yquansah
Copy link
Contributor Author

@andydunstall You are right. My thinking here was that it could be a catch all for unexpected errors, just as a safety net that users can turn on when necessary. A debug log as you mentioned would indeed make more sense here. I'll switch it to that.

@andydunstall
Copy link
Owner

@yquansah Added #144 which includes your commits plus a bit of tidy up (shortens log line and removes EOF check) - if that looks ok will merge

(I would have pushed to this PR though as its a fork I don't think I can)

@yquansah
Copy link
Contributor Author

@andydunstall Indeed! I approved your PR.

@yquansah yquansah closed this Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants