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

Feature: support gRPC compressed payloads #135

Merged
merged 3 commits into from
Oct 2, 2024

Conversation

emaheuxPEREN
Copy link
Contributor

Add support for viewing compressed gRPC payloads.

In theory we should check consistency of actual content encoding with 'grpc-encoding' header, but we do not have access to it in this context and thus I inferred compression scheme based on magic bytes.

@pimterry
Copy link
Member

Neat! Thanks for looking into this @emaheuxPEREN.

I do think it would be good to do this based on the actual header though, rather than content sniffing. Where possible it would be good to follow the grpc spec closely - this is a neat solution, but it's not what most clients will be doing, and matching normal behaviour will provide nicer UX & avoid various problems down the line.

Should be very doable though - we could add an optional headers parameter for formatters, and similarly optionally pass the full headers as a prop to ContentViewer (we already pass the content-type header, seems reasonable to just replace that with the full set), and then pass it along through the other APIs en route to get it here. Does that make sense?

It would also be good to add some tests for this. There's protobuf tests in test/unit/util/protobuf.spec.ts.

@CLAassistant
Copy link

CLAassistant commented Sep 26, 2024

CLA assistant check
All committers have signed the CLA.

@emaheuxPEREN
Copy link
Contributor Author

Neat! Thanks for looking into this @emaheuxPEREN.

I do think it would be good to do this based on the actual header though, rather than content sniffing. Where possible it would be good to follow the grpc spec closely - this is a neat solution, but it's not what most clients will be doing, and matching normal behaviour will provide nicer UX & avoid various problems down the line.

Should be very doable though - we could add an optional headers parameter for formatters, and similarly optionally pass the full headers as a prop to ContentViewer (we already pass the content-type header, seems reasonable to just replace that with the full set), and then pass it along through the other APIs en route to get it here. Does that make sense?

It would also be good to add some tests for this. There's protobuf tests in test/unit/util/protobuf.spec.ts.

Done :)

I also added:

  • a tiny inference about undeclared gRPC content as well (as for Protobuf)
  • some minor improvements for base64 content

@emaheuxPEREN
Copy link
Contributor Author

@pimterry would it be possible to have a list of contributors somewhere in the repo (beyond GitHub-embedded contributors, like a standard CONTRIBUTORS file) please before I can sign the CLA?

@pimterry
Copy link
Member

Done :)

Thanks for the other work updating this, looks good at first pass, I'll do a proper review before the end of the week 👍

@pimterry would it be possible to have a list of contributors somewhere in the repo (beyond GitHub-embedded contributors, like a standard CONTRIBUTORS file) please before I can sign the CLA?

I'd really rather not, to be honest. Mostly because it duplicates the git history, and will inevitably get out of sync pretty quickly (or will require an extra layer of tooling in CI to avoid that, and block PRs until it's updated). And it would be odd to add to just one repo - which implies a bunch of work to do the same for the other 50 or so HTTP Toolkit component repos.

The history of contributions is always available via Git itself though: git shortlog -sne shows every contributor within any given repo, with their name, git email & no of commits. PR are merged with that contribution history included, so that record is always there.

In the near term I'd like to create a proper /open-source page on the marketing site, to highlight all contributors (there's already a running total on the site, but specific recognition would be nice), the interesting/active repos people might want to explore & reuse, and connections to upstream projects HTTP Toolkit supports (https://httptoolkit.com/blog/open-source-funding-pledge/) but right this second I'm on paternity leave, so I'm not doing any development or substantial work for a couple of months.

By the way, in case you're not aware - HTTP Toolkit Pro is totally free for all contributors. Is that something you'd be interested in? Just let me know the email you'd like to use for your account if so (either here or send it to me at tim @ httptoolkit.com).

@emaheuxPEREN
Copy link
Contributor Author

I'd really rather not, to be honest. Mostly because it duplicates the git history, and will inevitably get out of sync pretty quickly (or will require an extra layer of tooling in CI to avoid that, and block PRs until it's updated). And it would be odd to add to just one repo - which implies a bunch of work to do the same for the other 50 or so HTTP Toolkit component repos.

The history of contributions is always available via Git itself though: git shortlog -sne shows every contributor within any given repo, with their name, git email & no of commits. PR are merged with that contribution history included, so that record is always there.

In the near term I'd like to create a proper /open-source page on the marketing site, to highlight all contributors (there's already a running total on the site, but specific recognition would be nice), the interesting/active repos people might want to explore & reuse, and connections to upstream projects HTTP Toolkit supports (https://httptoolkit.com/blog/open-source-funding-pledge/) but right this second I'm on paternity leave, so I'm not doing any development or substantial work for a couple of months.

Ok, fair enough :)

By the way, in case you're not aware - HTTP Toolkit Pro is totally free for all contributors. Is that something you'd be interested in? Just let me know the email you'd like to use for your account if so (either here or send it to me at tim @ httptoolkit.com).

Indeed, I'd really appreciate this, thank you!

@pimterry pimterry merged commit 479571d into httptoolkit:main Oct 2, 2024
7 checks passed
@pimterry
Copy link
Member

pimterry commented Oct 2, 2024

Thanks for the other work updating this, looks good at first pass, I'll do a proper review before the end of the week 👍

Reviewed, this all looks great! Thanks for all your work on this. Just merged now, that'll be live in 20 minutes or so (your HTTP Toolkit will update in the background within 30 seconds then the next time it runs, and then this will be active the following time).

Indeed, I'd really appreciate this, thank you!

Ok, great! I've just enabled that for your git email (e**.m**@p*.g*.fr), just click 'Get Pro' then 'Log into existing account' and enter your email to get started.

@emaheuxPEREN
Copy link
Contributor Author

Thanks for the other work updating this, looks good at first pass, I'll do a proper review before the end of the week 👍

Reviewed, this all looks great! Thanks for all your work on this. Just merged now, that'll be live in 20 minutes or so (your HTTP Toolkit will update in the background within 30 seconds then the next time it runs, and then this will be active the following time).

Indeed, I'd really appreciate this, thank you!

Ok, great! I've just enabled that for your git email (e**.m**@p*.g*.fr), just click 'Get Pro' then 'Log into existing account' and enter your email to get started.

Thank you, I appreciate. I'll continue contributing if I spot anything interesting to integrate :)

@emaheuxPEREN emaheuxPEREN deleted the grpc_compressed_payloads branch October 3, 2024 07:54
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.

3 participants