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

GZIP web responses #1458

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

CannonLock
Copy link
Contributor

8.2mb to 1.8mb

8.2mb to 1.8mb
@CannonLock
Copy link
Contributor Author

Going to keep this as a draft as I test to make sure that this doesn't break any endpoints.

8.2mb to 1.8mb
@CannonLock
Copy link
Contributor Author

CannonLock commented Jun 27, 2024

@haoming29 This is ready, but it will cause issues on any endpoints that already gzip such as the prometheus endpoints as it will double zip them.

Can you think of any endpoints other the prometheus that we don't produce?

I can also limit it to only zip web assets.

@CannonLock CannonLock marked this pull request as ready for review June 27, 2024 18:13
@CannonLock CannonLock requested a review from haoming29 June 27, 2024 18:14
@haoming29
Copy link
Contributor

haoming29 commented Jun 28, 2024

@haoming29 This is ready, but it will cause issues on any endpoints that already gzip such as the prometheus endpoints as it will double zip them.

Can you think of any endpoints other the prometheus that we don't produce?

I can also limit it to only zip web assets.

I can't think of any other endpoints that we import as as a whole (like Prometheus), which means we should be able to control the compression for all of the rest. Would gzip affect attachment though? We do have some endpoints that have Content-Disposition: attachment to download json file. That would be the only place I'm unsure about.

@haoming29
Copy link
Contributor

@CannonLock looks like there's a unhandled merge conflict that causes tests to fail

Copy link
Contributor

@haoming29 haoming29 left a comment

Choose a reason for hiding this comment

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

One other endpoint that need to be excluded; otherwise LGTM

web_ui/ui.go Outdated Show resolved Hide resolved
@CannonLock CannonLock requested a review from turetske August 6, 2024 14:33
Copy link
Collaborator

@turetske turetske left a comment

Choose a reason for hiding this comment

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

Seems good to me, but can you rebase/deal with the merge conflicts before I approve?

@CannonLock
Copy link
Contributor Author

@turetske What merge conflicts?

@turetske
Copy link
Collaborator

turetske commented Aug 6, 2024

@turetske What merge conflicts?

Ah, my bad, I was misinterpreting the test failures. Hopefully they are just intermittent failures.

@turetske
Copy link
Collaborator

turetske commented Aug 6, 2024

@CannonLock The broker tests keep having a timeout. Could adding the gzip like this potentially slow down a callback to be beyond our threshold? (10 minutes)?

@jhiemstrawisc
Copy link
Member

I noticed this PR is creeping on 5 months since it was opened and 4 months since it was last updated. Is it something that we plan on picking back up?

@CannonLock
Copy link
Contributor Author

@jhiemstrawisc I can close and reopen in the future when complete? This is a small upgrade that looks like it is going to take more time then it is worth to fix so it isn't high on my priority list.

@jhiemstrawisc
Copy link
Member

I think we can get away just marking it as a draft if you eventually plan on returning to it

@jhiemstrawisc jhiemstrawisc marked this pull request as draft October 23, 2024 17:32
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.

4 participants