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

Fix h2 request_summary #345

Closed
wants to merge 3 commits into from

Conversation

Congyuwang
Copy link
Contributor

Currently, HTTP/2 request_summary use HOST header for printing host, which is empty. While the path is also different from HTTP/1.x.

Before:

HTTP/1.x:

GET /, Host: example.com:8080

HTTP/2:

GET https://example.com:8080/, Host:

After this change:
HTTP/2:

GET /, Host: example.com:8080

@andrewhavck andrewhavck self-assigned this Aug 9, 2024
@andrewhavck andrewhavck added the bug Something isn't working label Aug 9, 2024
@Congyuwang
Copy link
Contributor Author

Congyuwang commented Aug 10, 2024

The CI error is interesting, maybe unrelated to this PR but I encountered it also in my own work:

[2024-08-07T07:43:35Z WARN  pingora_core::protocols::http::v1::server] Respond header is already sent, cannot send again

This warning seems likely to be followed with Upstream broken pipe. I don't know if this is as expected, or likely a bug. If needed, I can file an issue about this.

@andrewhavck
Copy link
Contributor

CI error looks like a flaky test. That warning is expected, it means an error occurred and since we already wrote response headers we cannot send them again with the error status.

@andrewhavck andrewhavck added the Accepted This change is accepted by us and merged to our internal repo label Aug 10, 2024
drcaramelsyrup pushed a commit that referenced this pull request Aug 16, 2024
---
also add port number
---
also print query with path for h2 just as h1

Includes-commit: 2df4e29
Includes-commit: 53e1810
Includes-commit: 8102a09
Replicated-from: #345
drcaramelsyrup pushed a commit that referenced this pull request Aug 16, 2024
---
also add port number
---
also print query with path for h2 just as h1

Includes-commit: 2df4e29
Includes-commit: 53e1810
Includes-commit: 8102a09
Replicated-from: #345
@Congyuwang
Copy link
Contributor Author

synced

@Congyuwang Congyuwang closed this Aug 20, 2024
@Congyuwang Congyuwang deleted the h2-request_summary branch August 20, 2024 15:41
escoffier pushed a commit to escoffier/pingora that referenced this pull request Sep 6, 2024
---
also add port number
---
also print query with path for h2 just as h1

Includes-commit: 2df4e29
Includes-commit: 53e1810
Includes-commit: 8102a09
Replicated-from: cloudflare#345
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted This change is accepted by us and merged to our internal repo bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants