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_stackdriver: support gzip compression #7101

Merged
merged 7 commits into from Jul 12, 2023
Merged

Conversation

CatherineF-dev
Copy link
Contributor

@CatherineF-dev CatherineF-dev commented Apr 1, 2023

Fixes #7089.


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.

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

Documentation

  • Documentation required for this feature

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.

@CatherineF-dev
Copy link
Contributor Author

CatherineF-dev commented Apr 1, 2023

  • Configuration example:
     [OUTPUT]
         # https://docs.fluentbit.io/manual/pipeline/outputs/stackdriver
         Name                      stackdriver
         ...
         compress_gzip             1
  • Debug output:
    Compression rate is about 95%, from 44562525 bytes to \2141049 bytes.

@CatherineF-dev CatherineF-dev changed the title plugin: out_stackdriver: support gzip compression out_stackdriver: support gzip compression Apr 1, 2023
@edsiper
Copy link
Member

edsiper commented Apr 2, 2023

quick review:

once those are fixed we can do another review

@CatherineF-dev
Copy link
Contributor Author

cc @qingling128

@CatherineF-dev
Copy link
Contributor Author

CatherineF-dev commented Apr 12, 2023

It seems splunk is using ctx->compress_gzip. cc @edsiper

https://github.com/fluent/fluent-bit/blob/master/plugins/out_splunk/splunk.c#L600

@CatherineF-dev
Copy link
Contributor Author

Hi team, have changed to compress

@CatherineF-dev
Copy link
Contributor Author

/retest

@CatherineF-dev CatherineF-dev temporarily deployed to pr April 18, 2023 18:25 — with GitHub Actions Inactive
@CatherineF-dev CatherineF-dev temporarily deployed to pr April 18, 2023 18:25 — with GitHub Actions Inactive
@CatherineF-dev CatherineF-dev temporarily deployed to pr April 18, 2023 18:25 — with GitHub Actions Inactive
@CatherineF-dev CatherineF-dev temporarily deployed to pr April 18, 2023 18:48 — with GitHub Actions Inactive
@CatherineF-dev
Copy link
Contributor Author

Hi team, it's ready to review.

@patrick-stephens
Copy link
Contributor

Can you link the docs PR?

@CatherineF-dev
Copy link
Contributor Author

cc @qingling128 Need a LGTM on gzip for out_stackdriver plugin, thanks!

@leonardo-albertovich
Copy link
Collaborator

I have already approved this PR, @edsiper will merge it when he's able to.

@CatherineF-dev
Copy link
Contributor Author

Ping ~

plugins/out_stackdriver/stackdriver_conf.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver_conf.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
compressed_payload_buffer = (flb_sds_t) payload_buf;
compressed_payload_size = flb_sds_len(payload_buf);
if (ctx->compress_gzip == FLB_TRUE) {
ret = flb_gzip_compress((void *) payload_buf, flb_sds_len(payload_buf),
Copy link
Contributor

Choose a reason for hiding this comment

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

[Optional] There's nearly identical code in out_http [1]. Should we look into a refactoring? Or defer? A question for @edsiper

[1] https://github.com/fluent/fluent-bit/blob/master/plugins/out_http/http.c#L137-L152

plugins/out_stackdriver/stackdriver.c Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.h Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
@igorpeshansky
Copy link
Contributor

Plan to add a safe check for NULL compressed

    if (compressed == FLB_TRUE) {
        flb_free(compressed_payload_buffer);
    }

BTW, flb_free uses free [1], which should already handle NULL properly [2].

[1] https://github.com/fluent/fluent-bit/blob/master/include/fluent-bit/flb_mem.h#L127
[2] https://linux.die.net/man/3/free

Signed-off-by: Catherine Fang <yinghongfang@google.com>
Signed-off-by: Catherine Fang <yinghongfang@google.com>
@CatherineF-dev
Copy link
Contributor Author

Thanks, have updated! @igorpeshansky

Okay, we can remove this safe check for flb_free in future PR.

if (ptr) {
flb_free(ptr);
}

plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver_conf.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver_conf.c Outdated Show resolved Hide resolved
Signed-off-by: Catherine Fang <yinghongfang@google.com>
plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.h Outdated Show resolved Hide resolved
CatherineF-dev and others added 2 commits June 25, 2023 07:26
Co-authored-by: igorpeshansky <igorpeshansky@users.noreply.github.com>
Signed-off-by: CatherineF-dev <yinghongfang@google.com>
Co-authored-by: igorpeshansky <igorpeshansky@users.noreply.github.com>
Signed-off-by: CatherineF-dev <yinghongfang@google.com>
Copy link
Contributor

@igorpeshansky igorpeshansky left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

This reverts commit 6621b4b.

Signed-off-by: Catherine Fang <yinghongfang@google.com>
@CatherineF-dev CatherineF-dev temporarily deployed to pr July 8, 2023 01:45 — with GitHub Actions Inactive
@CatherineF-dev CatherineF-dev temporarily deployed to pr July 8, 2023 01:45 — with GitHub Actions Inactive
@CatherineF-dev CatherineF-dev temporarily deployed to pr July 8, 2023 01:45 — with GitHub Actions Inactive
@CatherineF-dev CatherineF-dev temporarily deployed to pr July 8, 2023 02:10 — with GitHub Actions Inactive
@CatherineF-dev
Copy link
Contributor Author

cc @edsiper, this PR is ready to be merged

@igorpeshansky
Copy link
Contributor

cc @leonardo-albertovich as well.

@edsiper edsiper merged commit 708dfd7 into fluent:master Jul 12, 2023
38 of 41 checks passed
leonardo-albertovich pushed a commit that referenced this pull request Jul 17, 2023
Implement gzip compression

Signed-off-by: Catherine Fang <yinghongfang@google.com>
Co-authored-by: igorpeshansky <igorpeshansky@users.noreply.github.com>
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.

Support gzip compression in out_stackdriver plugin
6 participants