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

Adding functionality to death letter index instead of dropping events. #26952

Merged
merged 17 commits into from
Aug 2, 2021

Conversation

mjmbischoff
Copy link
Contributor

@mjmbischoff mjmbischoff commented Jul 19, 2021

What does this PR do?

Allows the user to specify the policy (/action) of how to deal with non-indexable events. We default to drop dropping the event which is current behavior. Alternatively one can specify death_letter_index to index into the specified index name

output.elasticsearch:
    non_indexable_policy.dead_letter_index:
        index: "dead-letter-index"
output.elasticsearch:
    non_indexable_policy.drop: ~

Why is it important?

Currently when there's a mapping conflict events are dropped, this is undesirable for a lot of customers. While filebeat technically doesn't violate it's guarantee that events are delivered at least once, it does cause the event to be lost. Which is both undesirable and unexpected behavior for most customers.

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.

How to test this PR locally

See integration test,
Configure filebeat with the ES ouput and specifying the action and index
Then creating a mapping conflict:

{ "bar": 0 }
{ "bar": "bar0" }

Related issues

Use cases

When a mapping conflict occurs the message is kept and indexed into an alternative index. This can then later be re-ingested after either the data or the index (the alias points to) is changed.

Other remarks

  • The index currently must reside on the same ES cluster. Changing this, if desired, would be a separate PR.
  • The format we use to store the events on the death letter index is message fields (containing the original message with escaped json), error.type (http code) and error.message (description)

Currently when theres a mapping conflict events are dropped, this commits allows one attempting to index it into a different index instead so it can be re-ingested later vs dropping or stalling ingest. The new code tries to death letter index for a wider rage including read only block(403) which is likely to fail for the death letter index as well. The index currently most reside on the same ES cluster. Changing this, if desired, would be a separate PR. Similair to the format we use to store on the death letter index (original message is ecapsed json in the message field.
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jul 19, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jul 19, 2021

💚 Build Succeeded

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

Expand to view the summary

Build stats

  • Start Time: 2021-08-02T15:42:58.813+0000

  • Duration: 134 min 48 sec

  • Commit: 0304845

Test stats 🧪

Test Results
Failed 0
Passed 49421
Skipped 5318
Total 54739

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 49421
Skipped 5318
Total 54739

@mjmbischoff mjmbischoff requested a review from kvch July 23, 2021 09:22
@ChrsMark ChrsMark added the Team:Elastic-Agent Label for the Agent team label Jul 26, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/agent (Team:Agent)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jul 26, 2021
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.

All in all, great PR with good tests. I have a few nits that are not required to be addressed if you do not want to. However, the configuration should be reworked a bit to be more user friendly.

mjmbischoff and others added 4 commits July 28, 2021 10:09
@mjmbischoff
Copy link
Contributor Author

dead letter vs death letter 😂

Made the changes to the config setuo, also reduced some more parameters by making additional functions a function of client
Added tests for the config.
If our CI is happy tomorrow, I'll have a look at the docs.

@mjmbischoff mjmbischoff requested a review from kvch July 30, 2021 08:10
@mergify
Copy link
Contributor

mergify bot commented Jul 30, 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 deathletter-index upstream/deathletter-index
git merge upstream/master
git push upstream deathletter-index

@mjmbischoff
Copy link
Contributor Author

@kvch think it's all good now

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.

Please update the PR description with the new configuration format. Also, as it is a new feature, please make it beta with cfgwarn.Beta and beta[] in the documentation.

Otherwise, the PR LGTM.

@kvch kvch added the backport-v7.15.0 Automated backport with mergify label Aug 2, 2021
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.

Awesome \o/

@kvch
Copy link
Contributor

kvch commented Aug 2, 2021

Once you fix the lint errors, you can merge it. Thanks for the PR!

@mjmbischoff mjmbischoff merged commit 60cf09d into elastic:master Aug 2, 2021
@mjmbischoff mjmbischoff deleted the deathletter-index branch August 2, 2021 18:20
mergify bot pushed a commit that referenced this pull request Aug 2, 2021
Adding functionality to death letter index instead of dropping events.

When there's a mapping conflict events are dropped, this commits allows one attempting to index it into a different index instead so it can be re-ingested later vs dropping or stalling ingest. The index currently must reside on the same ES cluster.

* buildCollectPublishFails and other functions changed to be a function of Client so not to have to pass arguments

Co-authored-by: Noémi Ványi <[email protected]>
(cherry picked from commit 60cf09d)
kvch pushed a commit that referenced this pull request Aug 3, 2021
Adding functionality to death letter index instead of dropping events.

When there's a mapping conflict events are dropped, this commits allows one attempting to index it into a different index instead so it can be re-ingested later vs dropping or stalling ingest. The index currently must reside on the same ES cluster.

* buildCollectPublishFails and other functions changed to be a function of Client so not to have to pass arguments

Co-authored-by: Noémi Ványi <[email protected]>
(cherry picked from commit 60cf09d)

Co-authored-by: Michael Bischoff <[email protected]>
@philippkahr philippkahr changed the title Adding functionality to death letter index instead of dropping events. Adding functionality to dead letter index instead of dropping events. Sep 9, 2021
@philippkahr philippkahr changed the title Adding functionality to dead letter index instead of dropping events. Adding functionality to death letter index instead of dropping events. Sep 9, 2021
mergify bot pushed a commit that referenced this pull request Sep 9, 2021
Adding functionality to death letter index instead of dropping events.

When there's a mapping conflict events are dropped, this commits allows one attempting to index it into a different index instead so it can be re-ingested later vs dropping or stalling ingest. The index currently must reside on the same ES cluster.

* buildCollectPublishFails and other functions changed to be a function of Client so not to have to pass arguments

Co-authored-by: Noémi Ványi <[email protected]>
(cherry picked from commit 60cf09d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v7.15.0 Automated backport with mergify enhancement Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants