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: Add partialSuccess: true to all logs sent to Google Cloud Logging API #8002

Merged

Conversation

avilevy18
Copy link
Contributor

@avilevy18 avilevy18 commented Oct 3, 2023

Adding partialSuccess: true as part of the request to Google Cloud Logging API.


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
[INPUT]
    Name                          tail
    Path                          <path-to-log-file>
    Tag                           foo
    Read_from_head                true
    Buffer_Max_Size               100MB
    Buffer_Chunk_Size             512k
[OUTPUT]
    Match                         *
    Name                          stackdriver
    Retry_Limit                   3
    http_request_key              logging.googleapis.com/httpRequest
    net.connect_timeout_log_error False
    resource                      gce_instance
    stackdriver_agent             Google-Cloud-Ops-Agent-Logging/latest (BuildDistro=build_distro;Platform=linux;ShortName=linux_platform;ShortVersion=linux_platform_version)
    storage.total_limit_size      2G
    tls                           On
    tls.verify                    Off
    workers                       8
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found

valgrind --leak-check=full ./fluent-bit --conf=<path>

==1339656== 
==1339656== HEAP SUMMARY:
==1339656==     in use at exit: 0 bytes in 0 blocks
==1339656==   total heap usage: 10,413 allocs, 10,413 frees, 7,432,311 bytes allocated
==1339656== 
==1339656== All heap blocks were freed -- no leaks are possible
==1339656== 
==1339656== For lists of detected and suppressed errors, rerun with: -s
==1339656== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

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.

@avilevy18 avilevy18 force-pushed the avilevy-stackdriver-partial-success branch from 49fec1c to 759a2e8 Compare October 4, 2023 15:06
@braydonk braydonk self-assigned this Oct 4, 2023
@braydonk braydonk changed the title Add partialSuccess: true to all logs sent to Google Cloud Logging API out_stackdriver: Add partialSuccess: true to all logs sent to Google Cloud Logging API Oct 4, 2023
@avilevy18 avilevy18 force-pushed the avilevy-stackdriver-partial-success branch from 759a2e8 to fea1d2c Compare October 4, 2023 15:41
@avilevy18 avilevy18 temporarily deployed to pr October 4, 2023 15:52 — with GitHub Actions Inactive
@avilevy18 avilevy18 temporarily deployed to pr October 4, 2023 15:52 — with GitHub Actions Inactive
@avilevy18 avilevy18 temporarily deployed to pr October 4, 2023 15:52 — with GitHub Actions Inactive
@avilevy18 avilevy18 temporarily deployed to pr October 4, 2023 17:23 — with GitHub Actions Inactive
@avilevy18 avilevy18 temporarily deployed to pr October 4, 2023 17:23 — with GitHub Actions Inactive
@avilevy18 avilevy18 temporarily deployed to pr October 4, 2023 17:23 — with GitHub Actions Inactive
@avilevy18 avilevy18 temporarily deployed to pr October 4, 2023 17:49 — with GitHub Actions Inactive
@avilevy18 avilevy18 force-pushed the avilevy-stackdriver-partial-success branch from da3738b to 5980688 Compare October 4, 2023 18:58
@avilevy18 avilevy18 temporarily deployed to pr October 4, 2023 19:17 — with GitHub Actions Inactive
@avilevy18 avilevy18 temporarily deployed to pr October 4, 2023 19:17 — with GitHub Actions Inactive
@avilevy18 avilevy18 temporarily deployed to pr October 4, 2023 19:17 — with GitHub Actions Inactive
@avilevy18 avilevy18 force-pushed the avilevy-stackdriver-partial-success branch from 5980688 to b379865 Compare October 4, 2023 19:20
@avilevy18 avilevy18 temporarily deployed to pr October 4, 2023 19:21 — with GitHub Actions Inactive
@avilevy18 avilevy18 temporarily deployed to pr October 4, 2023 19:21 — with GitHub Actions Inactive
@avilevy18 avilevy18 temporarily deployed to pr October 4, 2023 19:21 — with GitHub Actions Inactive
@avilevy18 avilevy18 changed the title out_stackdriver: Add partialSuccess: true to all logs sent to Google Cloud Logging API out_stackdriver: Add partialSuccess: true to all logs sent to Google Cloud Logging API Oct 4, 2023
@avilevy18 avilevy18 temporarily deployed to pr October 4, 2023 19:49 — with GitHub Actions Inactive
Copy link
Contributor

