[Controls] Register controls in a separate persistable state service by nickpeihl · Pull Request #209174 · elastic/kibana

nickpeihl · 2025-01-31T21:56:29Z

Summary

Register controls in a new ControlsPersistableStateService.

The embeddable persistable state service registry may soon add validation schemas. These schemas will be provided for the Dashboards HTTP API OpenAPI Spec. Since controls are not panels (yet), we want to maintain a separate registry for controls to register schemas so they can be documented in the correct place in the specification.

At this time, this change should have no effect on the UI. This should only affect telemetry collection for controls. In the future, we will use the extract/inject methods on the ControlsPersistableStateService on the Dashboard server so controls will not need to inject and extract references in the UI. More info in #206654.

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
Flaky Test Runner was used on any tests changed
The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

Identify risks

Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.

See some risk examples
...

… src/core/server/integration_tests/ci_checks'

…estateservice

elasticmachine · 2025-02-06T15:52:26Z

Pinging @elastic/kibana-presentation (Team:Presentation)

jloleysens · 2025-02-11T13:50:37Z

src/core/server/integration_tests/ci_checks/saved_objects/check_registered_types.test.ts

        "core-usage-stats": "b3c04da317c957741ebcdedfea4524049fdc79ff",
        "csp-rule-template": "c151324d5f85178169395eecb12bac6b96064654",
-        "dashboard": "211e9ca30f5a95d5f3c27b1bf2b58e6cfa0c9ae9",
+        "dashboard": "c4935ad4c077760a0c4bf5663e1b688114a4a248",


I'm guessing the registered type for these SOs changed bc they store controls, but I'm having a hard time figuring out what about them changed... Would you mind helping me out 😄 ?

The hashes created here for the Dashboard and Canvas saved objects include migrations. Previously, controls registered their migrations on the embeddable persistable state service. With this PR, controls is no longer registering migrations on on the embeddable service, but on its own persistable state service.

So Dashboards and Canvas both run embeddable migrations registered on the embeddable service. But controls can never exist on Canvas workpads. So the controls migrations would never change anything on the Canvas saved objects.

However, I missed including these migrations on the Dashboard saved object migrations. This has been remedied in fc21824.

I added some diffs below that show the hashFeed from src/core/packages/test-helpers/so-type-serializer/src/get_migration_hash.ts before they are encoded to hex. You can see that 8.7.0controls migration is removed from the migrationVersions in the Canvas saved objects. But these migrations are no-ops since controls never existed in Canvas.

- canvas-element-multiple-isolated-none-false-8.0.0-7.13.1,7.14.0,7.15.0,7.16.0,7.17.0,8.0.0,8.0.1,8.1.0,8.2.0,8.3.0,8.4.0,8.5.0,8.6.0,8.7.0,8.9.0--{"dynamic":false,"properties.@created.type":"date","properties.@timestamp.type":"date","properties.content.type":"text","properties.help.type":"text","properties.image.type":"text","properties.name.fields.keyword.type":"keyword","properties.name.type":"text"}-8.10.0- + canvas-element-multiple-isolated-none-false-8.0.0-7.13.1,7.14.0,7.15.0,7.16.0,7.17.0,8.0.0,8.0.1,8.1.0,8.2.0,8.3.0,8.4.0,8.5.0,8.6.0,8.9.0--{"dynamic":false,"properties.@created.type":"date","properties.@timestamp.type":"date","properties.content.type":"text","properties.help.type":"text","properties.image.type":"text","properties.name.fields.keyword.type":"keyword","properties.name.type":"text"}-8.10.0- - canvas-workpad-multiple-isolated-none-false-8.0.0-7.0.0,7.13.1,7.14.0,7.15.0,7.16.0,7.17.0,8.0.0,8.0.1,8.1.0,8.2.0,8.3.0,8.4.0,8.5.0,8.6.0,8.7.0,8.9.0--{"dynamic":false,"properties.@created.type":"date","properties.@timestamp.type":"date","properties.name.fields.keyword.type":"keyword","properties.name.type":"text"}-8.10.0- + canvas-workpad-multiple-isolated-none-false-8.0.0-7.0.0,7.13.1,7.14.0,7.15.0,7.16.0,7.17.0,8.0.0,8.0.1,8.1.0,8.2.0,8.3.0,8.4.0,8.5.0,8.6.0,8.9.0--{"dynamic":false,"properties.@created.type":"date","properties.@timestamp.type":"date","properties.name.fields.keyword.type":"keyword","properties.name.type":"text"}-8.10.0- - canvas-workpad-template-agnostic-none-false-none-7.13.1,7.14.0,7.15.0,7.16.0,7.17.0,8.0.0,8.0.1,8.1.0,8.2.0,8.3.0,8.4.0,8.5.0,8.6.0,8.7.0,8.9.0--{"dynamic":false,"properties.help.fields.keyword.type":"keyword","properties.help.type":"text","properties.name.fields.keyword.type":"keyword","properties.name.type":"text","properties.tags.fields.keyword.type":"keyword","properties.tags.type":"text","properties.template_key.type":"keyword"}-8.10.0- + canvas-workpad-template-agnostic-none-false-none-7.13.1,7.14.0,7.15.0,7.16.0,7.17.0,8.0.0,8.0.1,8.1.0,8.2.0,8.3.0,8.4.0,8.5.0,8.6.0,8.9.0--{"dynamic":false,"properties.help.fields.keyword.type":"keyword","properties.help.type":"text","properties.name.fields.keyword.type":"keyword","properties.name.type":"text","properties.tags.fields.keyword.type":"keyword","properties.tags.type":"text","properties.template_key.type":"keyword"}-8.10.0-

Ohh I see, thanks for the explanation. I don't have a lot of context on the history of controls and canvas, but that makes sense.

