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

[quic] add -v and -s to filter response headers #18

Merged
merged 3 commits into from
Mar 4, 2020
Merged

[quic] add -v and -s to filter response headers #18

merged 3 commits into from
Mar 4, 2020

Conversation

gfx
Copy link
Collaborator

@gfx gfx commented Feb 25, 2020

As discussed in #17, I'd like to add -v to enable send_response_header probe and -s to filter it (s comes from reSponse).

@toru
Copy link
Owner

toru commented Mar 4, 2020

Thank you for addressing the selective enable_probe issue. I left another comment that might be important.

toru
toru approved these changes Mar 4, 2020
h2olog Outdated Show resolved Hide resolved
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:
Copy link
Owner

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

Copy link
Owner

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).

Copy link
Collaborator Author

@gfx gfx Mar 4, 2020

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 include receive_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.

Copy link
Owner

Choose a reason for hiding this comment

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

I see your point. Thank you for the fix, and walking me through your reasoning. This makes sense to me. I also appreciate your flexibility:

but it can be removed if we do #21.

Agreed. There's no need to be fixated on that option if we do #21.

Copy link
Owner

@toru toru left a 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!

@toru toru merged commit 6595854 into toru:master Mar 4, 2020
@gfx gfx deleted the filter_for_res_headers branch March 4, 2020 04:59
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