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.

Sign up for GitHub

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

Jump to bottom

[Filebeat] Improve error message when handleSQSMessage failed #14113

Merged
merged 11 commits into from
Oct 25, 2019
Merged

[Filebeat] Improve error message when handleSQSMessage failed #14113

merged 11 commits into from
Oct 25, 2019

Conversation

kaiyan-sheng
Copy link
Contributor

@kaiyan-sheng kaiyan-sheng commented Oct 17, 2019 •
edited
Loading

When a message in SQS is not created by s3 or event name doesn't start with ObjectCreated, this message will be removed and log a more clearer error message. Also added debug log when message is removed.

@kaiyan-sheng kaiyan-sheng self-assigned this Oct 17, 2019
@kaiyan-sheng
Copy link
Contributor Author

@exekias Sorry I messed up my previous PR so I'm responding to your question here instead. I don't think it's a good idea to keep reading the same non-valid message from the queue again and again every time there is a new message coming in. I added a messageBlackList in the code to keep tracking the message IDs of the non-valid messages. Once the message goes back to the queue, the message ID stays the same. So we can use the ID to keep track of the messages we want to avoid. WDYT?

kaiyan-sheng
kaiyan-sheng commented Oct 17, 2019
View reviewed changes
CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
@kaiyan-sheng kaiyan-sheng added Team:Integrations Label for the Integrations team bug Filebeat Filebeat needs_backport PR is waiting to be backported to other branches. v7.5.0 labels Oct 17, 2019
@kaiyan-sheng kaiyan-sheng mentioned this pull request Oct 17, 2019
Closed
@kaiyan-sheng
Copy link
Contributor Author

Working on fixing the failing test.

@exekias
Copy link
Contributor

exekias commented Oct 18, 2019

@exekias Sorry I messed up my previous PR so I'm responding to your question here instead. I don't think it's a good idea to keep reading the same non-valid message from the queue again and again every time there is a new message coming in. I added a messageBlackList in the code to keep tracking the message IDs of the non-valid messages. Once the message goes back to the queue, the message ID stays the same. So we can use the ID to keep track of the messages we want to avoid. WDYT?

That would make Filebeat memory grow without bounds, as the list of ignored items could keep growing forever. I wonder if we should still remove the message but show an error to the user explaining what happened? This looks like a bad configuration, we should not get anything but S3 notifications on these queues

@kaiyan-sheng
Copy link
Contributor Author

@exekias Good point! Definitely don't want filebeat memory to grow out of bound. I removed the blacklist option and improved the log message so it's clear what went wrong.

@kaiyan-sheng kaiyan-sheng changed the title [Filebeat] Avoid deleting message from SQS if handleSQSMessage failed [Filebeat] Improve error message when handleSQSMessage failed Oct 22, 2019
kaiyan-sheng
kaiyan-sheng commented Oct 22, 2019
View reviewed changes
x-pack/filebeat/input/s3/input.go
p.logger.Error(err)
errC <- err
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switching the order here so the error message get logged first and then send to error channel. Otherwise the error log never get published.

exekias
exekias suggested changes Oct 25, 2019
View reviewed changes
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

LGTM besides changelog issues

CHANGELOG.next.asciidoc Show resolved Hide resolved
@kaiyan-sheng kaiyan-sheng merged commit b089094 into elastic:master Oct 25, 2019
@kaiyan-sheng kaiyan-sheng deleted the fix_delete_msg branch October 25, 2019 15:51
@kaiyan-sheng kaiyan-sheng mentioned this pull request Oct 25, 2019
Merged
@kaiyan-sheng kaiyan-sheng removed the needs_backport PR is waiting to be backported to other branches. label Oct 25, 2019
kaiyan-sheng added a commit that referenced this pull request Oct 25, 2019
@kaiyan-sheng
Cherry-pick #14113 to 7.5: [Filebeat] Improve error message when hand…
ad11393 
…leSQSMessage failed (#14248)

* [Filebeat] Improve error message when handleSQSMessage failed (#14113)

* Add message id to debug log

(cherry picked from commit b089094)

* run mage fmt under x-pack/libbeat
jorgemarey pushed a commit to jorgemarey/beats that referenced this pull request Jun 8, 2020
@kaiyan-sheng @jorgemarey
[Filebeat] Improve error message when handleSQSMessage failed (elasti…
35e600d 
…c#14113)

* Add message id to debug log
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
@kaiyan-sheng
Cherry-pick elastic#14113 to 7.5: [Filebeat] Improve error message wh…
d6783ab 
…en handleSQSMessage failed (elastic#14248)

* [Filebeat] Improve error message when handleSQSMessage failed (elastic#14113)

* Add message id to debug log

(cherry picked from commit f6a19eb)

* run mage fmt under x-pack/libbeat
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@exekias exekias exekias approved these changes

Assignees

@kaiyan-sheng kaiyan-sheng

Labels
bug Filebeat Filebeat Team:Integrations Label for the Integrations team v7.5.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kaiyan-sheng @exekias @andresrc