[ResponseOps][Cases] Populate total alerts and comments in the cases saved objects by cnasikas · Pull Request #223992 · elastic/kibana

cnasikas · 2025-06-14T09:05:03Z

Summary

This is a farewell PR to Cases. Probably my last PR to the cases codebase. It was quite a journey, and I learned a lot. I hope the best for the feature of Cases.

Decisions

Just before Cases was forbidden to do migrations, we did a last migration to all cases to persist total_alerts: -1 and total_comments: -1. We did that so that in the future, when we would want to populate the fields, we would know which cases have their fields populated and which do not. In this PR, due to time constraints and criticality of the feature, I took the following decisions:

Cases return from their APIs the total comments and alerts of each case. They do that by doing an aggregation, getting the counts, and merging them with the response. I did not change that behavior. In following PRs, it can be optimized and fetch the stats only for cases that do not yet have their stats populated (cases with -1 in the counts)
When a case is created, the counts are zero.
When a comment or alert is added, I do an aggregation to get the stats (total alerts and comments) of the current case, and then update the counters with the number of the newly created attachments. The case is updated without version checks. In race conditions, where an attachment is being added before updating the case, the numbers could be off. This is a deliberate choice. It can be fixed later with retries and version concurrency control.
The case service will continue to not return the total_alerts and total_comments.
The case service will accept the total_alerts and total_comments attributes to be able to set them.

Fixes: #217636

cc @michaelolo24

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2025-06-15T12:52:06Z

Pinging @elastic/response-ops (Team:ResponseOps)

elasticmachine · 2025-06-15T12:52:07Z

Pinging @elastic/response-ops-cases (Feature:Cases)

adcoelho · 2025-06-16T09:30:40Z

x-pack/platform/plugins/shared/cases/server/services/cases/transform.ts

+    | Partial<CaseTransformedAttributes>
+    | Partial<CaseTransformedAttributesWithAttachmentStats>


nit: Isn't this enough?

Suggested change

| Partial<CaseTransformedAttributes>

| Partial<CaseTransformedAttributesWithAttachmentStats>

| Partial<CaseTransformedAttributesWithAttachmentStats>

x-pack/platform/plugins/shared/cases/server/common/types/case.test.ts

x-pack/platform/plugins/shared/cases/server/common/models/case_with_comments.test.ts

cnasikas · 2025-06-18T10:25:35Z

@adcoelho, I have addressed your feedback and added the functionality for deletion. I changed the logic slightly and no longer rely on counting the stats in memory. I rely only on what ES returns. I realized that it is not feasible to perform the counts correctly in memory on each Kibana node, and I preferred to retrieve the state from Elasticsearch (ES) consistently. This will not fix the issue I mentioned in the description, but it will make it more robust.

adcoelho · 2025-06-18T13:44:18Z

x-pack/platform/plugins/shared/cases/server/client/attachments/delete.ts

+      updated_at: date,
+      updated_by: user,


I noticed we include these two updated_* here, and wanted to check if maybe we were doing that twice.

We create the user action, but never change the updated fields. Weren't we updating them when deleting attachments?

We do not update these fields when creating user actions. Not sure if we should or not. We wanted to avoid version conflicts that will be caused by updating the case SO as much as possible. But now, with the new functionality, it is inevitable. We need to update the stats. Thought, that makes me think, deleting or creating a comment, is it considered an update to the case? Alternatively, we could only update the stats, but not the updated_* fields, as updating them may lead to wrong assumptions about who updated the case last and when. Wdyt?

adcoelho

👍

adcoelho · 2025-06-18T13:50:19Z

x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comment.ts

+
+        const theCase = res.body.hits.hits[0]._source?.cases!;
+
+        // We added 3 attachments, 2 user and 1 alert and we deleted 1 of each


nit: Since we call bulkCreateAttachments, we could do some checks before deletion and make this clear in the code instead of in a comment.

expect(theCase.total_alerts).to.eql(1); expect(theCase.total_comments).to.eql(2);

This is tested in x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts. I thought it was better to focus only on the functionality we are interested in. Wdyt?

cnasikas · 2025-06-21T15:02:10Z

@elasticmachine merge upstream

elasticmachine · 2025-06-21T15:02:13Z

merge conflict between base and head

elasticmachine · 2025-06-21T19:03:26Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 364e616

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #46 / ESQL execution logic API @ess @serverless ES|QL rule type esql query specific syntax mv_expand command pagination should create alerts from expanded values
[job] [logs] FTR Configs #100 / ESQL execution logic API @ess @serverless ES|QL rule type shard failures should handle shard failures and include warning in logs for query that is not aggregating

Metrics [docs]

✅ unchanged

History

💔 Build #311160 failed 588c0f9
💔 Build #309845 failed 18f4164
💚 Build #308576 succeeded 4958720
💚 Build #308534 succeeded 41bd76a
💔 Build #308529 failed 7adb826

cc @cnasikas

michaelolo24 · 2025-06-23T15:27:27Z

x-pack/platform/plugins/shared/cases/server/common/models/case_with_comments.ts

        }),
-        this.partialUpdateCaseUserAndDateSkipRefresh(new Date().toISOString()),
-      ]);
+        refresh: true,


Should we apply refresh: true or refresh: wait_for here given #158134

michaelolo24

Just added a single comment regarding the refresh setting, otherwise relying on @adcoelho as the primary reviewer here to not block this work

kibanamachine · 2025-06-23T16:56:40Z

