Skip to content

Flashpoint ignite - is_time_sensitive() related changes #38862

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

Merged

Conversation

ShacharKidor
Copy link
Contributor

@ShacharKidor ShacharKidor commented Mar 3, 2025

Contributing to Cortex XSOAR Content

Make sure to register your contribution by filling the contribution registration form

The Pull Request will be reviewed only after the contribution registration form is filled.

Status

  • In Progress
  • Ready
  • In Hold - (Reason for hold)

Related Issues

relates: CIAC-12908

Description

  • Improved implementation of all the integration commands where the number of retries was lowered to 0 and the request timeout was lowered to 15 seconds when a command is called during an enrichment process.

Must have

  • Tests
  • Documentation

Copy link

github-actions bot commented Mar 3, 2025

Your contributed Flashpoint pack has been modified on files:

Packs/Flashpoint/pack_metadata.json
Packs/Flashpoint/ReleaseNotes/2_0_5.md
Packs/Flashpoint/Integrations/Ignite/Ignite.py
Please review the changes here

@CLAassistant
Copy link

CLAassistant commented Mar 4, 2025

CLA assistant check
All committers have signed the CLA.

@ShacharKidor ShacharKidor marked this pull request as ready for review March 5, 2025 11:40
@content-bot
Copy link
Collaborator

The following errors were thrown as a part of this pr: ST111.
The following errors cannot be ignored: ST111.
The following errors don't run as part of the nightly flow and therefore can be force merged: ST111.
##############################################################
Please note that the PR can be force merged from the validation perspective.
##############################################################

@content-bot
Copy link
Collaborator

This PR is marked as 'Stale' because it has been open for 30 days with no activity, it will be automatically closed in 15 days if no activity will be done. To reset the counter just remove the 'Stale' label or make changes to update this PR. If you wish this PR will never be marked as 'Stale' add the 'Ignore Stale'

@content-bot
Copy link
Collaborator

This PR is marked as 'Stale' because it has been open for 30 days with no activity, it will be automatically closed in 15 days if no activity will be done. To reset the counter just remove the 'Stale' label or make changes to update this PR. If you wish this PR will never be marked as 'Stale' add the 'Ignore Stale'

@LHousehold
Copy link
Contributor

Hello, this is Luke from Flashpoint. We've tested this change and it functions correctly and as expected. Thank you for your patience as we've been working through this.

On the technical side we have been discussing and we do have a concern that even a small network hiccup would cause the enrichment command to fail and harm the experience for customers. We'd like to suggest changing the 10 seconds to 15, and changing the 0 retries to 2 (the first retry happens instantly). Considering the feedback from yourselves and the customer, we agree that the current retry method is too heavy for enrichment purposes, but having no retries feels like a risk as well. If your team is okay with 15s and 2 retries, we can agree to this change immediately.

Additionally when we make these changes, we can also observe by how much this particular customers' timing is improved and discuss if we need to further reduce the timing or restrategize.

What are your thoughts?

@ShacharKidor
Copy link
Contributor Author

ShacharKidor commented May 19, 2025

Hi @LHousehold ,

Thanks for the update and for validating the fix.

I’ll increase the timeout to 15 seconds as suggested. However, regarding the retry count, we believe it's best to keep it at 0. During enrichment, minimizing the number of calls is critical to ensure optimal performance—additional retries could significantly degrade the user experience.

Let me know your thoughts.

Best regards,
Shachar

@ShacharKidor ShacharKidor added the ready-for-pipeline-running Whether the pr is ready for running the whole pipeline, including testing on SAAS machines label May 19, 2025
Copy link

github-actions bot commented May 19, 2025

Coverage

Coverage Report
FileStmtsMissCoverMissing
Packs/Flashpoint/Integrations/Ignite
   Ignite.py10304395%227, 292, 310–316, 392, 556, 559, 651, 846, 1084–1085, 1126, 1285, 1367, 1669, 1907–1912, 1914, 2084, 2090–2091, 2099, 2137–2138, 2443–2446, 2456, 2468, 2473–2474, 2477, 2481
TOTAL10304395% 

Tests Skipped Failures Errors Time
91 0 💤 0 ❌ 0 🔥 4.217s ⏱️

@content-bot
Copy link
Collaborator

Validate summary
The following errors were thrown as a part of this pr: RN106, ST111, PA114, DO106.
The following errors cannot be ignored: RN106, ST111, PA114, DO106.
The following errors don't run as part of the nightly flow and therefore can be force merged: RN106, ST111, PA114, DO106.

Verdict: PR can be force merged from validate perspective? ✅

@ShacharKidor ShacharKidor added docs-approved ForceMerge Forcing the merge of the PR despite the build status labels May 21, 2025
@ShacharKidor
Copy link
Contributor Author

@yuvalbenshalom - Please force merge the PR.
The [ST111] validation is failing for missing section headers, I didn't add the since this is a partner pack.

@yuvalbenshalom yuvalbenshalom merged commit 21cb620 into master May 21, 2025
22 of 25 checks passed
@yuvalbenshalom yuvalbenshalom deleted the Flashpoint_ignite_is_time_sensitive_related_changes branch May 21, 2025 10:26
oatias pushed a commit that referenced this pull request May 22, 2025
* lowered retries and timeout when is_time_sensitive is True

* Added RN

* fixed typo

* Updated the docker image

* Added a comment

* Increase timeout on enrichment to 15 sec

* Added RN

* revert change in old RN file

* Updated the docker image tag

* pre commit fixes

* ruff fixes
TOUFIKIzakarya pushed a commit to TOUFIKIzakarya/content that referenced this pull request May 22, 2025
* lowered retries and timeout when is_time_sensitive is True

* Added RN

* fixed typo

* Updated the docker image

* Added a comment

* Increase timeout on enrichment to 15 sec

* Added RN

* revert change in old RN file

* Updated the docker image tag

* pre commit fixes

* ruff fixes
Pinger77 pushed a commit to Pinger77/content that referenced this pull request Jun 12, 2025
* lowered retries and timeout when is_time_sensitive is True

* Added RN

* fixed typo

* Updated the docker image

* Added a comment

* Increase timeout on enrichment to 15 sec

* Added RN

* revert change in old RN file

* Updated the docker image tag

* pre commit fixes

* ruff fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-approved ForceMerge Forcing the merge of the PR despite the build status ready-for-pipeline-running Whether the pr is ready for running the whole pipeline, including testing on SAAS machines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants