[Cloud Security] Fixed failing FTR

[kfirpeled]

Summary

fixes: #198632

FTR failed due to the usage of relative start and end when querying for the graph.

Together with @tinnytintin10 we decided it is safe to assume the time to query would be 30 minutes before and after the alert.

Some enhancements and fixes:

• Disabled re-fetch /graph API when window focus returned

Checklist

Delete any items that are not applicable to this PR.

• Unit or functional tests were updated or added to match the most common scenarios
• Flaky Test Runner was used on any tests changed

[albertoblaz]

What happens if timestamp is null? In lines 37-38, start and end will be set to null.

Should we default to something like i.e. 'now-30m/m' and 'now+30m/m' or is it safe to leave them as null?

[kfirpeled]

Great question. This component is not rendered if the timestamp is missing. The same is true when eventIds is undefined.

These checks happen on the parent component. This design is not ideal. I hope to find a better flow and refactor it. I'm also open to ideas.

and I'll run a jest UT to check what happens when timestamp is null; i'm curious myself

[kfirpeled]

@albertoblaz to answer your question, when timestamp is null, moment(null) returns null. Which will cause an error.

I fixed that with a default value of undefined

[PhilippeOberti]

should a new unit test be added to check that?

[opauloh]

Having to think about the side effect of moment(undefined) leading to the current Timestamp, seems to add to me some cognitive load that I would like to avoid it possible, i.e instead of falling back to undefined we can explicitly fallback to the current date object.

Also, I liked @albertoblaz suggestion of using Elasticsearch relatives start/end, so I guess we could combine both suggestions in this way for ex:

  const timestamp = getField(getFieldsData('@timestamp')) ?? new Date().toISOString();

  const { isLoading, isError, data } = useFetchGraphData({
    req: {
      query: {
        eventIds,
        start: `${timestamp}||-30m`,
        end: `${timestamp}||+30m`,
      },
    },
    options: {
      refetchOnWindowFocus: false,
    },
  });

This way we can even remove moment as well

[opauloh]

Also as @PhilippeOberti pointed out, some unit tests to check how useFetchGraphData is called based on the getFieldsData would be important

[kfirpeled]

Thanks @opauloh , @PhilippeOberti !
I enhanced the assertions of the current UT and changed the logic to use date math
I believe it is more robust now

[PhilippeOberti]

approving as the code LGTM, but I feel like unit tests should be improved

[PhilippeOberti]

I know this wasn't introduced by this PR specifically, but I just realized that these test only check the isAuditLog values, instead of checking everything that the useGraphPreview returns (timestamp, eventIds, actorIds, action and isAuditLog).
I feel like it would be better to verify all the values.

[PhilippeOberti]

as an example, you made a change to the hook by adding a new timestamp property returned, but none of these test verify that this property exists or what its value should be...

[kfirpeled]

good point @PhilippeOberti , I added assertion for the other values

[opauloh]

LGTM!

[kibanamachine]

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11809024912

[elasticmachine]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 958dc01

Failed CI Steps

• x-pack/test/alerting_api_integration/security_and_spaces/group1/config.ts

Test Failures

• [job] [logs] x-pack/test/alerting_api_integration/security_and_spaces/group1/config.ts / alerting api integration security and spaces enabled Alerts - Group 1 alerts backfill rule runs schedule backfill space_1_all_alerts_none_actions at space1 should handle scheduling multiple backfill job requests for a single rule appropriately

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
securitySolution	13.4MB	13.4MB	+304.0B

History

• 💔 Build #250110 failed 72fa6cf
• 💔 Build #250070 failed ab4953b

[kibanamachine]

💔 All backports failed

Status	Branch	Result
❌	8.x	Backport failed because of merge conflicts You might need to backport the following PRs to 8.x: - [Cloud Security] Added popover support for graph component (#199053)

Manual backport

To create the backport manually run:

node scripts/backport --pr 199683

Questions ?

Please refer to the Backport tool documentation

[kibanamachine]

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11809024912

[kibanamachine]

💔 All backports failed

Status	Branch	Result
❌	8.x	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 199683

Questions ?

Please refer to the Backport tool documentation

[kibanamachine]

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 199683 locally

[kibanamachine]

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 199683 locally

[kibanamachine]

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 199683 locally

[kfirpeled]

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation