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

[aws-for-fluent-bit] Add helm chart high performance kinesis stream plugin #951

Merged
merged 18 commits into from
Jul 26, 2023

Conversation

changhyuni
Copy link
Contributor

@changhyuni changhyuni commented May 25, 2023

Issue

Description of changes

reference document: https://github.com/aws/amazon-kinesis-streams-for-fluent-bit#how-can-i-migrate-to-the-higher-performance-plugin

  • Added high performance kinesis stream plugin support for aws-for-fluent-bit Helm Chart
  • Updated ConfigMap and values.yaml
  • Updated README doc for Helm Chart

Checklist

  • Added/modified documentation as required (such as the README.md for modified charts)
  • Incremented the chart version in Chart.yaml for the modified chart(s)
  • Manually tested. Describe what testing was done in the testing section below
  • Make sure the title of the PR is a good description that can go into the release notes

Testing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -91,6 +91,15 @@ helm delete aws-for-fluent-bit --namespace kube-system
| `firehose.timeKey` | Add the timestamp to the record under this key. By default the timestamp from Fluent Bit will not be added to records sent to Kinesis. | |
| `firehose.timeKeyFormat` | strftime compliant format string for the timestamp; for example, `%Y-%m-%dT%H:%M:%S%z`. This option is used with `time_key`. | |
| `firehose.extraOutputs` | Append extra outputs with value | `""` |
| `kinesis_streams.enabled` | Whether this plugin should be enabled or not, [details](https://github.com/aws/amazon-kinesis-streams-for-fluent-bit) | `false` | ✔
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@PettitWesley PettitWesley left a comment

Choose a reason for hiding this comment

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

Thank you for adding this!! Few comments/fixes.

| `kinesis_streams.enabled` | Whether this plugin should be enabled or not, [details](https://github.com/aws/amazon-kinesis-streams-for-fluent-bit) | `false` | ✔
| `kinesis_streams.region` | The AWS region. | ✔
| `kinesis_streams.stream` | The name of the Kinesis Streams Delivery Stream that you want log records send to. | ✔
| `kinesis_streams.sts_endpoint` | Custom endpoint for the STS API. `kinesis_streams.sts_endpoint`. | |
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be missing a few options:

  • endpoint
  • role_arn

Copy link
Contributor Author

@changhyuni changhyuni May 26, 2023

Choose a reason for hiding this comment

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

@PettitWesley Thnak you for review!
I updated readme your comment.

| `kinesis_streams.sts_endpoint` | Custom endpoint for the STS API. `kinesis_streams.sts_endpoint`. | |
| `kinesis_streams.time_key` | Add the timestamp to the record under this key. By default the timestamp from Fluent Bit will not be added to records sent to Kinesis. | |
| `kinesis_streams.time_key_format` | strftime compliant format string for the timestamp; for example, the default is `%Y-%m-%dT%H:%M:%S`. Supports millisecond precision with `%3N` and supports nanosecond precision with `%9N` and `%L`; for example, adding `%3N` to support millisecond `%Y-%m-%dT%H:%M:%S.%3N`. This option is used with `time_key`. | |
| `kinesis_streams.log_key` | By default, the whole log record will be sent to Kinesis. If you specify a key name with this option, then only the value of that key will be sent to Kinesis. For example, if you are using the Fluentd Docker log driver, you can specify `log_key` `log` and only the log message will be sent to Kinesis. | |
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Please use log_key log same formatting as the public docs do to make it clear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PettitWesley i fixed readme thank you.

@@ -91,6 +91,17 @@ helm delete aws-for-fluent-bit --namespace kube-system
| `firehose.timeKey` | Add the timestamp to the record under this key. By default the timestamp from Fluent Bit will not be added to records sent to Kinesis. | |
| `firehose.timeKeyFormat` | strftime compliant format string for the timestamp; for example, `%Y-%m-%dT%H:%M:%S%z`. This option is used with `time_key`. | |
| `firehose.extraOutputs` | Append extra outputs with value | `""` |
| `kinesis_streams.enabled` | It has all the core features of the `aws/amazon-kinesis-streams-for-fluent-bit` Golang Fluent Bit plugin released in 2019. The Golang plugin was named `kinesis`; this new high performance and highly efficient kinesis plugin is called `kinesis_streams` to prevent conflicts/confusion, [details](https://docs.fluentbit.io/manual/pipeline/outputs/kinesis) | `false` | ✔
Copy link
Contributor

Choose a reason for hiding this comment

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

aws/amazon-kinesis-streams-for-fluent-bit => this could be a link

Copy link
Contributor

@PettitWesley PettitWesley left a comment

Choose a reason for hiding this comment

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

Please make the one readme change to add a link, then please confirm for me that you have tested this, and then we should be good to go

@PettitWesley
Copy link
Contributor

I may forget about this PR, @ mention me to remind me.

@changhyuni
Copy link
Contributor Author

@PettitWesley fixed!
What do you think about the url name "detail"?

Copy link
Contributor

@PettitWesley PettitWesley left a comment

Choose a reason for hiding this comment

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

@changhyuni thanks, I think the link "details" title is fine... you could also change it to "documentation" instead

@PettitWesley
Copy link
Contributor

@changhyuni once we get to the final version, can you please paste the results of generating the config map from an example with values.yaml containing all kinesis_streams config options as evidence of testing. Then I can approve.

@PettitWesley
Copy link
Contributor

Please @ me when ready, I have a lot on my backlog and may forget this PR otherwise

@changhyuni
Copy link
Contributor Author

@PettitWesley I'm sorry for replying late
I tested it with my company account and it works well
I erased the company information
image

| `kinesis_streams.stream` | The name of the Kinesis Streams Delivery Stream that you want log records send to. | ✔
| `kinesis_streams.endpoint` | Specify a custom endpoint for the Kinesis Streams API. | |
| `kinesis_streams.role_arn` | ARN of an IAM role to assume (for cross account access). | |
| `kinesis_streams.sts_endpoint` | Custom endpoint for the STS API. `kinesis_streams.sts_endpoint`. | |
Copy link
Contributor

Choose a reason for hiding this comment

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

Custom endpoint for the STS API. kinesis_streams.sts_endpoint

Why add the config option name again? kinesis_streams.sts_endpoint

Copy link
Contributor

Choose a reason for hiding this comment

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

Its redundant and not done for any other option.

PettitWesley
PettitWesley previously approved these changes Jul 10, 2023
@PettitWesley
Copy link
Contributor

@changhyuni CI failed, looks like you need to bump chart version again, we did a release recently.

@changhyuni
Copy link
Contributor Author

@PettitWesley thanks you!
i fixed it!

@@ -91,6 +91,17 @@ helm delete aws-for-fluent-bit --namespace kube-system
| `firehose.timeKey` | Add the timestamp to the record under this key. By default the timestamp from Fluent Bit will not be added to records sent to Kinesis. | |
| `firehose.timeKeyFormat` | strftime compliant format string for the timestamp; for example, `%Y-%m-%dT%H:%M:%S%z`. This option is used with `time_key`. | |
| `firehose.extraOutputs` | Append extra outputs with value | `""` |
| `kinesis_streams.enabled` | It has all the core features of the [documentation](https://github.com/aws/amazon-kinesis-streams-for-fluent-bit) Golang Fluent Bit plugin released in 2019. The Golang plugin was named `kinesis`; this new high performance and highly efficient kinesis plugin is called `kinesis_streams` to prevent conflicts/confusion, [details](https://docs.fluentbit.io/manual/pipeline/outputs/kinesis) | `false` | ✔
Copy link
Contributor

Choose a reason for hiding this comment

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

This reads a bit awkward. I'd do:

"Enable the high performance kinesis plugin; it has all the core features of the Golang Fluent Bit plugin..

PettitWesley
PettitWesley previously approved these changes Jul 12, 2023
@PettitWesley
Copy link
Contributor

PettitWesley commented Jul 12, 2023

Screen Shot 2023-07-12 at 4 00 49 PM

@changhyuni sorry but it says there are still conflicts...

I think you need a rebase:

git checkout master
git pull
git checkout update/aws-for-fluent-bit # feature branch
git rebase master
git push --force

@changhyuni
Copy link
Contributor Author

@PettitWesley thanks you,
I solved it

@PettitWesley
Copy link
Contributor

@changhyuni Sorry it still says I can't merge it. Sometimes Github UI rebase does not work and you need to rebase on command line: #951 (comment)

Screen Shot 2023-07-13 at 10 23 09 AM

@PettitWesley PettitWesley merged commit 41f2640 into aws:master Jul 26, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants