support retention for cloudwatch_logs output #2739
Conversation
shogo82148
force-pushed
the
introduce-log-retention-days
branch
from
02d3169 to
bfe8988
Compare
shogo82148
requested review from
edsiper,
fujimotos and
koleini
as code owners
PettitWesley
added
the
AWS
|
Thank you for working on adding this to the new core cloudwatch logs plugin! |
|
Per the contributing guide: https://github.com/fluent/fluent-bit/blob/master/CONTRIBUTING.md You commit message should be something like: `out_cloudwatch_logs: add log_retention_days option" |
shogo82148
force-pushed
the
introduce-log-retention-days
branch
from
bfe8988 to
1c04d31
Compare
|
Thanks. I updated the commit message. |
There was a problem hiding this comment.
Thanks again for contributing this. I added a few more comments (sorry, I didn't have time to do full review before). I also tested it- once these are fixed we can merge it.
If you have time, please also submit a PR to the docs repo and @ mention me on it: https://github.com/fluent/fluent-bit-docs
If not, let me know, and I will submit the PR for docs.
| {"put_retention_policy_success", flb_test_cloudwatch_put_retention_policy_success }, | ||
| {"already_exists_create_group_put_retention_policy", flb_test_cloudwatch_already_exists_create_group_put_retention_policy }, | ||
| {"error_put_retention_policy", flb_test_cloudwatch_error_put_retention_policy }, |
There was a problem hiding this comment.
Thank you for adding tests!
shogo82148
force-pushed
the
introduce-log-retention-days
branch
from
1c04d31 to
08e1850
Compare
Addresses fluent/fluent-bit#2739 Signed-off-by: Ichinose Shogo <[email protected]>
|
Thank you for your review! |
| @@ -1058,7 +1142,8 @@ int create_log_group(struct flb_cloudwatch *ctx) | |||
| ctx->group_created = FLB_TRUE; | |||
| flb_sds_destroy(body); | |||
| flb_http_client_destroy(c); | |||
| return 0; | |||
| int ret = set_log_group_retention(ctx); | |||
There was a problem hiding this comment.
always declare variables at the top of the function, per the style guide: https://github.com/fluent/fluent-bit/blob/master/CONTRIBUTING.md#variable-definitions
|
One new comment... then I think this is ready |
|
Hey, actually, thinking about this some more, can you modify the code so that the retention policy is set even if the log group already exists. Right now you only set the retention policy if the CreateLogGroup API responds with 200 success. You can also set the retention policy if it responds that the group already exists (which is a few lines down in the create group function) This has been requested: aws/amazon-cloudwatch-logs-for-fluent-bit#121 |
Signed-off-by: Ichinose Shogo <[email protected]>
shogo82148
force-pushed
the
introduce-log-retention-days
branch
from
08e1850 to
5fe54f3
Compare
|
Sorry for my late response. |
Addresses fluent/fluent-bit#2739 Signed-off-by: Ichinose Shogo <[email protected]>
Addresses fluent/fluent-bit#2739 Signed-off-by: Ichinose Shogo <[email protected]>
Addresses fluent/fluent-bit#2739 Signed-off-by: Ichinose Shogo <[email protected]>
|
#2691 is not stale, but done. |
support retention for
cloudwatch_logsoutput plugin.introduce a new property
log_retention_days.Addresses #2691
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:
Documentation
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.