-
Notifications
You must be signed in to change notification settings - Fork 522
checkpoint_harmony_endpoint: fixes in CEL to control data flow #13474
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: fixes in CEL to control data flow #13474
Conversation
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
🚀 Benchmarks reportTo see the full report comment with |
|
💚 Build Succeeded
|
efd6
left a comment
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.
LGTM
Please wrap the commit message body text (also some typos fixed):
Three changes have been made to improve handling of pulled data:
- Base subsequent request time ranges on the timestamp of the latest
data received instead of the last requested range, to avoid losing
events that could not be pulled in the previous iteration.
- Adjusted default request limits (results per search, and results per
page) to avoid an overload on the API server that could lead to a
timeout.
- When receiving a Canceled state[1] for the TaskStatusResponse,
gracefully ending, clearing task ID and restarting the sequence for
the same time frame.
Ref:
- [1] https://app.swaggerhub.com/apis-docs/Check-Point/infinity-events-api/1.0.0#/Search%20Event%20Logs/getTaskStatus
The appearance of setting cursor fields to null to indicate absence is troubling, but that has existed since the package was originally written. It would be nice to fix it, but it can (should) happen in another PR.
| // 'Canceled' (Error or timeout) - Clear the task ID and reset the sequence for the same timeframe. | ||
| state.with( | ||
| { | ||
| "events": [], |
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.
This is allowed to be null, i.e. "events": null,. I'm not sure that it's clearer either way and the perf delta would be ~0, so no need to change this, just noting.
|
Package checkpoint_harmony_endpoint - 0.7.0 containing this change is available at https://epr.elastic.co/package/checkpoint_harmony_endpoint/0.7.0/ |
chrisberkhout
left a comment
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.
Looks good.
If we find Cancelled happening often, then next thing to try would be to limit the maximum time range.




Proposed commit message
Note
Each change is delivered in a different commit for better reviewing.
Note
All data streams have identical
cel.yml.hbsfiles.Checklist
changelog.ymlfile.