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

add support for kinesis record deaggregation in functionbeat #28241

Merged
merged 6 commits into from
Oct 27, 2021

Conversation

jpaskhay
Copy link
Contributor

@jpaskhay jpaskhay commented Oct 4, 2021

What does this PR do?

This PR adds support for properly deaggregating Kinesis records which are in the aggregated record format.

Why is it important?

This change allows FunctionBeat to properly consume records being sent to Kinesis using the aggregated record format. Users often do this for throughput and cost optimization purposes. The current implementation will instead show single records that have the embedded Protobuf serialized format.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
    - [ ] I have made corresponding changes to the documentation
    - [ ] I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

Notes:

  • Had to add AWS SDK v1 due to current kinesis-aggregation support. There is an open PR that adds v2 support. Whenever it is reviewed and merged, there can be a followup change to remove the v1 dependency.
  • Performed end to end test to verify functionality using a local functionbeat build and test AWS account. Thanks to @ravinaik1312 for his help on testing this!

How to test this PR locally

  • Build local version of functionbeat
  • Deploy to AWS Lambda function
  • Send aggregated records to Kinesis stream (e.g. using KPL, fluentd, fluent-bit, etc. with the aggregated records option enabled)
  • Verify records are properly deaggregated in Elastic

Related issues

Use cases

Screenshots

Logs

- tests are heavily based on record generation in https://github.com/awslabs/kinesis-aggregation/blob/master/go/deaggregator/deaggregator_test.go
- had to add AWS SDK v1 due to current kinesis-aggregation support (there is an open PR that adds v2 support)
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 4, 2021
@mergify
Copy link
Contributor

mergify bot commented Oct 4, 2021

This pull request does not have a backport label. Could you fix it @jpaskhay? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Oct 4, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Oct 4, 2021

❕ Build Aborted

The PR is not allowed to run in the CI yet

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Reason: The PR is not allowed to run in the CI yet

  • Start Time: 2021-10-25T15:47:31.149+0000

  • Duration: 10 min 42 sec

  • Commit: 11db8de

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

@mergify
Copy link
Contributor

mergify bot commented Oct 7, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b functionbeat-kinesis-deaggregation upstream/functionbeat-kinesis-deaggregation
git merge upstream/master
git push upstream functionbeat-kinesis-deaggregation

@bvader bvader added Functionbeat and removed needs_team Indicates that the issue/PR needs a Team:* label labels Oct 13, 2021
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 13, 2021
@bvader bvader requested a review from jsoriano October 13, 2021 17:47
@jsoriano jsoriano removed their request for review October 14, 2021 08:01
@andresrc andresrc added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Oct 14, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 14, 2021
@kvch kvch self-assigned this Oct 14, 2021
@@ -31,8 +31,10 @@ require (
github.com/apoydence/eachers v0.0.0-20181020210610-23942921fe77 // indirect
github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5
github.com/aws/aws-lambda-go v1.13.3
github.com/aws/aws-sdk-go v1.19.48
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason why you need this version? In beats we use github.com/aws/aws-sdk-go-v2. I would rather not add a new dependency for the same service, if it is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I actually called that out in the Author's Checklist section (felt like the most appropriate place for the note). Unfortunately, the AWS deaggregation library only supports v1 of the SDK. There is an open PR that adds support for v2, but we're still waiting on the repo maintainer to review and merge it.

Copy link
Contributor

@kvch kvch left a comment

Choose a reason for hiding this comment

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

Could you please cleanup the depencency? Otherwise great feature and the code impeccable!

@kvch
Copy link
Contributor

kvch commented Oct 19, 2021

jenkins run tests

@jpaskhay
Copy link
Contributor Author

Could you please cleanup the depencency? Otherwise great feature and the code impeccable!

Thank you! 😄 Replied inline to your dependency comment.

I'll try to resolve the license linting build failure in the meantime.

- the go mod is embedded in the repo since it is a monorepo, so couldn't specify the licenseFile. linked to the license file in the url field instead ("make notice" succeeded locally)
@mergify
Copy link
Contributor

mergify bot commented Oct 20, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b functionbeat-kinesis-deaggregation upstream/functionbeat-kinesis-deaggregation
git merge upstream/master
git push upstream functionbeat-kinesis-deaggregation

@kvch
Copy link
Contributor

kvch commented Oct 25, 2021

jenkins run tests

@kvch
Copy link
Contributor

kvch commented Oct 25, 2021

Could you please run make notice and push the updated NOTICE.txt file?

@jpaskhay
Copy link
Contributor Author

Could you please run make notice and push the updated NOTICE.txt file?

Done. Sorry, I should've realized I needed to push that after testing the overrides locally.

@kvch
Copy link
Contributor

kvch commented Oct 27, 2021

jenkins run tests

@kvch kvch merged commit 5ac77d3 into elastic:master Oct 27, 2021
@kvch
Copy link
Contributor

kvch commented Oct 27, 2021

Thank you for your contribution!

@elasticmachine
Copy link
Collaborator

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

@kvch kvch added the backport-v7.16.0 Automated backport with mergify label Oct 27, 2021
@mergify mergify bot removed the backport-skip Skip notification from the automated backport with mergify label Oct 27, 2021
mergify bot pushed a commit that referenced this pull request Oct 27, 2021
@jpaskhay jpaskhay deleted the functionbeat-kinesis-deaggregation branch October 27, 2021 16:44
kvch pushed a commit that referenced this pull request Oct 28, 2021
…#28681)

(cherry picked from commit 5ac77d3)

Co-authored-by: Joey Paskhay <joey.paskhay@gmail.com>
Icedroid pushed a commit to Icedroid/beats that referenced this pull request Nov 1, 2021
@ruflin ruflin added the backport-v8.0.0 Automated backport with mergify label Dec 8, 2021
kvch pushed a commit that referenced this pull request Dec 8, 2021
kvch pushed a commit that referenced this pull request Dec 8, 2021
…#29340)

(cherry picked from commit 5ac77d3)

Co-authored-by: Joey Paskhay <joey.paskhay@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v7.16.0 Automated backport with mergify backport-v8.0.0 Automated backport with mergify Functionbeat Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Functionbeat] Add support for AWS Kinesis record deaggregation
6 participants