[DataViews] - Add caching to fields.toSpec by michaelolo24 · Pull Request #224767 · elastic/kibana

michaelolo24 · 2025-06-21T04:23:15Z

Summary

The goal of this PR is primarily for performance improvements. When the index patterns in a given dataview have a significant enough number of fields, the performance of the application can slow down pretty significantly. When accessing the dataview saved object directly, we have the benefit of the network request being cached here.

This drastically helps with the performance when working with the dataview directly, but when working with the DataViewSpec produced by DataView.toSpec() here, there is no such optimization. This can be problematic given a significant number of fields and the nested iterations in the fields.toSpec() call seen here and here.

This PR introduces a simple in memory cache for this potentially expensive fields.toSpec() call.

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

Identify risks

Potential risk: since the cache is entirely in the UI, there could be a potential issue confirming the cache is invalidated, bust since it is entirely in memory, this is mitigated by just refreshing the given view.

michaelolo24 · 2025-06-23T04:05:21Z

src/platform/plugins/shared/data_views/common/fields/field_list.test.ts

+    expect(shortDotsList[0].spec.shortDotsEnable).toBe(true);
+  });
+
+  it('does not fail if add is called with a field with the same name', () => {


added, but not sure if this should be valid?

Hmm hard to say, but I'd be hesitant to change it now 😅

michaelolo24 · 2025-06-23T04:06:06Z

src/platform/plugins/shared/data_views/common/fields/field_list.ts

      this.byName.delete(field.name);

      const fieldIndex = findIndex(this, { name: field.name });
+      if (fieldIndex === -1) return;


if a name is provided not actually in the list, the mutation still took place.

Wait, so did this used to just always remove the last field in the list if passed a not-found field? 🤔

elasticmachine · 2025-06-23T04:09:28Z

Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations)

elasticmachine · 2025-06-24T10:47:50Z

💔 Build Failed

Failed CI Steps

Test Failures

[job] [logs] Investigations - Security Solution Cypress Tests #1 / Alert details expandable flyout right panel should mark as acknowledged should mark as acknowledged
[job] [logs] Serverless Investigations - Security Solution Cypress Tests #10 / Alert details expandable flyout right panel should mark as acknowledged should mark as acknowledged
[job] [logs] Investigations - Security Solution Cypress Tests #1 / Alert details expandable flyout right panel should mark as closed should mark as closed
[job] [logs] Serverless Investigations - Security Solution Cypress Tests #10 / Alert details expandable flyout right panel should mark as closed should mark as closed
[job] [logs] FTR Configs #100 / Rules Management - Rule Bulk Action API @ess @serverless @skipInServerless perform_bulk_action @skipInServerlessMKI fill gaps run action should trigger the backfilling of the gaps for the rules in the request

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`dataViews`	62.0KB	62.3KB	+329.0B

Unknown metric groups

API count

id	before	after	diff
`dataViews`	1241	1243	+2

History

💔 Build #311486 failed 47eccad
💔 Build #311207 failed c50000e

davismcphee

The code changes look good in general and the idea makes sense, but it's hard to have confidence in these types of changes since data views are so broadly relied on. I left a few comments and questions, and some ideas on how we might better gauge the impact. It's going to be a tricky one for sure.

Also what approach were you using for testing the changes locally? I'm not really sure where to begin with it tbh.

davismcphee · 2025-06-26T01:15:51Z

src/platform/plugins/shared/data_views/common/data_views/data_views.test.ts

+    const firstSpec = indexPattern.fields.toSpec();
+    const secondSpec = indexPattern.fields.toSpec();
+    expect(firstSpec).toEqual(secondSpec);


One risk here is if there's anywhere in the codebase that retrieves and modifies a spec, it could corrupt the cache value. Maybe we should use defaultFreeze on it like what's done on responses in this PR, which will freeze it in dev and help catch these issues.

Do you know if CI runs in dev? I'd feel a bit more confident if we had a full CI run with freezing enabled in case it reveals any issues. Otherwise we might want to let it go through CI once with deepFreeze explicitly to see if it passes.

davismcphee · 2025-06-26T01:25:38Z

src/platform/plugins/shared/data_views/common/fields/field_list.test.ts

Nice to actually have some tests for this now 👍

davismcphee · 2025-06-26T01:28:23Z

src/platform/plugins/shared/data_views/common/fields/field_list.test.ts

+  it('does not throw if remove is called on a field not in the list', () => {
+    const field = new DataViewField({ ...baseField, name: 'notfound' });
+    expect(() => list.remove(field)).not.toThrow();
+  });
+
+  it('does not throw if update is called on a field not in the list', () => {
+    const field: FieldSpec = { ...baseField, name: 'notfound' };
+    expect(() => list.update(field)).not.toThrow();
+  });


Are these duplicates of the above tests with "handles _ on non-existent field gracefully"?

davismcphee · 2025-06-26T01:29:10Z

src/platform/plugins/shared/data_views/common/fields/field_list.test.ts

+    expect(shortDotsList[0].spec.shortDotsEnable).toBe(true);
+  });
+
+  it('does not fail if add is called with a field with the same name', () => {


Hmm hard to say, but I'd be hesitant to change it now 😅

davismcphee · 2025-06-26T01:37:56Z

src/platform/plugins/shared/data_views/common/data_views/data_view.ts

      existingField.runtimeField = undefined;
+      this.fields.clearSpecCache();


It sucks that we need to expose a public clearSpecCache method just for this case... But it also sucks that we do this by mutating the field (and that we expose mutable props on the field class in the first place). Kinda makes me feel like we should try sticking readonly on all the public field class props and run a CI build to see if we catch anywhere else in the codebase where we do this, since it could corrupt the cache value.

Thinking about it a bit more, wouldn't we need to invalidate the entire field list cache whenever any property of any field is modified? Most of them look to be mutable.

davismcphee · 2025-06-26T01:45:49Z

src/platform/plugins/shared/data_views/common/fields/field_list.ts

      this.byName.delete(field.name);

      const fieldIndex = findIndex(this, { name: field.name });
+      if (fieldIndex === -1) return;


Wait, so did this used to just always remove the last field in the list if passed a not-found field? 🤔

davismcphee · 2025-06-26T02:00:31Z

src/platform/plugins/shared/data_views/common/fields/field_list.ts

A couple of questions to better understand the security use case:

Why does security use data view specs so heavily? Would it not work better to use actual data view instances instead, which are already cached by the data views service?

Would using specs that don't include the fields, e.g. toSpec(false) or toMinimalSpec() help at all here? I'm guessing not since presumably you want to keep the fields around?

michaelolo24 · 2025-06-28T00:43:59Z

Closing this for now as we do not need to rely on this for our performance enhancements, but a noticeable benefit was seen for even Discover with this caching layer.

michaelolo24 mentioned this pull request Jun 22, 2025

Sourcerer Improvements #224781

Closed

9 tasks

michaelolo24 force-pushed the improve-fields-caching branch from 90659fd to c50000e Compare June 22, 2025 11:13

michaelolo24 added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting v9.1.0 v8.19.0 Team:Threat Hunting:Investigations Security Solution Threat Hunting Investigations Team labels Jun 22, 2025

michaelolo24 mentioned this pull request Jun 22, 2025

[WIP] - Add dataview cache to toSpec call #223466

Closed

michaelolo24 commented Jun 23, 2025

View reviewed changes

michaelolo24 changed the title ~~add caching to fields spec call~~ [DataViews] - Add caching to fields.toSpec Jun 23, 2025

michaelolo24 added ci:cloud-deploy Create or update a Cloud deployment ci:cloud-redeploy Always create a new Cloud deployment ci:cloud-persist-deployment Persist cloud deployment indefinitely and removed ci:cloud-redeploy Always create a new Cloud deployment labels Jun 23, 2025

michaelolo24 marked this pull request as ready for review June 23, 2025 04:09

michaelolo24 requested a review from a team as a code owner June 23, 2025 04:09

michaelolo24 added 2 commits June 23, 2025 09:14

add caching to fields spec call

7b16d27

minor changes

47eccad

michaelolo24 force-pushed the improve-fields-caching branch from 7c52bdc to 47eccad Compare June 23, 2025 13:14

This was referenced Jun 23, 2025

[Security Solution][Sourcerer] Replace unnecessary useDataViewSpec calls #224831

Merged

[Security Solution][Sourcerer] Fix timeline filters for DV manager #224834

Merged

Merge branch 'main' into improve-fields-caching

3365ea8

davismcphee reviewed Jun 26, 2025

View reviewed changes

michaelolo24 closed this Jun 28, 2025

		existingField.runtimeField = undefined;
		this.fields.clearSpecCache();

Conversation

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

Uh oh!

Summary

Checklist

Identify risks

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jun 23, 2025

Uh oh!

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

Uh oh!

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

Page load bundle

API count

History

Uh oh!

davismcphee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelolo24 commented Jun 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

michaelolo24 commented Jun 21, 2025 •

edited

Loading

elasticmachine commented Jun 24, 2025 •

edited

Loading