-
Notifications
You must be signed in to change notification settings - Fork 443
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
checkpoint_harmony_endpoint: improve handling of 404 and 503 errors #13009
Merged
chemamartinez
merged 5 commits into
elastic:main
from
chemamartinez:checkpoint_harmony_endpoint-cel_fixes
Mar 10, 2025
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
91aa4bf
Apply CEL improvements in forensics to all data streams
chemamartinez 3583633
Improve handling of 404 and 503 API errors
chemamartinez f35e509
Bump version
chemamartinez 6dd0a18
Update changelog
chemamartinez a614735
Log error when receiving 503 responses
chemamartinez File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
want_more
is true, a dummy event is required to make it immediately re-run. If there are no events it will wait for an interval.Here,
want_more
is false, so that dummy event is unnecessary.I think it should either be an empty array:
or a single event with an error message (single events get logged):
or the same with the polling message, so the event will be logged, but the pipeline will drop it:
The last one would be easier if the pipeline expected parsed JSON instead of
{ "message": "JSON string" }
, because the CEL input logging code only logs a single message if it's parsed JSON.I don't have a strong opinion about which one. The later ones are more messy, but get some useful information in the log/ES. The first one is clean and 404s shouldn't be happening a lot, and if they are they can always be seen with request tracing.
Here's a relevant part of the CEL documentation:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are talking about the 503 case instead of the 404 (that's the one that sets
want_more
to false).Thanks for the detailed explanation, I would choose to log the error, as receiving a 503 is not very common and so we can discover it more easily. My question is if we are going to log the error, why not use the same format as for the rest of the errors? I have submitted a commit with the changes, would you mind taking a look at it again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right it was 503. The change looks good. I think that kind of logged error event could be used for all error cases. Doesn't necessarily have to be now.
For these error cases there are two choices:
So for now it's logged and not dropped, which is fine. I think in the future we could have some conventions for those choices.