[SOR] Remove `fields` API #153447

afharo · 2023-03-22T14:03:21Z

Summary

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

Risk Matrix

Risk	Probability	Severity	Mitigation/Notes
Performance impact from loading the entirety of the documents	High	Low	On average, the documents are small enough to consider this an issue.
External consumers may be affected by this breaking change.	Low	Low	It's a very specific API that's probably not widely used (by 3rd-party apps). The workaround is fairly simple.

For maintainers

This was checked for breaking API changes and was labeled appropriately

afharo · 2023-03-24T13:45:44Z

...s/core-saved-objects-import-export-server-internal/src/import/lib/check_reference_origins.ts

@@ -43,7 +43,6 @@ async function checkOrigin(
    rootSearchFields: ['_id', 'originId'],
    page: 1,
    perPage: 1, // we only need one result for now
-    fields: ['title'], // we don't actually need the object's title, we just specify one field so we don't fetch *all* fields


We may want to add an option for retrieving the root fields only

Perhaps an option to return just "meta" fields like ID, createdAt, updatedAt?

afharo · 2023-03-24T13:55:22Z

x-pack/plugins/alerting/server/routes/find_rules.ts

-        if (req.query.fields) {
-          usageCounter?.incrementCounter({
-            counterName: `alertingFieldsUsage`,
-            counterType: 'alertingFieldsUsage',
-            incrementBy: 1,
-          });
-        }


I checked our telemetry, and this is not used in 8.x versions.

afharo · 2023-03-24T13:56:38Z

x-pack/plugins/alerting/server/routes/legacy/find.ts

-      if (query.fields) {
-        usageCounter?.incrementCounter({
-          counterName: `legacyAlertingFieldsUsage`,
-          counterType: 'alertingFieldsUsage',
-          incrementBy: 1,
-        });
-      }


Telemetry tells us that 7.16 was the last version this was used.

elasticmachine · 2023-03-24T19:16:37Z

Pinging @elastic/kibana-core (Team:Core)

sorenlouv · 2023-03-24T21:40:00Z

x-pack/plugins/apm/server/saved_objects/migrations/migrate_legacy_apm_indices_to_space_aware.ts

@@ -57,7 +57,6 @@ export async function migrateLegacyAPMIndicesToSpaceAware({
      type: 'space',
      page: 1,
      perPage: 10_000, // max number of spaces as of 8.2
-      fields: ['name'], // to avoid fetching *all* fields


This fee;s like a pretty big change in terms of performance. Is the suggestion to avoid using SOs for documents with many fields (or fields containing big blobs of data) where only few or lightweight fields are needed in the majories of queries?

We are already using custom (system) indices for certain features (agent configs, custom links) where SOs were deemed a bad fit. This seems like yet another reason to avoid SOs.

Edit: Perhaps worth adding to #80912

I'm also concerned about the performance impact of this change. Synthetics is using saved objects for monitor configurations, and those documents can quite heavy. This is especially true for our project monitor feature where the script of the monitor is base64 encoded zip file of the script, plus any imports including node_modules.

If the saved object client was only called on the server side, and the fields set to the request were always hardcoded, not part of the HTTP API contract, and transformed to match the HTTP contract before returning the data, would that be enough to satisfy the requirements for the HTTP versioning initiative? This is close to how we use fields currently and matches our original expectations of the work needed to meet the requirements.

elasticmachine · 2023-03-24T21:40:07Z

Pinging @elastic/fleet (Team:Fleet)

elasticmachine · 2023-03-24T21:40:07Z

Pinging @elastic/apm-ui (Team:APM)

elasticmachine · 2023-03-24T21:40:07Z

Pinging @elastic/uptime (Team:uptime)

azasypkin

Changes in SO API integration tests LGTM.

kibana-ci · 2023-03-27T09:17:58Z

💚 Build Succeeded

Buildkite Build
Commit: 529da57

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
`dashboard`	387.9KB	387.9KB	-34.0B
`lens`	1.3MB	1.3MB	-17.0B
`maps`	2.7MB	2.7MB	-48.0B
`ml`	3.4MB	3.4MB	-17.0B
`savedObjectsFinder`	5.2KB	5.1KB	-118.0B
`securitySolution`	15.8MB	15.8MB	-17.0B
total			-251.0B

Page load bundle

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

id	before	after	diff
`core`	351.9KB	351.9KB	-16.0B
`savedObjects`	29.5KB	29.5KB	-17.0B
`savedSearch`	5.4KB	5.4KB	-17.0B
`visualizations`	57.1KB	57.1KB	-17.0B
total			-67.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-api-server`	338	336	-2

ESLint disabled line counts

id	before	after	diff
`securitySolution`	433	436	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	513	516	+3

History

💚 Build #115395 succeeded 796ccf1
💔 Build #115294 failed 72b8f87
💛 Build #115243 was flaky 3c087d4
💔 Build #115019 failed a502da6
💔 Build #114961 failed ccd71c4
💔 Build #114875 failed 26d990f

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @afharo

tonyghiani

Infra UI changes LGTM.

jloleysens

I read through core-only changes and they look good to me. Great job @afharo !

I am glad to see we are reducing API surface area, but I do wonder whether removal of this option has any performance impact on either server or browser? I don't have data to think there is an issue but I can imagine that if we are now returning an array of really large objects there could be an issue.

Otherwise, approving to unblock progress!

jloleysens · 2023-03-27T10:14:01Z

...s/core-saved-objects-import-export-server-internal/src/import/lib/check_reference_origins.ts

@@ -43,7 +43,6 @@ async function checkOrigin(
    rootSearchFields: ['_id', 'originId'],
    page: 1,
    perPage: 1, // we only need one result for now
-    fields: ['title'], // we don't actually need the object's title, we just specify one field so we don't fetch *all* fields


Perhaps an option to return just "meta" fields like ID, createdAt, updatedAt?

juliaElastic

Fleet change LGTM, though I have the same concern related to performance. Is there an alternative we could use to avoid returning all fields?

afharo · 2023-03-27T13:33:32Z

@sqren @juliaElastic @shahzad31, the main reason for this change is that, in the scope of ZDT migrations, Kibana might run when migrations have not been applied yet. In that phase, we might apply the migrations at runtime. Fetching a partial document from ES before being migrated won't ensure the requested fields exist.

Having said that, we are looking at an alternative option: during the transition, internally fetch the entire document from ES, apply the migration, and lodash.pick the fields. I've created #153766 to track this effort. I'll close this PR in the meanwhile 😇

We might need some help from your end, though: given we are using these filters for performance purposes, we'd like to know the performance gain. It'd be great if we had some scalability tests for the tasks you want to optimize.

afharo added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Saved Objects release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Mar 22, 2023

afharo self-assigned this Mar 22, 2023

afharo force-pushed the sor/remove-fields-API branch 10 times, most recently from 3c087d4 to 72b8f87 Compare March 24, 2023 13:43

afharo commented Mar 24, 2023

View reviewed changes

[SOR] Remove fields API

796ccf1

afharo force-pushed the sor/remove-fields-API branch from 72b8f87 to 796ccf1 Compare March 24, 2023 17:20

afharo marked this pull request as ready for review March 24, 2023 19:16

afharo requested review from a team as code owners March 24, 2023 19:16

afharo requested a review from xcrzx March 24, 2023 19:16

sorenlouv reviewed Mar 24, 2023

View reviewed changes

botelastic bot added Team:APM All issues that need APM UI Team support Team:Fleet Team label for Observability Data Collection Fleet team Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Mar 24, 2023

Merge branch 'main' into sor/remove-fields-API

529da57

azasypkin approved these changes Mar 27, 2023

View reviewed changes

tonyghiani approved these changes Mar 27, 2023

View reviewed changes

jloleysens approved these changes Mar 27, 2023

View reviewed changes

juliaElastic approved these changes Mar 27, 2023

View reviewed changes

CohenIdo approved these changes Mar 27, 2023

View reviewed changes

afharo closed this Mar 27, 2023

afharo mentioned this pull request Mar 27, 2023

SOR: remove fields option from the find API #152808

Closed

afharo deleted the sor/remove-fields-API branch October 1, 2024 08:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SOR] Remove `fields` API #153447

[SOR] Remove `fields` API #153447

afharo commented Mar 22, 2023 •

edited

Loading

afharo Mar 24, 2023

jloleysens Mar 27, 2023

afharo Mar 24, 2023

afharo Mar 24, 2023

elasticmachine commented Mar 24, 2023

sorenlouv Mar 24, 2023 •

edited

Loading

dominiqueclarke Mar 27, 2023

elasticmachine commented Mar 24, 2023

elasticmachine commented Mar 24, 2023

elasticmachine commented Mar 24, 2023

azasypkin left a comment

kibana-ci commented Mar 27, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

tonyghiani left a comment

jloleysens left a comment

jloleysens Mar 27, 2023

juliaElastic left a comment •

edited

Loading

afharo commented Mar 27, 2023

[SOR] Remove fields API #153447

[SOR] Remove fields API #153447

Conversation

afharo commented Mar 22, 2023 • edited Loading

Summary

Checklist

Risk Matrix

For maintainers

afharo Mar 24, 2023

Choose a reason for hiding this comment

jloleysens Mar 27, 2023

Choose a reason for hiding this comment

afharo Mar 24, 2023

Choose a reason for hiding this comment

afharo Mar 24, 2023

Choose a reason for hiding this comment

elasticmachine commented Mar 24, 2023

sorenlouv Mar 24, 2023 • edited Loading

Choose a reason for hiding this comment

dominiqueclarke Mar 27, 2023

Choose a reason for hiding this comment

elasticmachine commented Mar 24, 2023

elasticmachine commented Mar 24, 2023

elasticmachine commented Mar 24, 2023

azasypkin left a comment

Choose a reason for hiding this comment

kibana-ci commented Mar 27, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

tonyghiani left a comment

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

jloleysens Mar 27, 2023

Choose a reason for hiding this comment

juliaElastic left a comment • edited Loading

Choose a reason for hiding this comment

afharo commented Mar 27, 2023

[SOR] Remove `fields` API #153447

[SOR] Remove `fields` API #153447

afharo commented Mar 22, 2023 •

edited

Loading

sorenlouv Mar 24, 2023 •

edited

Loading

juliaElastic left a comment •

edited

Loading