-
Notifications
You must be signed in to change notification settings - Fork 5
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
[quic] add -v and -s to filter response headers #18
Conversation
…trace_send_response_header_1’ which could not be resolved!
Thank you for addressing the selective |
h2olog
Outdated
@@ -710,6 +710,9 @@ def handle_quic_event(cpu, data, size): | |||
if allowed_quic_event and ev.type != allowed_quic_event: | |||
return | |||
|
|||
if allowed_res_header_name and ev.type == "send_response_header" and ev.h2o_header_name != allowed_res_header_name: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this condition what you really intended? My take is that this condition would allow HTTP-level send_response_header
events to be printed even when -s foo
is omitted. Would something like this make more sense?
if ev.type == "send_response_header":
if allowed_res_header_name == None or ev.h2o_header_name != allowed_res_header_name:
return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing, if we adopt the above condition, we'll no longer need the verbose option (at least for this round).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it has broken by 13146a8 and fixed in 320974b
My intention is:
-v
enables HTTP-level events (it might includereceive_request_header
in the future)- If
-v
is specified and-s
is omitted, all the HTTP-level events are printed - if
-v
and-s
are specified, only specified response headers
I think -v
should exist if h2olog quic
handles only quic
tracing by default, but it can be removed if we do #21.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for addressing all the comments!
As discussed in #17, I'd like to add
-v
to enablesend_response_header
probe and-s
to filter it (s
comes from reSponse).