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

out_opensearch: Add gzip compression #7381

Merged

Conversation

wndhydrnt
Copy link
Contributor

@wndhydrnt wndhydrnt commented May 14, 2023

This change adds support for compressing the payload sent to an OpenSearch server using gzip.

Compression is off by default, similar to other outputs that support compression.


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • Example configuration file for the change
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • [N/A] Run local packaging test showing all targets (including any new ones) build.
  • [N/A] Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • Documentation required for this feature

fluent/fluent-bit-docs#1104

Backporting

  • Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

@wndhydrnt wndhydrnt force-pushed the feature-opensearch-compression branch from 984d90e to 825ebea Compare May 14, 2023 18:40
@wndhydrnt
Copy link
Contributor Author

Example configuration file

[SERVICE]
    Flush 1
    Log_Level debug

[INPUT]
    Name dummy
    Samples 1

[OUTPUT]
    Name  opensearch
    Match *
    Host  host.docker.internal
    Port  9200
    tls On
    tls.verify Off
    HTTP_User admin
    HTTP_Passwd admin
    Suppress_Type_Name On
    Compress gzip

Debug log

debug.log

Command to start OpenSearch:

docker run -p 9200:9200 -p 9600:9600 -e "discovery.type=single-node" --name opensearch-node -d opensearchproject/opensearch:2.7.0

Valgrind output

valgrind.log

Valgrind repeatedly prints the message Syscall param epoll_ctl(event) points to uninitialised byte(s).
It looks like this is a known issue on aarch64 and does not relate to this change. Bug 422623) tracks the issue. It has been fixed in release 3.17.0 of Valgrind.

@cosmo0920
Copy link
Contributor

Would you mind if you added the test case for your change?
Here is the corresponding unit test file for out_opensearch: https://github.com/fluent/fluent-bit/blob/master/tests/runtime/out_opensearch.c
It would be better to confirm this patch with unit tests.

@wndhydrnt
Copy link
Contributor Author

Hey @cosmo0920, thank you for your suggestion.

When writing the change, I could not come up with a good way to test it.
My idea would be to check that, when setting Compress gzip, the payload sent to OpenSearch is properly gzipped and that the header Content-Encoding: gzip is set too. That would require to mock the HTTP client, but I could not figure out how to do this. There are also no tests that validate gzip compression in other outputs on which a new test could be based.

I have to admit that this is the first time for me contributing to a codebase written in C, so it might just be a matter of lack of experience. It would be helpful to me if you can provide some hints on how to approach the implementation of a test.

@leonardo-albertovich
Copy link
Collaborator

To be fair there is no easy way to do it (in our codebase) that I know of and this is a fairly rutinary change (there's been a few of the same within the past few weeks) so as long as it's approved by a maintainer I think we could skip the unit test and make a note to ourselves to improve that aspect of the testing system.

plugins/out_opensearch/opensearch.c Show resolved Hide resolved
plugins/out_opensearch/opensearch.c Outdated Show resolved Hide resolved
@leonardo-albertovich
Copy link
Collaborator

Thanks for fixing those issues @wndhydrnt, this is ready to be merged.

@wndhydrnt wndhydrnt temporarily deployed to pr May 16, 2023 16:12 — with GitHub Actions Inactive
@wndhydrnt wndhydrnt temporarily deployed to pr May 16, 2023 16:12 — with GitHub Actions Inactive
@wndhydrnt wndhydrnt temporarily deployed to pr May 16, 2023 16:13 — with GitHub Actions Inactive
@wndhydrnt wndhydrnt temporarily deployed to pr May 16, 2023 16:38 — with GitHub Actions Inactive
@leonardo-albertovich leonardo-albertovich merged commit bd713e7 into fluent:master May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants