-
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
add http handlers for CORS, metrics and compression #27
Conversation
Update dependencies
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #27 +/- ##
=========================================
+ Coverage 0 21.76% +21.76%
=========================================
Files 0 5 +5
Lines 0 611 +611
=========================================
+ Hits 0 133 +133
- Misses 0 473 +473
- Partials 0 5 +5 ☔ View full report in Codecov by Sentry. |
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 cleaning this up @ns4plabs, removes need for CORS on reverse proxy + JSON routing return is highly compressible due to repeating strings. ❤️
nit: I think this PR includes changes from #23, we probably need to land that one first (iirc blocked by review?), and then rebase this one.
Let's discuss #23 next week.
}).Handler(handler) | ||
|
||
// Add compression. | ||
compress, err := httpcompression.DefaultAdapter() |
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.
Note to myself: check if this handler adds extra buffering.
If this happen this would be bad since we would be forced to wait before using any of the response.
I think it could be possible that a writer would need to first fill it's window buffer before streaming out bytes. We can compress each entry individually and concatenate them if this ever happens (decompress(compress(a + b)) == decompress(compress(a) + compress(b))
for gzip and zstd).
@hacdias I think it auto closed and you didn't meant to do it. (should have been a stack on graphite 😄) |
Oh, yeah, it autoclosed. GitHub usually just changes the base branch automatically. No idea how this happened. |
@ns4plabs would you mind making a new rebased PR? |
Depends on #23