cloudwatch_logs: remove sequence tokens from API calls #7973

matthewfala · 2023-09-26T03:28:45Z

Allow multiple workers on cloudwatch_logs by removing sequence tokens 🎉🎉🎉

This PR removes the sequence tokens from CloudWatch Log API requests entirely (as per the recent CWL API update).

Also removes error handling for InvalidSequence and DataAlreadyAccepted exceptions which no longer occur.

CWL API Docs:

The sequence token is now ignored in PutLogEvents actions. PutLogEvents actions are always accepted and never return InvalidSequenceTokenException or DataAlreadyAcceptedException even if the sequence token is not valid. You can use parallel PutLogEvents actions on the same log stream.

https://docs.aws.amazon.com/AmazonCloudWatchLogs/latest/APIReference/API_PutLogEvents.html

Reopened from: #6741

Note: This is a cleaned up version of the following branch https://github.com/PettitWesley/fluent-bit.git 1_9-sequence-token

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.

PettitWesley · 2023-09-26T05:36:46Z

src/aws/flb_aws_util.c

+        /* error can not be parsed, print raw response */
+        flb_plg_error(ins, "Raw response: %s", response);


This might not be the right way to think about things, but one interesting thought I just had- we already have folks complaining about the rates of hits for "error" from Fluent Bit: https://github.com/aws/aws-for-fluent-bit/blob/mainline/troubleshooting/debugging.md#rate-of-network-errors

So I wonder if this should be warn, because its printing a raw response which if its an error, will probably have error in it, so we don't need to further increase our error hits anymore...

🧐

That's reasonable.

PettitWesley · 2023-09-26T05:37:37Z

src/aws/flb_aws_util.c

@@ -596,6 +598,7 @@ void flb_aws_print_error(char *response, size_t response_len,
    }

    flb_sds_destroy(error);
+    return;


Why add empty return?

No reason I can recall. Can remove.

PettitWesley · 2023-09-26T05:38:25Z

Minus one small comment, I think this is probably good to go.

PettitWesley · 2023-09-28T15:49:47Z

plugins/out_cloudwatch_logs/cloudwatch_api.c

-                /* error can not be parsed, print raw response to debug */
-                flb_plg_debug(ctx->ins, "Raw response: %s", c->resp.payload);
+                /* error can not be parsed, print raw response */
+                flb_plg_error(ctx->ins, "Raw response: %s", c->resp.payload);


@matthewfala for same reasoning as my earlier comment on error => warn for raw responses, let's also do warn here?

PettitWesley · 2023-09-28T15:50:12Z

plugins/out_cloudwatch_logs/cloudwatch_api.c

-                /* error can not be parsed, print raw response to debug */
-                flb_plg_debug(ctx->ins, "Raw response: %s", c->resp.payload);
+                /* error can not be parsed, print raw response */
+                flb_plg_error(ctx->ins, "Raw response: %s", c->resp.payload);


same comment here

Signed-off-by: Matthew Fala <[email protected]>

matthewfala · 2023-09-29T03:14:12Z

Updated: https://github.com/fluent/fluent-bit/compare/eeb3009725be1ec21067c232074f57be028d6951..e17679c9efa6766716c86729d44fb71db3638321

PettitWesley · 2023-09-29T15:29:32Z

src/aws/flb_aws_util.c

@@ -581,6 +581,8 @@ void flb_aws_print_error(char *response, size_t response_len,

    error = flb_json_get_val(response, response_len, "__type");
    if (!error) {
+        /* error can not be parsed, print raw response */
+        flb_plg_warn(ins, "Raw response: %s", response);


This lacks full context that it could have: we can print the api value in the raw response here, like: PutLogEvents: Raw Response:

I guess based on the response, the API should be obvious? Is this true? I think if this does really add context, it'd be nice to add.

We could also follow up with it later I suppose, as long we stage it now, so we don't forget it..

What are your thoughts on how important this context is?

I agree this is nice to have but not essential. I'll update this for master, and include it in aws-for-fluent-bit after release of 2.32.0

Cool and thanks!

Here: https://github.com/fluent/fluent-bit/pull/8122/files

PettitWesley

Looks good to go minus one minor CX comment

edsiper · 2023-10-17T14:36:44Z

thanks, merged.

note: commits must be with proper plugin name e.g: out_cloudwatch_logs.

Signed-off-by: Matthew Fala <[email protected]>

cdancy · 2023-12-01T18:00:25Z

@edsiper @PettitWesley @matthewfala is it safe to say that with 2.2.0 release of fluent-bit workers are at least supported with cloudwatch_logs output? I don't see the documentation updated but just wanted to double check with you all.

matthewfala requested a review from PettitWesley as a code owner September 26, 2023 03:28

github-actions bot added the docs-required label Sep 26, 2023

matthewfala temporarily deployed to pr September 26, 2023 03:29 — with GitHub Actions Inactive

matthewfala temporarily deployed to pr September 26, 2023 03:56 — with GitHub Actions Inactive

matthewfala mentioned this pull request Sep 26, 2023

Support New CloudWatch Logs Sequence Token Deprecation and Ingestion Limit Removal aws/aws-for-fluent-bit#526

Closed

PettitWesley reviewed Sep 26, 2023

View reviewed changes

PettitWesley requested changes Sep 26, 2023

View reviewed changes

matthewfala force-pushed the cwl-sequence-token-deprecation branch from 6c9656d to 737cb82 Compare September 28, 2023 00:58

matthewfala temporarily deployed to pr September 28, 2023 00:59 — with GitHub Actions Inactive

matthewfala temporarily deployed to pr September 28, 2023 01:23 — with GitHub Actions Inactive

PettitWesley reviewed Sep 28, 2023

View reviewed changes

matthewfala force-pushed the cwl-sequence-token-deprecation branch from 737cb82 to f6c1c45 Compare September 29, 2023 02:52

matthewfala temporarily deployed to pr September 29, 2023 02:53 — with GitHub Actions Inactive

matthewfala force-pushed the cwl-sequence-token-deprecation branch from f6c1c45 to 6c9656d Compare September 29, 2023 03:02

matthewfala temporarily deployed to pr September 29, 2023 03:02 — with GitHub Actions Inactive

matthewfala force-pushed the cwl-sequence-token-deprecation branch from 6c9656d to 737cb82 Compare September 29, 2023 03:03

matthewfala temporarily deployed to pr September 29, 2023 03:03 — with GitHub Actions Inactive

matthewfala force-pushed the cwl-sequence-token-deprecation branch from 737cb82 to eeb3009 Compare September 29, 2023 03:07

matthewfala temporarily deployed to pr September 29, 2023 03:13 — with GitHub Actions Inactive

cloudwatch_logs: remove sequence tokens from API calls

e17679c

Signed-off-by: Matthew Fala <[email protected]>

matthewfala force-pushed the cwl-sequence-token-deprecation branch from eeb3009 to e17679c Compare September 29, 2023 03:13

matthewfala temporarily deployed to pr September 29, 2023 03:21 — with GitHub Actions Inactive

matthewfala temporarily deployed to pr September 29, 2023 03:51 — with GitHub Actions Inactive

PettitWesley reviewed Sep 29, 2023

View reviewed changes

edsiper merged commit 284a4eb into fluent:master Oct 17, 2023
40 of 41 checks passed

leonardo-albertovich pushed a commit that referenced this pull request Nov 3, 2023

out_cloudwatch_logs: remove sequence tokens from API calls (#7973)

26a4833

Signed-off-by: Matthew Fala <[email protected]>

BrewTestBot mentioned this pull request Nov 9, 2023

fluent-bit 2.2.0 Homebrew/homebrew-core#153758

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cloudwatch_logs: remove sequence tokens from API calls #7973

cloudwatch_logs: remove sequence tokens from API calls #7973

matthewfala commented Sep 26, 2023 •

edited

Loading

PettitWesley Sep 26, 2023

matthewfala Sep 27, 2023

PettitWesley Sep 26, 2023

matthewfala Sep 27, 2023

PettitWesley commented Sep 26, 2023

PettitWesley Sep 28, 2023

PettitWesley Sep 28, 2023

matthewfala commented Sep 29, 2023

PettitWesley Sep 29, 2023

PettitWesley Oct 6, 2023

matthewfala Oct 6, 2023 •

edited

Loading

PettitWesley Oct 6, 2023

matthewfala Nov 2, 2023

PettitWesley left a comment

edsiper commented Oct 17, 2023

cdancy commented Dec 1, 2023

		/* error can not be parsed, print raw response */
		flb_plg_error(ins, "Raw response: %s", response);

cloudwatch_logs: remove sequence tokens from API calls #7973

cloudwatch_logs: remove sequence tokens from API calls #7973

Conversation

matthewfala commented Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PettitWesley commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewfala commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewfala Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PettitWesley left a comment

Choose a reason for hiding this comment

edsiper commented Oct 17, 2023

cdancy commented Dec 1, 2023

matthewfala commented Sep 26, 2023 •

edited

Loading

matthewfala Oct 6, 2023 •

edited

Loading