Starting backport for target branches: 8.19

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

kibanamachine · 2025-06-23T17:02:46Z

💔 All backports failed

Status	Branch	Result
❌	8.19	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 223992

Questions ?

Please refer to the Backport tool documentation

cnasikas · 2025-06-23T17:10:46Z

💚 All backports created successfully

Status	Branch	Result
✅	8.19

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

Questions ?

Please refer to the Backport tool documentation

@michaelolo24

… cases saved objects (#223992) (#224928) # Backport This will backport the following commits from `main` to `8.19`: - [[ResponseOps][Cases] Populate total alerts and comments in the cases saved objects (#223992)](#223992)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

@michaelolo24

…saved objects (elastic#223992) ## Summary This is a farewell PR to Cases. Probably my last PR to the cases codebase. It was quite a journey, and I learned a lot. I hope the best for the feature of Cases. ## Decisions Just before Cases was forbidden to do migrations, we did a last migration to all cases to persist `total_alerts: -1` and `total_comments: -1`. We did that so that in the future, when we would want to populate the fields, we would know which cases have their fields populated and which do not. In this PR, due to time constraints and criticality of the feature, I took the following decisions: - Cases return from their APIs the total comments and alerts of each case. They do that by doing an aggregation, getting the counts, and merging them with the response. I did not change that behavior. In following PRs, it can be optimized and fetch the stats only for cases that do not yet have their stats populated (cases with -1 in the counts) - When a case is created, the counts are zero. - When a comment or alert is added, I do an aggregation to get the stats (total alerts and comments) of the current case, and then update the counters with the number of the newly created attachments. The case is updated without version checks. In race conditions, where an attachment is being added before updating the case, the numbers could be off. This is a deliberate choice. It can be fixed later with retries and version concurrency control. - The case service will continue to not return the `total_alerts` and `total_comments`. - The case service will accept the `total_alerts` and `total_comments` attributes to be able to set them. Fixes: elastic#217636 cc @michaelolo24 ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Add total comments and alerts to the cases so when attaching attachments

7adb826

cnasikas self-assigned this Jun 14, 2025

cnasikas added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// Feature:Cases Cases feature backport:version Backport to applied version labels v9.1.0 v8.19.0 labels Jun 14, 2025

cnasikas added 5 commits June 14, 2025 12:39

Add integration tests

ad5eb62

Add unit tests

7cd98c2

Remove attachment stats from attributes

c10ccc3

Add more unit tests

41bd76a

Merge branch 'main' into cases_total_alerts_comments

4958720

cnasikas marked this pull request as ready for review June 15, 2025 12:52

cnasikas requested a review from a team as a code owner June 15, 2025 12:52

adcoelho reviewed Jun 16, 2025

View reviewed changes

cnasikas requested a review from a team June 16, 2025 12:09

cnasikas added 4 commits June 17, 2025 11:36

Merge branch 'main' into cases_total_alerts_comments

55fe39b

Merge branch 'main' into cases_total_alerts_comments

a2ed1aa

Get the stats from ES and do not calculate on memory

cd25f2e

Add integration tests for deletion

18f4164

adcoelho reviewed Jun 18, 2025

View reviewed changes

adcoelho approved these changes Jun 18, 2025

View reviewed changes

cnasikas added 2 commits June 21, 2025 18:04

Merge branch 'main' into cases_total_alerts_comments

f059301

Fix and add more tests and PR feedback

588c0f9

Fix tests

364e616

michaelolo24 reviewed Jun 23, 2025

View reviewed changes

michaelolo24 approved these changes Jun 23, 2025

View reviewed changes

cnasikas merged commit f30335a into elastic:main Jun 23, 2025
11 checks passed

cnasikas deleted the cases_total_alerts_comments branch June 23, 2025 16:56

cnasikas mentioned this pull request Jun 23, 2025

[8.19] [ResponseOps][Cases] Populate total alerts and comments in the cases saved objects (#223992) #224928

Merged

michaelolo24 mentioned this pull request Sep 24, 2025

[Cases] Add extract observables setting to cases SO mapping #236028

Merged

10 tasks

		\| Partial<CaseTransformedAttributes>
		\| Partial<CaseTransformedAttributesWithAttachmentStats>


		const theCase = res.body.hits.hits[0]._source?.cases!;

		// We added 3 attachments, 2 user and 1 alert and we deleted 1 of each

Conversation

cnasikas commented Jun 14, 2025 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Decisions

Checklist

Uh oh!

elasticmachine commented Jun 15, 2025

Uh oh!

elasticmachine commented Jun 15, 2025

Uh oh!

adcoelho Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cnasikas commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adcoelho Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

cnasikas Jun 21, 2025

Choose a reason for hiding this comment

Uh oh!

adcoelho left a comment

Choose a reason for hiding this comment

Uh oh!

adcoelho Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

cnasikas Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cnasikas commented Jun 21, 2025

Uh oh!

elasticmachine commented Jun 21, 2025

Uh oh!

elasticmachine commented Jun 21, 2025

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

History

Uh oh!

michaelolo24 Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

michaelolo24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Jun 23, 2025

Uh oh!

kibanamachine commented Jun 23, 2025

💔 All backports failed

Manual backport

Questions ?

Uh oh!

cnasikas commented Jun 23, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cnasikas commented Jun 14, 2025 •

edited by kibanamachine

Loading

cnasikas commented Jun 18, 2025 •

edited

Loading

cnasikas Jun 21, 2025 •

edited

Loading