Skip to content

Conversation

@chemamartinez
Copy link
Contributor

Proposed commit message

Three changes have been made to improve handling of pulled data: - Base subsequent request timeranges 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, gratefully ending, clearing task ID and restarting the sequence for the same timeframe. Ref: - [1] https://app.swaggerhub.com/apis-docs/Check-Point/infinity-events-api/1.0.0#/Search%20Event%20Logs/getTaskStatus 

Note

Each change is delivered in a different commit for better reviewing.

Note

All data streams have identical cel.yml.hbs files.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.
  • I have verified that any added dashboard complies with Kibana's Dashboard good practices
@chemamartinez chemamartinez added enhancement New feature or request Team:Security-Service Integrations Security Service Integrations team [elastic/security-service-integrations] Integration:checkpoint_harmony_endpoint Check Point Harmony Endpoint labels Apr 8, 2025
@chemamartinez chemamartinez self-assigned this Apr 8, 2025
@chemamartinez chemamartinez marked this pull request as ready for review April 8, 2025 17:12
@chemamartinez chemamartinez requested a review from a team as a code owner April 8, 2025 17:12
@elasticmachine
Copy link

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

@elastic-vault-github-plugin-prod

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@elasticmachine
Copy link

💚 Build Succeeded

cc @chemamartinez

Copy link
Contributor

@efd6 efd6 left a 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": [],
Copy link
Contributor

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.

@chemamartinez chemamartinez merged commit efc75e7 into elastic:main Apr 9, 2025
7 checks passed
@chemamartinez chemamartinez deleted the checkpoint_harmony_endpoint-cel-flow-control-fixes branch April 9, 2025 06:29
@elastic-vault-github-plugin-prod

Package checkpoint_harmony_endpoint - 0.7.0 containing this change is available at https://epr.elastic.co/package/checkpoint_harmony_endpoint/0.7.0/

Copy link
Contributor

@chrisberkhout chrisberkhout left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request Integration:checkpoint_harmony_endpoint Check Point Harmony Endpoint Team:Security-Service Integrations Security Service Integrations team [elastic/security-service-integrations]

4 participants