Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Onboard dashboard_telemetry task type to use stateSchemaByVersion for task state validation #161855

Merged
merged 4 commits into from
Jul 20, 2023

Conversation

mikecote
Copy link
Contributor

Part of #159342.

In this PR, I'm preparing the dashboard_telemetry for serverless by defining an explicit task state schema. This schema is used to validate the task's state before saving the task but also when reading the task. In the scenario an older Kibana node runs a task after a newer Kibana node has stored additional task state, the unknown state properties will be dropped. Additionally, this will prompt developers to be aware that adding required fields to the task state is a breaking change that must be handled with care. (see #155764).

For more information on how to use stateSchemaByVersion, see #159048 and https://github.com/elastic/kibana/blob/main/x-pack/plugins/task_manager/README.md.

@mikecote mikecote added release_note:skip Skip the PR/issue when compiling release notes Feature:Task Manager v8.10.0 labels Jul 13, 2023
@mikecote mikecote self-assigned this Jul 13, 2023
@mikecote mikecote added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Jul 13, 2023
@mikecote mikecote marked this pull request as ready for review July 13, 2023 16:05
@mikecote mikecote requested a review from a team as a code owner July 13, 2023 16:05
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes make sense to me, LGTM! Tested locally by ensuring the telemetry collection task runs and that everything still adds up as expected.

import { TASK_ID } from './dashboard_telemetry_collection_task';
import { emptyState, type LatestTaskStateSchema } from './task_state';

// TODO: Merge with LatestTaskStateSchema. Requires a refactor of collectPanelsByType() because
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this TODO something that we should tackle on the Presentation team side? What is the priority?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, I don't foresee urgency on having this fixed, it's mainly if the presentation team would like to have a single source of type definitions after this PR merges. It can be considered technical debt, otherwise the engineer will have to add their new telemetry field in two places.

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

@mikecote mikecote enabled auto-merge (squash) July 20, 2023 11:36
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @mikecote

@mikecote mikecote merged commit 6d70bdf into elastic:main Jul 20, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 20, 2023
dgieselaar pushed a commit to dgieselaar/kibana that referenced this pull request Jul 23, 2023
… task state validation (elastic#161855)

Part of elastic#159342.

In this PR, I'm preparing the `dashboard_telemetry` for serverless by
defining an explicit task state schema. This schema is used to validate
the task's state before saving the task but also when reading the task.
In the scenario an older Kibana node runs a task after a newer Kibana
node has stored additional task state, the unknown state properties will
be dropped. Additionally, this will prompt developers to be aware that
adding required fields to the task state is a breaking change that must
be handled with care. (see
elastic#155764).

For more information on how to use `stateSchemaByVersion`, see
elastic#159048 and
https://github.com/elastic/kibana/blob/main/x-pack/plugins/task_manager/README.md.

---------

Co-authored-by: Kibana Machine <[email protected]>
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
… task state validation (elastic#161855)

Part of elastic#159342.

In this PR, I'm preparing the `dashboard_telemetry` for serverless by
defining an explicit task state schema. This schema is used to validate
the task's state before saving the task but also when reading the task.
In the scenario an older Kibana node runs a task after a newer Kibana
node has stored additional task state, the unknown state properties will
be dropped. Additionally, this will prompt developers to be aware that
adding required fields to the task state is a breaking change that must
be handled with care. (see
elastic#155764).

For more information on how to use `stateSchemaByVersion`, see
elastic#159048 and
https://github.com/elastic/kibana/blob/main/x-pack/plugins/task_manager/README.md.

---------

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Task Manager release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants