socketry / async-http

MIT License
298 stars 45 forks source link

Downgrade log level #139

Closed zarqman closed 9 months ago

zarqman commented 9 months ago

This PR changes one log entry from error to debug to reduce log noise. Since this exception is fully handled by resetting the stream, using error seems unnecessary.

Types of Changes

Contribution

ioquatix commented 9 months ago

Did you see this in practice? I think this situation is pretty abnormal - stream reset is required but not a desirable outcome.

zarqman commented 9 months ago

Yes, if async-http is public facing, it's pretty easy to be on the receiving end of non-conforming http requests. I'm glad to see async-http strongly validating incoming headers, but the log message at level=error can turn out to be noisy.

There are several other spots where send_reset_stream is called without logging at all. Is there something about this case that makes the logging particularly important--perhaps I'm missing something?

ioquatix commented 9 months ago

I'm okay with this, I'm also okay with removing it.

I believe the reason why I added it was to help understand header validation issues. In practice, this probably isn't needed, but some users might find it userful when debugging HTTP/2.

zarqman commented 9 months ago

Thanks much!

Do you have a general preference or guideline on when to emit a debug log vs skip logging when rescuing errors of this sort? I may have a couple other errors to quiet down the logs (related to tasks ending with unhandled exceptions) and I'd prefer to submit them how you like them. 😄

ioquatix commented 9 months ago

I feel like logs like this are battle earned knowledge. However, in isolation, it probably looks like it doesn't help anyone. In my experience, there is no good "general logs" as it depends on what you care about. Log levels help a bit.

I think the biggest concern I have, is when code eats an error, without any visible information. This can take hours, days, weeks even to track down in a large system. Hence, why I think generally, logs like this can be useful.