-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
feat(cloud_pubsub): Add support for gzip compression #13094
Conversation
Also fixed a bug with GzipEncoder & GzipDecoder
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
Thanks so much for the pull request! |
!signed-cla |
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.
Thanks for putting this up! Have you been able to try the artifacts with cloud_pubsub input and output?
I have a few comments but no big concerns at this time.
Currently we run a modification of this code at my company. It's running at a huge scale with no problems and tons of cost savings. Looking to test this branch directly and see if we have any issues. Will update |
I ran this in an linux amd64 container with 18k metrics input from a pubsub and output to a pubsub over a period of 6 hours (3k/hour). |
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.
Thank you for submitting this PR and making some changes. I left a few small comments, but I am going to mark this as approved, so Sven can take a look as well.
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.
@dayvar14 thanks for reviving the matter! I do have a few comments...
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.
This looks good. I have three more comments that improve the formatting a bit. Nothing too big...
Once this looks good, I want to test once more in a large traffic environment to see if this still works as expected. |
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.
One more suggestion...
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 🥳 This pull request decreases the Telegraf binary size by -3.00 % for linux amd64 (new size: 167.6 MB, nightly size 172.8 MB) 📦 Click here to get additional PR build artifactsArtifact URLs |
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.
Thanks a lot @dayvar14 for taking care of this!
Getting this error. @srebhan Are we sure the mutex lock wasn't necessary?
My environment consists of one telegraf instance outputting to a pubsub in gzip format and then another telegraf instance reading from that pubsub. I used the same configuration when testing in previous commits.
|
Can you check with the mutex back in? Please open an issue anyway! |
Mutex lock fixed the issue. Making an issue now |
Required for all PRs
resolves #9157
Revives a PR that was adding support for Cloud PubSub gzip compression