@braydonk braydonk left a comment

Choose a reason for hiding this comment

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

General note, please change these style guide things where necessary:

  • braces opening a new function (and only when opening a new function) should go on a newline
  • Line length should not exceed 90 columns, so long lines need to be reformatted so they don't exceed that (see other examples where arguments to functions or in function signatures are put on newlines to fit this requirement)

plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
@@ -2637,6 +2728,10 @@ static void cb_stackdriver_flush(struct flb_event_chunk *event_chunk,
}
else if (c->resp.status >= 400 && c->resp.status < 500) {
ret_code = FLB_ERROR;
if(c->resp.status == 400){
ret = parse_partial_success_response(c, ctx, &partial_failure);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you be checking the value of ret here to see if it worked?

@@ -2637,6 +2728,10 @@ static void cb_stackdriver_flush(struct flb_event_chunk *event_chunk,
}
else if (c->resp.status >= 400 && c->resp.status < 500) {
ret_code = FLB_ERROR;
if(c->resp.status == 400){
ret = parse_partial_success_response(c, ctx, &partial_failure);
cmt_counter_add(ctx->cmt_failed_requests, ts, (double)partial_failure, 1, (char *[]) {name});
Copy link
Contributor

Choose a reason for hiding this comment

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

We're still counting this as one "failed request". We're already going to be increasing this counter further down this function.

The number of failures in the partial failure should be added to cmt_dropped_records. You can access that counter through ctx->ins->cmt_dropped_records.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the point was to count the request as "successful", but count the records themselves as erroneous?..

plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
@@ -2637,6 +2728,10 @@ static void cb_stackdriver_flush(struct flb_event_chunk *event_chunk,
}
else if (c->resp.status >= 400 && c->resp.status < 500) {
ret_code = FLB_ERROR;
if(c->resp.status == 400){
ret = parse_partial_success_response(c, ctx, &partial_failure);
cmt_counter_add(ctx->cmt_failed_requests, ts, (double)partial_failure, 1, (char *[]) {name});
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the point was to count the request as "successful", but count the records themselves as erroneous?..

plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
@avilevy18 avilevy18 temporarily deployed to pr October 19, 2023 18:41 — with GitHub Actions Inactive
@avilevy18 avilevy18 temporarily deployed to pr October 19, 2023 18:41 — with GitHub Actions Inactive
@avilevy18 avilevy18 temporarily deployed to pr October 19, 2023 18:41 — with GitHub Actions Inactive
@avilevy18 avilevy18 temporarily deployed to pr October 19, 2023 19:07 — with GitHub Actions Inactive
plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
Copy link
Contributor

@braydonk braydonk left a comment

Choose a reason for hiding this comment

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

I think it would be good to give this another valgrind, lots of stuff has been added since the last time.

plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Show resolved Hide resolved
braydonk
braydonk previously approved these changes Nov 20, 2023
Copy link
Contributor

@braydonk braydonk left a comment

Choose a reason for hiding this comment

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

LGTM!

plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
braydonk
braydonk previously approved these changes Nov 20, 2023
plugins/out_stackdriver/stackdriver.c Outdated Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.c Show resolved Hide resolved
plugins/out_stackdriver/stackdriver.h Outdated Show resolved Hide resolved
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:

@braydonk
Copy link
Contributor

Before merge, could you flatten all the "Addressed comments" commit into the base one?

@avilevy18 avilevy18 force-pushed the avilevy-stackdriver-partial-success branch from b38ead0 to 329a565 Compare November 21, 2023 16:21
Add partialSuccess: true to all logs sent to Google Cloud Logging API

Signed-off-by: avilevy <[email protected]>
@avilevy18 avilevy18 force-pushed the avilevy-stackdriver-partial-success branch from 329a565 to 83f8c4c Compare November 21, 2023 16:27
@braydonk braydonk merged commit 2555bd4 into fluent:master Nov 21, 2023
44 checks passed
igorpeshansky pushed a commit to igorpeshansky/fluent-bit that referenced this pull request Nov 29, 2023
…tial-success

out_stackdriver: Add partialSuccess: true to all logs sent to Google Cloud Logging API
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