elasticmachine · 2025-02-11T17:18:21Z

💚 Build Succeeded

Buildkite Build
Commit: 9c6884d

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`controls`	133	135	+2
`embeddable`	141	143	+2
total			+4

Unknown metric groups

API count

id	before	after	diff
`controls`	137	139	+2
`embeddable`	168	170	+2
total			+4

References to deprecated APIs

id	before	after	diff
`controls`	18	15	-3

History

💔 Build #275181 failed fc21824
💚 Build #274052 succeeded f04a304
💔 Build #272453 failed b6e26d5

jloleysens

Based on what I understand there is no real change to migrations except for canvas which is a noop since the controls migrations would never have run or been used.

Generally I find it a bit scary to remove migrations (even if we believe they are noops) so, my 2c, having some E2E tests in place would make me feel more at ease.

jloleysens · 2025-02-12T10:37:43Z

...ared/dashboard/server/dashboard_saved_object/migrations/dashboard_saved_object_migrations.ts


+  const controlsMigrations = deps.controls.getAllMigrations();
+
+  const dependencyMigrations = mergeMigrationFunctionMaps(embeddableMigrations, controlsMigrations);


So if I'm following correctly: this refactor is split controlsMigrations from embeddableMigrations and so the end result for dashboards is the same...

ThomThomson

Great to see this separated out, especially because it also mirrors some of the changes we've made clientside where Controls are no longer in the same Embeddable registry.

Looked through the code and everything looks great. Also ran locally, imported the controls migration smoke test (with the workaround to #211113 in place, and ensured that the Controls migrations run properly.

gsoldevila

I think we should define a new modelVersion with an unsafe_transform function instead.

Otherwise, this function will not run for customers that migrate from a version newer than the ones defined in the newly added "migrations", as the data will be considered up-to-date.

nickpeihl · 2025-03-06T15:18:37Z

I think we should define a new modelVersion with an unsafe_transform function instead.

Otherwise, this function will not run for customers that migrate from a version newer than the ones defined in the newly added "migrations", as the data will be considered up-to-date.

There might be a safer and easier method where we use Dashboard's Content Management system to transform at run time the controlWidth: "auto" in the saved object to the current ControlWidth type. This would avoid requiring a new modelVersion. @ThomThomson what do you think?

ThomThomson · 2025-03-06T16:22:42Z

I think a runtime transformation in Content Management sounds like a safe way to solve this. @gsoldevila, is this something that is acceptable to core?

Additionally, this PR isn't introducing any new migrations or transforms - would you mind updating your review?

gsoldevila · 2025-03-07T11:15:22Z

src/core/server/integration_tests/ci_checks/saved_objects/check_registered_types.test.ts

-        "canvas-workpad-template": "c077b0087346776bb3542b51e1385d172cb24179",
+        "canvas-element": "5cea187f4d74520e3da36b18e465e91d344fad3e",
+        "canvas-workpad": "b38728356a6ee25d560f519671735fdbadf4604a",
+        "canvas-workpad-template": "e32ebe04845a65e33fd441c3d0ab47053c2e3016",


I don't understand why these old types have been updated here 🤔 .

nreese · 2025-03-12T19:36:31Z

Ca you hold off merging until #214265 is resolved. That way, we can avoid merge conflicts with backporting as much as possible

nickpeihl and others added 3 commits January 31, 2025 16:51

Register controls in a separate persistable state service

2def47b

[CI] Auto-commit changed files from 'node scripts/jest_integration -u…

b6e26d5

… src/core/server/integration_tests/ci_checks'

Merge remote-tracking branch 'upstream/main' into controls-persistabl…

f04a304

…estateservice

nickpeihl marked this pull request as ready for review February 6, 2025 15:51

nickpeihl requested review from a team as code owners February 6, 2025 15:51

nickpeihl added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// release_note:skip Skip the PR/issue when compiling release notes labels Feb 6, 2025

nickpeihl added the backport:skip This PR does not require backporting label Feb 6, 2025

nickpeihl mentioned this pull request Feb 7, 2025

[Dashboards] Inject/extract references on server #206654

Draft

15 tasks

jloleysens reviewed Feb 11, 2025

View reviewed changes

botelastic bot added the Feature:Embedding Embedding content via iFrame label Feb 11, 2025

nickpeihl added 3 commits February 11, 2025 10:00

Fix missing controls migrations in dashboard migration

fc21824

Fix tests

f0b5f1e

Merge branch 'main' into controls-persistablestateservice

9c6884d

jloleysens approved these changes Feb 12, 2025

View reviewed changes

ThomThomson approved these changes Feb 14, 2025

View reviewed changes

gsoldevila requested changes Feb 17, 2025

View reviewed changes

gsoldevila reviewed Mar 7, 2025

View reviewed changes

gsoldevila approved these changes Mar 7, 2025

View reviewed changes


		const controlsMigrations = deps.controls.getAllMigrations();

		const dependencyMigrations = mergeMigrationFunctionMaps(embeddableMigrations, controlsMigrations);

Conversation

nickpeihl commented Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Identify risks

Uh oh!

elasticmachine commented Feb 6, 2025

Uh oh!

jloleysens Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

nickpeihl Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

jloleysens Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Feb 11, 2025

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

References to deprecated APIs

History

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

jloleysens Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

nickpeihl Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

ThomThomson left a comment

Choose a reason for hiding this comment

Uh oh!

gsoldevila left a comment

Choose a reason for hiding this comment

Uh oh!

nickpeihl commented Mar 6, 2025

Uh oh!

ThomThomson commented Mar 6, 2025

Uh oh!

gsoldevila Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

nreese commented Mar 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nickpeihl commented Jan 31, 2025 •

edited

Loading