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 compression #915

Merged
merged 7 commits into from
Jan 19, 2021
Merged

gzip compression #915

merged 7 commits into from
Jan 19, 2021

Conversation

saeltz
Copy link
Contributor

@saeltz saeltz commented Dec 9, 2020

Following the discussion on #911.

This drafts an implementation for gzip compression using https://prometheus.github.io/client_java/src-html/io/prometheus/client/exporter/HTTPServer.html as inspiration.

@saeltz saeltz marked this pull request as ready for review December 9, 2020 16:23
@saeltz
Copy link
Contributor Author

saeltz commented Dec 9, 2020

@SimunKaracic, please take a look.
The failing test is in the untouched DatadogSpanReporterSpec.

@SimunKaracic
Copy link
Contributor

SimunKaracic commented Dec 10, 2020

The datadog part was a flip-flop on github actions, feel free to ignore that.
But the failure in kamon-prometheus looks a bit more sinister:

2020-12-10T09:18:18.0477655Z [info] SunHttpServerSpecSuite:
2020-12-10T09:18:18.0889867Z 09:18:18.087 [pool-1-thread-1] INFO kamon.prometheus.PrometheusReporter - Started the embedded HTTP server on http://0.0.0.0:9095
2020-12-10T09:18:18.1077238Z [info] the embedded sun http server
2020-12-10T09:18:18.1697248Z [info] - should provide no data comment on GET to /metrics when no data loaded yet *** FAILED ***
2020-12-10T09:18:18.1698842Z [info]   java.net.SocketException: Unexpected end of file from server
2020-12-10T09:18:18.1700255Z [info]   at sun.net.www.http.HttpClient.parseHTTPHeader(HttpClient.java:851)
2020-12-10T09:18:18.1701790Z [info]   at sun.net.www.http.HttpClient.parseHTTP(HttpClient.java:678)
2020-12-10T09:18:18.1776582Z [info]   at sun.net.www.http.HttpClient.parseHTTPHeader(HttpClient.java:848)
2020-12-10T09:18:18.1778387Z [info]   at sun.net.www.http.HttpClient.parseHTTP(HttpClient.java:678)
2020-12-10T09:18:18.1780380Z [info]   at sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1593)
2020-12-10T09:18:18.1791442Z [info]   at sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1498)
2020-12-10T09:18:18.1793374Z [info]   at java.net.URL.openStream(URL.java:1068)
2020-12-10T09:18:18.1794260Z [info]   at scala.io.Source$.fromURL(Source.scala:141)
2020-12-10T09:18:18.1796293Z [info]   at kamon.prometheus.EmbeddedHttpServerSpecSuite.kamon$prometheus$EmbeddedHttpServerSpecSuite$$httpGetMetrics(EmbeddedHttpServerSpec.scala:80)
2020-12-10T09:18:18.1800512Z [info]   at kamon.prometheus.EmbeddedHttpServerSpecSuite$$anonfun$1$$anonfun$apply$mcV$sp$1.apply(EmbeddedHttpServerSpec.scala:36)

I'll check it out later, but we should probably have an additional test that checks that the gzip compression works.

Copy link
Contributor

@SimunKaracic SimunKaracic left a comment

Choose a reason for hiding this comment

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

You're almost over the finish line!

Please fix that one branch, add a test that validates the compression (you can just do what you did with curl in #911), and we're done here :)

@saeltz
Copy link
Contributor Author

saeltz commented Dec 16, 2020

@SimunKaracic, I made the changes you requested but would need some support with the test.

@SimunKaracic
Copy link
Contributor

It looks good!

Next up, small tests:

  1. Assert that we return the correct header in the response (Content-Encoding: gzip)
  2. Another test where we send the same request to the endpoint, once with gzip, once without it, and check to see that the response size is different.

How does that sound?

@saeltz
Copy link
Contributor Author

saeltz commented Dec 22, 2020

The test cases sound good. I'm just not sure how tu run them.

At the moment, the tests are using scala.io.Source.fromURL(new URL(s"http://127.0.0.1:$port$endpoint")) which does not allow for content encodings.
What do you suggest to use instead?

@SimunKaracic
Copy link
Contributor

It should be possible to add headers to the URL, with something like this:
https://stackoverflow.com/a/9453655

Feel free to make another method httpGetGzippedMetrics, or rip apart the existing one for your purposes!
I'd rather have more cases than the cleanest code!

@SimunKaracic
Copy link
Contributor

Nice, quick response 🎉

This looks done, and it's definitely going in the next release!
I'll give it one more thorough overview and try to break it with some curling, but I think your job here is done.

If there's any small changes to be made, I'll just push them directly, you can go enjoy your holidays :D

It was not closing connections correctly on gzip requests
@SimunKaracic SimunKaracic merged commit 7fcb4fe into kamon-io:master Jan 19, 2021
@saeltz saeltz deleted the gzip-metrics branch January 20, 2021 08:13
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.

None yet

2 participants