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

Log protocol errors #371

Merged
merged 7 commits into from Jun 17, 2019
Merged

Log protocol errors #371

merged 7 commits into from Jun 17, 2019

Conversation

hawkw
Copy link
Collaborator

@hawkw hawkw commented Jun 14, 2019

Currently, there are many cases where h2 will fail a connection or
stream with a PROTOCOL_ERROR, without recording why the protocol error
occurred. Since protocol errors may result from a bug in h2 or from a
misbehaving peer, it is important to be able to debug the cause of
protocol errors.

This branch adds a log line to almost all cases where a protocol error
occurs. I've tried to make the new log lines consistent with the
existing logging, and in some cases, changed existing log lines to make
them internally consistant with other log lines in that module. I have
not tried to enforce a single style for log lines across all modules,
however, as the debug-level in some modules uses a different style than
the trace logging elsewhere. Presumably this is because it is expected
to be more user-facing.

Signed-off-by: Eliza Weisman eliza@buoyant.io

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested review from seanmonstar and olix0r June 14, 2019 17:30
@hawkw hawkw self-assigned this Jun 14, 2019
Copy link
Collaborator

@olix0r olix0r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why warn vs debug vs trace?

@hawkw
Copy link
Collaborator Author

hawkw commented Jun 14, 2019

@olix0r I used warn because I thought a situation likely resulting from a misbehaving client ought to result in a warning, but upon further consideration, those logs probably ought to be at the trace level, since almost all logging in h2 is at trace. I used debug in framed_read since that module was already logging protocol errors at the debug level.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Copy link
Collaborator

@olix0r olix0r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about making all of these logs at debug! so that we can more easily access them without getting the full firehose?

@seanmonstar
Copy link
Member

Yea, I'd put logs about errors at debug level.

@hawkw
Copy link
Collaborator Author

hawkw commented Jun 14, 2019

Okay, will do! Should they also all be changed to use the format of the framed_read errors?

src/server.rs Outdated Show resolved Hide resolved
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
Copy link
Collaborator Author

hawkw commented Jun 16, 2019

I've updated this branch so that all protocol errors are logged at the debug level, and changed all the log lines to use a consistent formatting based on the protocol error log lines in framed_read, using a helper macro to reduce boilerplate.

src/codec/framed_read.rs Outdated Show resolved Hide resolved
src/proto/streams/recv.rs Outdated Show resolved Hide resolved
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested a review from olix0r June 17, 2019 20:46
Copy link
Collaborator

@olix0r olix0r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw merged commit 0e9fbe4 into master Jun 17, 2019
@seanmonstar seanmonstar deleted the eliza/log-invalid-states branch June 17, 2019 21:15
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.

None yet

3 participants