Skip to content

Log error at HTTP response close#9

Merged
rueian merged 1 commit into
git-lfs-fuse:mainfrom
dentiny:hjiang/error-logging
Apr 21, 2025
Merged

Log error at HTTP response close#9
rueian merged 1 commit into
git-lfs-fuse:mainfrom
dentiny:hjiang/error-logging

Conversation

@dentiny

@dentiny dentiny commented Apr 21, 2025

Copy link
Copy Markdown
Contributor

I understand we don't want to (and it's not easy to) propagate error at function return, but at least we could log it out for easier debugging if something goes wrong.

@rueian rueian merged commit 0dcce9b into git-lfs-fuse:main Apr 21, 2025
@rueian

rueian commented Apr 21, 2025

Copy link
Copy Markdown
Member

Thanks, @dentiny. We log errors while reading the response body, but we don’t really care about errors that occur when closing the body. Do you have any experience suggesting that they can be helpful?

@dentiny

dentiny commented Apr 21, 2025

Copy link
Copy Markdown
Contributor Author

Thanks, @dentiny. We log errors while reading the response body, but we don’t really care about errors that occur when closing the body. Do you have any experience suggesting that they can be helpful?

My general practice is don't ignore error in all cases; logging, propagation, crash are all strictly better than ignore.
In this particular case, we read from response body and discard; if the read operation fails somehow, connection cannot be reused and could lead to unexpected performance degradation.

@rueian

rueian commented Apr 21, 2025

Copy link
Copy Markdown
Member

if the read operation fails somehow, connection cannot be reused and could lead to unexpected performance degradation.

And how can that log be helpful in this case?

@dentiny

dentiny commented Apr 21, 2025

Copy link
Copy Markdown
Contributor Author

And how can that log be helpful in this case?

According to the USE method, error logging is one of the items we need to check first. The logging here helps people understand what was going wrong in the system.

@rueian

rueian commented Apr 21, 2025

Copy link
Copy Markdown
Member

According to the USE method, error logging is one of the items we need to check first.

It doesn't talk about application logs, but instead talk about metrics provided by the operation system. Those metrics are indeed a better source for monitoring TCP efficiency, including reconnections.

The logging here helps people understand what was going wrong in the system.

Aren't the error logs while reading the response body enough? Here:

n, err := io.Copy(w, resp.Body)

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