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

feat(outputs.file): Add compression #13245

Merged
merged 29 commits into from
Jul 6, 2023
Merged

feat(outputs.file): Add compression #13245

merged 29 commits into from
Jul 6, 2023

Conversation

zekena2
Copy link
Contributor

@zekena2 zekena2 commented May 5, 2023

Required for all PRs

resolves #12875

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label May 5, 2023
@zekena2 zekena2 changed the title feat: add zstd compression to the file output plugin feat(outputs.file): add zstd compression to the file output plugin May 6, 2023
Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this. Could you also add a test for the Init error scenario's?

plugins/outputs/file/sample.conf Outdated Show resolved Hide resolved
plugins/outputs/file/file.go Outdated Show resolved Hide resolved
plugins/outputs/file/file.go Outdated Show resolved Hide resolved
plugins/outputs/file/file.go Outdated Show resolved Hide resolved
plugins/outputs/file/file_test.go Outdated Show resolved Hide resolved
@Hipska Hipska added the plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins label May 8, 2023
@zekena2 zekena2 requested a review from Hipska May 9, 2023 06:19
@Hipska
Copy link
Contributor

Hipska commented May 9, 2023

Would it be much work to also include support for Gzip with internal.CompressWithGzip?

plugins/outputs/file/file.go Outdated Show resolved Hide resolved
plugins/outputs/file/file.go Outdated Show resolved Hide resolved
plugins/outputs/file/file.go Outdated Show resolved Hide resolved
plugins/outputs/file/file_test.go Outdated Show resolved Hide resolved
plugins/outputs/file/file_test.go Outdated Show resolved Hide resolved
remove some exported code and little refactor in tests.

Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>
@zekena2
Copy link
Contributor Author

zekena2 commented May 9, 2023

For using internal.CompressWithGzip I think it's better to use either the stdlib or the the current library to be able to specify the level. I prefer the current library klauspost as it looks optimized more than the stdlib.

plugins/outputs/file/file.go Outdated Show resolved Hide resolved
@zekena2 zekena2 requested a review from Hipska May 10, 2023 15:06
@zekena2
Copy link
Contributor Author

zekena2 commented May 10, 2023

I think the test failed because Circle-CI had a minor outage.

Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

Great improvements!

go.mod Outdated Show resolved Hide resolved
plugins/outputs/file/sample.conf Outdated Show resolved Hide resolved
plugins/outputs/file/file.go Outdated Show resolved Hide resolved
plugins/outputs/file/file.go Outdated Show resolved Hide resolved
plugins/outputs/file/file.go Outdated Show resolved Hide resolved
plugins/outputs/file/file_test.go Outdated Show resolved Hide resolved
plugins/outputs/file/file_test.go Outdated Show resolved Hide resolved
plugins/outputs/file/file_test.go Outdated Show resolved Hide resolved
plugins/outputs/file/file_test.go Outdated Show resolved Hide resolved
plugins/outputs/file/file_test.go Show resolved Hide resolved
Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

Only this minor remark

plugins/outputs/file/file.go Show resolved Hide resolved
@Hipska Hipska added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label May 11, 2023
@zekena2 zekena2 changed the title feat(outputs.file): add zstd compression to the file output plugin feat(outputs.file): add zstd and gzip compression to the file output plugin May 11, 2023
Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

@zekena2 thanks for the good idea and the PR. I want you to look into the existing encoder implementation instead of reimplementing part of it.

plugins/outputs/file/file.go Outdated Show resolved Hide resolved
@srebhan srebhan self-assigned this May 17, 2023
@powersj
Copy link
Contributor

powersj commented Jun 28, 2023

@Hipska Can you add the ready for final review label?

@srebhan needs to +1 this first - I've re-requested a review from him

@powersj powersj requested review from srebhan and removed request for srebhan June 28, 2023 17:03
@zekena2
Copy link
Contributor Author

zekena2 commented Jul 4, 2023

Is there any updates on this?

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

@zekena2 sorry for this taking so long on my side! I do have some more small comments...

plugins/outputs/file/file.go Outdated Show resolved Hide resolved
plugins/outputs/file/file_test.go Outdated Show resolved Hide resolved
plugins/outputs/file/sample.conf Outdated Show resolved Hide resolved
plugins/outputs/file/sample.conf Outdated Show resolved Hide resolved
@zekena2 zekena2 requested a review from srebhan July 4, 2023 14:24
plugins/outputs/file/file.go Outdated Show resolved Hide resolved
plugins/outputs/file/file.go Outdated Show resolved Hide resolved
Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

@zekena2 not sure why you always come back to a special handling of identity but I really think it should be no different to other algorithms. Furthermore, the sample.conf description does not match the internal code, so one of them (or both) are wrong...

plugins/outputs/file/README.md Outdated Show resolved Hide resolved
plugins/outputs/file/file.go Outdated Show resolved Hide resolved
@zekena2
Copy link
Contributor Author

zekena2 commented Jul 4, 2023

The docs in sample.conf matches the code it's just an error message that's causing the confusion. The supported levels are in this line and they're four levels listed in the flate package here which is then used by gzip and zlib.

@srebhan
Copy link
Contributor

srebhan commented Jul 5, 2023

The docs in sample.conf matches the code it's just an error message that's causing the confusion. The supported levels are in this line and they're four levels listed in the flate package here which is then used by gzip and zlib.

Ok let me repeat my initial critique/idea: I don't want to expose the inner-workings of the underlying algorithms! So by passing on those values to the user(!!!) we open-up all possible trouble paths. What if one of the library changes the values? What if we need to switch to another implementation with other values?

Do you remember the beginning where I suggested to have best speed, default and best compression? This would be a suitable abstraction still IMO. You and me are engineers loving to dig into technical details, but there are also users out there that don't want to read the Telegraf config, then try to find out what level 3 stands for requiring them to dig into the code to extract which library we use to read the documentation of that library to check what level 3 means.

So my suggested behavior is:

  • if you specify a valid level (0,1,9 for gzip and zstd or 1,3,7,11 for zlib) you get exactly that level
  • if you specify a value but it is not valid, you get an error
  • if you don't specify a level you get the default of the algorithm

With my code, the plugin implements exactly this. As I said above, instead of the numeric levels I would have loved to just use three values best speed, default and best compression as I think there is no real-world case that needs more. :-)

@zekena2 zekena2 requested a review from srebhan July 5, 2023 19:59
@Hipska Hipska added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Jul 6, 2023
Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Some more clarifications...

internal/content_coding.go Outdated Show resolved Hide resolved
plugins/outputs/file/file.go Outdated Show resolved Hide resolved
plugins/outputs/file/file.go Outdated Show resolved Hide resolved
@zekena2 zekena2 requested review from srebhan and Hipska July 6, 2023 12:51
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Jul 6, 2023

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @zekena2!

@srebhan srebhan assigned powersj and unassigned srebhan Jul 6, 2023
@srebhan srebhan changed the title feat(outputs.file): add compression to the file output plugin feat(outputs.file): Add compression Jul 6, 2023
@powersj powersj merged commit 7aa3d79 into influxdata:master Jul 6, 2023
25 checks passed
@srebhan srebhan added this to the v1.28.0 milestone Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add compression for the file output plugin (prefereably zstd)
4 participants