cloudwatch_logs: remove sequence tokens from API calls #6741

matthewfala · 2023-01-26T19:04:57Z

Signed-off-by: Matthew Fala falamatt@amazon.com

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

Also removes error handling for InvalidSequence and DataAlreadyAccepted exceptions

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

This PR is in testing.

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-01-26T22:00:20Z

Adding a note here that we discussed that we will update our cloudwatch_logs load tests to incrementally test higher throughput now that sequence tokens and ingestion limits are removed.

Right now we have 1, 2, 3 MB/s. We can incrementally increase it next release to 2, 4, 6 MB/s and see those results in our pipeline. If they are good, next time we can increment it again.

In theory, cloudwatch_logs after this change should be able to be tested at the same 30 MB/s that we test other plugins.

https://github.com/aws/aws-for-fluent-bit/releases/tag/v2.30.0

Though @matthewfala noted that the cloudwatch_logs validation takes the longest, so we might need to improve that before we can increase the load too much or might have to stick with a lower load because of it.

CloudWatch can export logs to S3, we may want to look into how fast that is. Validating exported logs in S3 might possibly be faster for high throughput tests than validating against CW.

PettitWesley · 2023-01-26T22:41:00Z

plugins/out_cloudwatch_logs/cloudwatch_api.c

            error = flb_aws_error(c->resp.payload, c->resp.payload_size);
            if (error != NULL) {
-                if (strcmp(error, ERR_CODE_INVALID_SEQUENCE_TOKEN) == 0) {


flb_aws_error and flb_aws_print_error are basically the same.

Here were are now only still using flb_aws_error to check if we have an error so we choose to either print it with flb_aws_print_error or print the full API payload to debug if we can't parse the error.

If you made a small change to add a return value to flb_aws_print_error, it could do the check of whether or not here is an error that we can parse. This simplifies the code.

This does make the code simpler. I've updated the code to reflect this suggestion.

PettitWesley · 2023-01-26T22:56:58Z

plugins/out_cloudwatch_logs/cloudwatch_logs.c

        FLB_OUTPUT_RETURN(FLB_RETRY);
    }

-    // TODO: this msg is innaccurate if events are skipped
-    flb_plg_debug(ctx->ins, "Sent %d events to CloudWatch", event_count);


huh I swear I had already removed this when I added: https://github.com/fluent/fluent-bit/blob/master/plugins/out_cloudwatch_logs/cloudwatch_api.c#L549

You did remove it in the sequence token deprecation branch. That may be what you are thinking.

plugins/out_cloudwatch_logs/cloudwatch_api.c

PettitWesley · 2023-01-26T22:59:23Z

plugins/out_cloudwatch_logs/cloudwatch_logs.c

+    return -1;
+}
+
+struct cw_flush *new_buffer()


As you noted yourself to me, need to also remove the singleplex scheduler.

The singleplex scheduler has been removed.

matthewfala

About to force push my revision.

plugins/out_cloudwatch_logs/cloudwatch_api.c

matthewfala · 2023-01-27T20:38:42Z

plugins/out_cloudwatch_logs/cloudwatch_api.c

            error = flb_aws_error(c->resp.payload, c->resp.payload_size);
            if (error != NULL) {
-                if (strcmp(error, ERR_CODE_INVALID_SEQUENCE_TOKEN) == 0) {


This does make the code simpler. I've updated the code to reflect this suggestion.

matthewfala · 2023-01-27T20:38:56Z

plugins/out_cloudwatch_logs/cloudwatch_logs.c

+    return -1;
+}
+
+struct cw_flush *new_buffer()


The singleplex scheduler has been removed.

matthewfala · 2023-01-27T20:39:36Z

plugins/out_cloudwatch_logs/cloudwatch_logs.c

        FLB_OUTPUT_RETURN(FLB_RETRY);
    }

-    // TODO: this msg is innaccurate if events are skipped
-    flb_plg_debug(ctx->ins, "Sent %d events to CloudWatch", event_count);


You did remove it in the sequence token deprecation branch. That may be what you are thinking.

matthewfala · 2023-01-27T20:39:49Z

plugins/out_cloudwatch_logs/cloudwatch_logs.c

@@ -605,7 +603,7 @@ static struct flb_config_map config_map[] = {
     "errors, which may help improve throughput when there are transient/random "
     "networking issues."
    },
-
+    


Remove stray whitespace

PettitWesley · 2023-01-27T22:54:35Z

plugins/out_cloudwatch_logs/cloudwatch_api.c

