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 json body check for sqs message #21727

Merged
merged 8 commits into from
Dec 4, 2020
Merged

Add json body check for sqs message #21727

merged 8 commits into from
Dec 4, 2020

Conversation

kaiyan-sheng
Copy link
Contributor

@kaiyan-sheng kaiyan-sheng commented Oct 12, 2020

What does this PR do?

I suspect sometimes AWS SQS message is not an invalid JSON format. This PR is to add check on it and improving error message for better debugging.

For example:

2020-08-26T10:05:10.780Z        ERROR   [s3]    s3/input.go:257 handleSQSMessage failed: json unmarshal sqs message body failed: invalid character 'e' in literal true (expecting 'r')

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.

Related issues

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 12, 2020
@kaiyan-sheng kaiyan-sheng self-assigned this Oct 12, 2020
@kaiyan-sheng kaiyan-sheng added the Team:Platforms Label for the Integrations - Platforms team label Oct 12, 2020
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 12, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Oct 12, 2020

💚 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

Expand to view the summary

Build stats

  • Build Cause: Pull request #21727 updated

  • Start Time: 2020-12-03T16:43:24.768+0000

  • Duration: 43 min 56 sec

Test stats 🧪

Test Results
Failed 0
Passed 1979
Skipped 259
Total 2238

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 1979
Skipped 259
Total 2238

@kaiyan-sheng kaiyan-sheng marked this pull request as ready for review October 19, 2020 18:19
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-platforms (Team:Platforms)

@kaiyan-sheng kaiyan-sheng added needs_backport PR is waiting to be backported to other branches. review labels Oct 19, 2020
@kaiyan-sheng
Copy link
Contributor Author

@exekias I have a question about this: When json body check json.Valid failed, should this SQS message be deleted from the queue? Right now with this PR, it will not be removed. This means the visibility timeout will be updated and the message will eventually go back into the queue and get processed again later. But if the json body is invalid, it will not pass json.Valid check next time around either.

@exekias
Copy link
Contributor

exekias commented Oct 29, 2020

Sorry I miss this notification 🙏 , two things:

  • Calling Valid here is the same as handling the Unmarshall error, but Valid will add more processing, you are basically reading the message twice. It would be better just to do the logging when the error happens.
  • An invalid JSON message is permanent to us, so I think it would make sense to remove it from the queue, is there any way to retrieve it later after removing it? I guess the answer is no

@kaiyan-sheng
Copy link
Contributor Author

@exekias Thank you for the input!!

  • Calling Valid here is the same as handling the Unmarshall error, but Valid will add more processing, you are basically reading the message twice. It would be better just to do the logging when the error happens.

Good point, will remove the extra Valid here.

  • An invalid JSON message is permanent to us, so I think it would make sense to remove it from the queue, is there any way to retrieve it later after removing it? I guess the answer is no

Yeah there is no way to retrieve it later after removing.

@elasticmachine
Copy link
Collaborator

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 1947
Skipped 259
Total 2206

@kaiyan-sheng kaiyan-sheng merged commit 86fc865 into elastic:master Dec 4, 2020
@kaiyan-sheng kaiyan-sheng deleted the s3_unmarshal_error branch December 4, 2020 22:36
@kaiyan-sheng kaiyan-sheng added v7.11.0 and removed needs_backport PR is waiting to be backported to other branches. labels Dec 5, 2020
kaiyan-sheng added a commit that referenced this pull request Dec 7, 2020
* Add json body check for sqs message (#21727)


(cherry picked from commit 86fc865)

* update changelog
kaiyan-sheng added a commit that referenced this pull request Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Platforms Label for the Integrations - Platforms team v7.10.1 v7.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants