-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ES|QL] [Discover] Keeps the preferred chart configuration when possible #197453
Conversation
@@ -43,3 +43,18 @@ export interface Suggestion<T = unknown, V = unknown> { | |||
changeType: TableChangeType; | |||
keptLayerIds: string[]; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this here as it is used in many places already
src/plugins/unified_histogram/public/services/lens_vis_service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving but please consider the comment I've left above
src/plugins/unified_histogram/public/services/lens_vis_service.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com>
export const mapVisToChartType = (visualizationType: string) => { | ||
switch (visualizationType) { | ||
case LensVisualizationType.XY: | ||
return ChartType.XY; | ||
case LensVisualizationType.Metric: | ||
case LensVisualizationType.LegacyMetric: | ||
return ChartType.Metric; | ||
case LensVisualizationType.Pie: | ||
return ChartType.Pie; | ||
case LensVisualizationType.Heatmap: | ||
return ChartType.Heatmap; | ||
case LensVisualizationType.Gauge: | ||
return ChartType.Gauge; | ||
case LensVisualizationType.Datatable: | ||
return ChartType.Table; | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing Marco's comment I actually think he might be right. Changing the code to the one below would make sure that if we add a new lens vis type, typescript will complain. WDYT?
export const mapVisToChartType = (visualizationType: string) => { | |
switch (visualizationType) { | |
case LensVisualizationType.XY: | |
return ChartType.XY; | |
case LensVisualizationType.Metric: | |
case LensVisualizationType.LegacyMetric: | |
return ChartType.Metric; | |
case LensVisualizationType.Pie: | |
return ChartType.Pie; | |
case LensVisualizationType.Heatmap: | |
return ChartType.Heatmap; | |
case LensVisualizationType.Gauge: | |
return ChartType.Gauge; | |
case LensVisualizationType.Datatable: | |
return ChartType.Table; | |
} | |
}; | |
type ValueOf<T> = T[keyof T]; | |
type LensToChartMap = { | |
[K in ValueOf<typeof LensVisualizationType>]: ChartType; | |
}; | |
const lensTypesToChartTypes: LensToChartMap = { | |
[LensVisualizationType.XY]: ChartType.XY, | |
[LensVisualizationType.Metric]: ChartType.Metric, | |
[LensVisualizationType.LegacyMetric]: ChartType.Metric, | |
[LensVisualizationType.Pie]: ChartType.Pie, | |
[LensVisualizationType.Heatmap]: ChartType.Heatmap, | |
[LensVisualizationType.Gauge]: ChartType.Gauge, | |
[LensVisualizationType.Datatable]: ChartType.Table, | |
}; | |
function isLensVisualizationType(value: string): value is LensVisualizationType { | |
return value in LensVisualizationType; | |
} | |
export const mapVisToChartType = (visualizationType: string) => { | |
if (isLensVisualizationType(visualizationType)) { | |
return lensTypesToChartTypes[visualizationType]; | |
} | |
}; | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanx MArta, done here 6daa2c8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This creates a bug actually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here cf707f8
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
History
|
Starting backport for target branches: 8.x https://github.com/elastic/kibana/actions/runs/11693873531 |
…ble (elastic#197453) ## Summary Closes elastic#184631 It keeps the chart configuration when the user is doing actions compatible with the current query such as: - Adding a where filter (by clicking the table, the sidebar, the chart) - Changes the breakdown field and the field type is compatible with the current chart - Changing to a compatible chart type (from example from bar to line or pie to treemap) - Changing the query that doesnt affect the generated columns mapped to a chart. For example adding a limit or creating a runtime field etc. The logic depends on the suggestions. If the suggestions return the preferred chart type, then we are going to use this. So it really depends on the api and the type / number of columns. It is as smarter as it can in order to not create bugs. I am quite happy with the result. It is much better than what we have so far. ![meow](https://github.com/user-attachments/assets/c4249e5e-e785-4e57-8651-d1f660f5a61a) ### Next steps I would love to do the same on the dahsboard too, needs more time though. But the changes made here will def work in favor ### Checklist - [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 - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed --------- Co-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com> (cherry picked from commit ccbcab9)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
… possible (#197453) (#199069) # Backport This will backport the following commits from `main` to `8.x`: - [[ES|QL] [Discover] Keeps the preferred chart configuration when possible (#197453)](#197453) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Stratoula Kalafateli","email":"efstratia.kalafateli@elastic.co"},"sourceCommit":{"committedDate":"2024-11-05T22:56:17Z","message":"[ES|QL] [Discover] Keeps the preferred chart configuration when possible (#197453)\n\n## Summary\r\n\r\nCloses https://github.com/elastic/kibana/issues/184631\r\n\r\nIt keeps the chart configuration when the user is doing actions\r\ncompatible with the current query such as:\r\n\r\n- Adding a where filter (by clicking the table, the sidebar, the chart)\r\n- Changes the breakdown field and the field type is compatible with the\r\ncurrent chart\r\n- Changing to a compatible chart type (from example from bar to line or\r\npie to treemap)\r\n- Changing the query that doesnt affect the generated columns mapped to\r\na chart. For example adding a limit or creating a runtime field etc.\r\n\r\nThe logic depends on the suggestions. If the suggestions return the\r\npreferred chart type, then we are going to use this. So it really\r\ndepends on the api and the type / number of columns. It is as smarter as\r\nit can in order to not create bugs. I am quite happy with the result. It\r\nis much better than what we have so far.\r\n\r\n\r\n![meow](https://github.com/user-attachments/assets/c4249e5e-e785-4e57-8651-d1f660f5a61a)\r\n\r\n### Next steps\r\nI would love to do the same on the dahsboard too, needs more time\r\nthough. But the changes made here will def work in favor\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n\r\n---------\r\n\r\nCo-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com>","sha":"ccbcab9623af4df0bdccb0e3e194b8484d2161a1","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Discover","enhancement","v9.0.0","release_note:feature","Team:Obs AI Assistant","Feature:ES|QL","ci:project-deploy-observability","backport:version","v8.17.0"],"title":"[ES|QL] [Discover] Keeps the preferred chart configuration when possible","number":197453,"url":"https://github.com/elastic/kibana/pull/197453","mergeCommit":{"message":"[ES|QL] [Discover] Keeps the preferred chart configuration when possible (#197453)\n\n## Summary\r\n\r\nCloses https://github.com/elastic/kibana/issues/184631\r\n\r\nIt keeps the chart configuration when the user is doing actions\r\ncompatible with the current query such as:\r\n\r\n- Adding a where filter (by clicking the table, the sidebar, the chart)\r\n- Changes the breakdown field and the field type is compatible with the\r\ncurrent chart\r\n- Changing to a compatible chart type (from example from bar to line or\r\npie to treemap)\r\n- Changing the query that doesnt affect the generated columns mapped to\r\na chart. For example adding a limit or creating a runtime field etc.\r\n\r\nThe logic depends on the suggestions. If the suggestions return the\r\npreferred chart type, then we are going to use this. So it really\r\ndepends on the api and the type / number of columns. It is as smarter as\r\nit can in order to not create bugs. I am quite happy with the result. It\r\nis much better than what we have so far.\r\n\r\n\r\n![meow](https://github.com/user-attachments/assets/c4249e5e-e785-4e57-8651-d1f660f5a61a)\r\n\r\n### Next steps\r\nI would love to do the same on the dahsboard too, needs more time\r\nthough. But the changes made here will def work in favor\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n\r\n---------\r\n\r\nCo-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com>","sha":"ccbcab9623af4df0bdccb0e3e194b8484d2161a1"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/197453","number":197453,"mergeCommit":{"message":"[ES|QL] [Discover] Keeps the preferred chart configuration when possible (#197453)\n\n## Summary\r\n\r\nCloses https://github.com/elastic/kibana/issues/184631\r\n\r\nIt keeps the chart configuration when the user is doing actions\r\ncompatible with the current query such as:\r\n\r\n- Adding a where filter (by clicking the table, the sidebar, the chart)\r\n- Changes the breakdown field and the field type is compatible with the\r\ncurrent chart\r\n- Changing to a compatible chart type (from example from bar to line or\r\npie to treemap)\r\n- Changing the query that doesnt affect the generated columns mapped to\r\na chart. For example adding a limit or creating a runtime field etc.\r\n\r\nThe logic depends on the suggestions. If the suggestions return the\r\npreferred chart type, then we are going to use this. So it really\r\ndepends on the api and the type / number of columns. It is as smarter as\r\nit can in order to not create bugs. I am quite happy with the result. It\r\nis much better than what we have so far.\r\n\r\n\r\n![meow](https://github.com/user-attachments/assets/c4249e5e-e785-4e57-8651-d1f660f5a61a)\r\n\r\n### Next steps\r\nI would love to do the same on the dahsboard too, needs more time\r\nthough. But the changes made here will def work in favor\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n\r\n---------\r\n\r\nCo-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com>","sha":"ccbcab9623af4df0bdccb0e3e194b8484d2161a1"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
…ble (elastic#197453) ## Summary Closes elastic#184631 It keeps the chart configuration when the user is doing actions compatible with the current query such as: - Adding a where filter (by clicking the table, the sidebar, the chart) - Changes the breakdown field and the field type is compatible with the current chart - Changing to a compatible chart type (from example from bar to line or pie to treemap) - Changing the query that doesnt affect the generated columns mapped to a chart. For example adding a limit or creating a runtime field etc. The logic depends on the suggestions. If the suggestions return the preferred chart type, then we are going to use this. So it really depends on the api and the type / number of columns. It is as smarter as it can in order to not create bugs. I am quite happy with the result. It is much better than what we have so far. ![meow](https://github.com/user-attachments/assets/c4249e5e-e785-4e57-8651-d1f660f5a61a) ### Next steps I would love to do the same on the dahsboard too, needs more time though. But the changes made here will def work in favor ### Checklist - [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 - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed --------- Co-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com>
I came across this in the release notes for 8.17 and I've definitely been bitten by this before. Looking forward to trying this out! |
Summary
Closes #184631
It keeps the chart configuration when the user is doing actions compatible with the current query such as:
The logic depends on the suggestions. If the suggestions return the preferred chart type, then we are going to use this. So it really depends on the api and the type / number of columns. It is as smarter as it can in order to not create bugs. I am quite happy with the result. It is much better than what we have so far.
Next steps
I would love to do the same on the dahsboard too, needs more time though. But the changes made here will def work in favor
Checklist