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

(feat)! : outdated aws-for-fluent-bit chart updated to use new high performance cloudwatch_logs plugin #903

Merged
merged 22 commits into from
Mar 19, 2023

Conversation

deepend-dev
Copy link
Contributor

@deepend-dev deepend-dev commented Feb 19, 2023

Issue

Resolves below issue/s:
#901

Also can be added as resolver of:
#340
#719
#436
#833
#671

Current fluent bit helm chart is outdated.

Aligning with recommendations from current repo to use New Higher Performance Core Fluent Bit Plugin.(https://github.com/fluent/fluent-bit/tree/master/plugins/out_cloudwatch_logs) is directly integrated into fluent bit.

  • It can achieve higher throughput and will consume less CPU and memory. The [new cloudwatch plugin].
  • It also provides features to send metrics to cloudwatch with cloudwatch metric namespaces and dimesions.
  • Has better credential management with sts_endpoint.

Description of changes

  • Added cloudwatch_logs plugin parameters
  • Updated fluent-bit image to 2.31.2
  • Added support for all new cloudwatch_logs features.
  • Updated readme to reflect the change.

Checklist

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

Testing

  • added as part of argocd addon repository deployed with eks terraform blueprint. Here's the addon repo

  • configfile is correctly reflecting as per provided config

image

  • Helm chart deployed successfully

Chart version: 0.1.24
image

All pods up:
image

  • Testing template feature:
    Added below config to generate a new loggroup per namespace and new stream per app
cloudWatch:
    enable: true
    logGroupName: /eks/newchart/workload-logs/fallback
    logStreamPrefix: fallback-
    logGroupTemplate: /eks/newchart/workload-logs/$kubernetes['namespace_name']
    logStreamTemplate: $kubernetes['pod_name'].$kubernetes['container_name']

Namespace isolated logs being generated:
image

Application stream:
image

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

Copy link

@dnaprawa-capgemini dnaprawa-capgemini left a comment

Choose a reason for hiding this comment

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

I have tested the changes and they works good.

wesmiles
wesmiles previously approved these changes Feb 20, 2023
@PettitWesley
Copy link
Contributor

@deepend-dev Thanks! I've been meaning to do this myself for a while. S3 also needs to be added, possibly opensearch. I will review soonish.

I'm wondering though if the old plugin should still be an option though. It does have one advantage that is can be more stable. We've recently had several bad issues that impact only the high performance core C plugin, and giving customers an easy option to downgrade to older stable go plugin is important: aws/aws-for-fluent-bit#542

I could see two ways of doing this:

  1. make the plugin name configurable in the chart
  2. have 2 entries for cloudwatch, one for go, one for C

I think I prefer #2.

What do you think?

@deepend-dev
Copy link
Contributor Author

I would prefer option 2 if we really want to keep both versions.
I will update PR with changes for all s3, opensearch and both cloudwatch plugins today.

@PettitWesley
Copy link
Contributor

@deepend-dev thanks!

README.md Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
@deepend-dev
Copy link
Contributor Author

Testing S3 config

image

Getting S3 logs

image

@deepend-dev
Copy link
Contributor Author

deepend-dev commented Feb 25, 2023

Testing Opensearch:

Indexes are created
image

here's sample data
image

Issue

doesn't impact our implementation or this chart as its an issue with fluent bit itself

I noticed an issue that opensearch plugin doesnt work with record accessor, although it is mentioned in fluent-bit docs that it should.

I tried multiple combinations but it just doesnt seem to work. Raised an issue on fluentbit repo - issue . Have you seen this behaviour before?

Here's a setting example

[OUTPUT]
        Name                opensearch
        Match               *
        AWS_Region          ap-southeast-1
        AWS_Auth            On
        Host                search-fluent-bit-logging-g6lh44s35e7ytj5vh2yrsds7qa.ap-southeast-1.es.amazonaws.com
        Port                443
        tls                true
        Buffer_Size         5m
        Index               $kubernetes['namespace_name']
        Type                _doc
        Logstash_Format     Off
        Logstash_Prefix     logstash
        Logstash_DateFormat %Y.%m.%d
        Time_Key            @timestamp
        Time_Key_Format     %Y-%m-%dT%H:%M:%S
        Time_Key_Nanos      Off
        Include_Tag_Key     Off
        Tag_Key             _flb-key
        Generate_ID         Off
        Write_Operation     create
        Replace_Dots        Off
        Trace_Output        On
        Trace_Error         Off
        Current_Time_Index  Off
        Suppress_Type_Name  On

Here's fluent bit log showing that index api call is not correct. Its literally sending in $kubernetes['namespace_name'] as string without resolving :

image

Update 1

Solution

I have found solution to the above problem and it is maximum weird.

Above issue comes with all fluent bit ECR images aws-observability/aws-for-fluent-bit. I have tried withlatest, 2.31.3, 2.31.2, 2.30.0, 2.21.0, 2.22.0
but it works with docker hub image fluent/fluent-bit.

How do you want to proceed with this issue @PettitWesley

image

FYI I have raised below issues:
fluent bit : fluent/fluent-bit#6914
aws-for-fluent-bit: aws/aws-for-fluent-bit#557

Update 2

As per the notes on the issue, it is happening because latest public ecr image 2.31.3 is created with fluentbit 1.9.10 while the record accessor feature is introduced with fluent-bit 2.0.5. Current docker hub image for fluentbit is at 2.0.9 hence it works with that image.
Having said that we now know that the issue will resolve with later ecr image releases which will have base fluent bit image > 2.0.5

Copy link
Contributor

@matthewfala matthewfala left a comment

Choose a reason for hiding this comment

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

Looks mostly good! Thank you for your work on this!

stable/aws-for-fluent-bit/values.yaml Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/values.yaml Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/values.yaml Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
@deepend-dev
Copy link
Contributor Author

updated all reviews and tested changes to be working as expected

@deepend-dev deepend-dev changed the title feat: outdated aws-for-fluent-bit chart updated to use new high performance cloudwatch_logs plugin (feat)! : outdated aws-for-fluent-bit chart updated to use new high performance cloudwatch_logs plugin Mar 14, 2023
@deepend-dev deepend-dev requested review from PettitWesley and matthewfala and removed request for PettitWesley March 14, 2023 17:31
stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
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.

@deepend-dev I think this is almost ready to go, just 2 new comments, and then can you please sqash your changes into 1 or a few commits please?

stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
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.

Sorry.. we are almost there but found a minor issue in the readme formatting. Please click the "..." button and click "view file" in github to review the readme formatting yourself.

stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
stable/aws-for-fluent-bit/README.md Outdated Show resolved Hide resolved
@deepend-dev
Copy link
Contributor Author

Sorry.. we are almost there but found a minor issue in the readme formatting. Please click the "..." button and click "view file" in github to review the readme formatting yourself.

@PettitWesley formatting is looking good to me.
image

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.

5 participants