-                flb_sds_destroy(error);
-            }
-            else {
+            /* some other error occurred; notify user */


nit: remove word "other"

PettitWesley · 2023-01-27T22:55:08Z

include/fluent-bit/flb_aws_util.h

+ * Returns 0 if an error cannot be found in the response,
+ * and 1 if the error is found and printed.
 */
-void flb_aws_print_error(char *response, size_t response_len,
+int flb_aws_print_error(char *response, size_t response_len,


Why these return values?? Let's use either typical 0 success and -1 failure or FLB_TRUE and FLB_ERROR

PettitWesley · 2023-01-27T22:55:59Z

plugins/out_cloudwatch_logs/cloudwatch_api.c

+            /* some other error occurred; notify user */
+            is_error_parsed = flb_aws_print_error(c->resp.payload, c->resp.payload_size,
+                                               "PutRetentionPolicy", ctx->ins);
+            if (!is_error_parsed) {


let's use fluent bit best practice of comparing directly with FLB_TRUE or FLB_FALSE. I think its more readable too.

PettitWesley · 2023-01-27T22:58:12Z

plugins/out_cloudwatch_logs/cloudwatch_api.c

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


Do you think we should make this a non-debug log?? I kind of think we should, otherwise user is missing the clear error response.

Of course, this case shouldn't really happen unless we get a truncated response or something such that the AWS API error can't be parsed.

matthewfala · 2023-01-28T02:54:59Z

Here are the changes since your last review: https://github.com/fluent/fluent-bit/compare/3aa6cd4..b10ffbb45cd531b0ae2a5647519ce4765b664625#diff-d70aca905ecc02fb9ccb4518905d0caefe2a8e5254b4cbe69dc930291be75be4L1259

Signed-off-by: Matthew Fala <falamatt@amazon.com>

PettitWesley

good idea on just printing the response in aws_print_error instead of my over complicated idea of the return value for the same!

github-actions · 2023-05-01T01:57:11Z

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

matthewfala requested a review from PettitWesley as a code owner January 26, 2023 19:04

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

matthewfala temporarily deployed to pr January 26, 2023 19:05 — with GitHub Actions Inactive

matthewfala temporarily deployed to pr January 26, 2023 19:06 — with GitHub Actions Inactive

matthewfala temporarily deployed to pr January 26, 2023 19:22 — with GitHub Actions Inactive

PettitWesley mentioned this pull request Jan 26, 2023

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

Closed

PettitWesley reviewed Jan 26, 2023

View reviewed changes

plugins/out_cloudwatch_logs/cloudwatch_api.c Show resolved Hide resolved

PettitWesley reviewed Jan 26, 2023

View reviewed changes

matthewfala commented Jan 27, 2023

View reviewed changes

matthewfala force-pushed the sequence-token-dep-1.9.10 branch from 813d38d to adb05e6 Compare January 27, 2023 20:43

matthewfala requested review from edsiper, leonardo-albertovich, fujimotos and koleini as code owners January 27, 2023 20:43

matthewfala temporarily deployed to pr January 27, 2023 20:44 — with GitHub Actions Inactive

matthewfala force-pushed the sequence-token-dep-1.9.10 branch from adb05e6 to 3aa6cd4 Compare January 27, 2023 20:44

matthewfala temporarily deployed to pr January 27, 2023 20:45 — with GitHub Actions Inactive

matthewfala temporarily deployed to pr January 27, 2023 20:59 — with GitHub Actions Inactive

PettitWesley reviewed Jan 27, 2023

View reviewed changes

matthewfala force-pushed the sequence-token-dep-1.9.10 branch from 3aa6cd4 to c7ff858 Compare January 28, 2023 02:40

matthewfala had a problem deploying to pr January 28, 2023 02:40 — with GitHub Actions Failure

matthewfala force-pushed the sequence-token-dep-1.9.10 branch from c7ff858 to e81b3e9 Compare January 28, 2023 02:45

matthewfala had a problem deploying to pr January 28, 2023 02:46 — with GitHub Actions Failure

matthewfala had a problem deploying to pr January 28, 2023 02:47 — with GitHub Actions Failure

matthewfala force-pushed the sequence-token-dep-1.9.10 branch from e81b3e9 to b10ffbb Compare January 28, 2023 02:52

matthewfala had a problem deploying to pr January 28, 2023 02:52 — with GitHub Actions Failure

matthewfala added 3 commits January 28, 2023 03:22

cloudwatch_logs: remove sequence tokens from API calls

28e6861

Signed-off-by: Matthew Fala <falamatt@amazon.com>

kinesis_firehose: print api malformed error message to plg_error

08ef05b

Signed-off-by: Matthew Fala <falamatt@amazon.com>

kinesis_streams: print api malformed error message to plg_error

23b1509

Signed-off-by: Matthew Fala <falamatt@amazon.com>

matthewfala force-pushed the sequence-token-dep-1.9.10 branch from b10ffbb to 23b1509 Compare January 28, 2023 03:22

matthewfala temporarily deployed to pr January 28, 2023 03:23 — with GitHub Actions Inactive

matthewfala temporarily deployed to pr January 28, 2023 03:42 — with GitHub Actions Inactive

PettitWesley approved these changes Jan 30, 2023

View reviewed changes

github-actions bot added the Stale label May 1, 2023

matthewfala mentioned this pull request Sep 26, 2023

cloudwatch_logs: remove sequence tokens from API calls #7973

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cloudwatch_logs: remove sequence tokens from API calls #6741

cloudwatch_logs: remove sequence tokens from API calls #6741

matthewfala commented Jan 26, 2023 •

edited

Loading

PettitWesley commented Jan 26, 2023

PettitWesley Jan 26, 2023

matthewfala Jan 27, 2023

PettitWesley Jan 26, 2023

matthewfala Jan 27, 2023

PettitWesley Jan 26, 2023

matthewfala Jan 27, 2023

matthewfala left a comment

matthewfala Jan 27, 2023

matthewfala Jan 27, 2023

matthewfala Jan 27, 2023

matthewfala Jan 27, 2023

PettitWesley Jan 27, 2023

PettitWesley Jan 27, 2023

PettitWesley Jan 27, 2023

PettitWesley Jan 27, 2023

matthewfala commented Jan 28, 2023

PettitWesley left a comment

github-actions bot commented May 1, 2023

cloudwatch_logs: remove sequence tokens from API calls #6741

Are you sure you want to change the base?

cloudwatch_logs: remove sequence tokens from API calls #6741

Conversation

matthewfala commented Jan 26, 2023 • edited Loading

PettitWesley commented Jan 26, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewfala left a comment

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

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

Choose a reason for hiding this comment

matthewfala commented Jan 28, 2023

PettitWesley left a comment

Choose a reason for hiding this comment

github-actions bot commented May 1, 2023

matthewfala commented Jan 26, 2023 •

edited

Loading