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

Downgrade log level #139

Merged
merged 1 commit into from
Sep 20, 2023
Merged

Downgrade log level #139

merged 1 commit into from
Sep 20, 2023

Conversation

zarqman
Copy link
Contributor

@zarqman zarqman commented Sep 19, 2023

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

  • Maintenance.

Contribution

@ioquatix
Copy link
Member

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

@zarqman
Copy link
Contributor Author

zarqman commented Sep 20, 2023

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 ioquatix merged commit 26a52f0 into socketry:main Sep 20, 2023
15 of 18 checks passed
@ioquatix
Copy link
Member

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 zarqman deleted the log-level branch September 20, 2023 21:11
@zarqman
Copy link
Contributor Author

zarqman commented Sep 20, 2023

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
Copy link
Member

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.

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