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

[Lens] Improve request error in case of last value of a runtime field returning array values #149011

Closed
wants to merge 5 commits into from

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Jan 17, 2023

Summary

Fix #148613

This PR introduces a basic infrastructure to improve with more context runtime errors (errors that we know only after an ES request) in Lens by applying specific operations knowledge to the raw error message.
In this case a runtime field of type keyword that returns an array of strings would not be supported if the used operation is last value (top_metrics underneath) and the new error message would try to explain to the user the problem and how to solve it.

Screenshot 2023-01-17 at 11 04 17

In the screenshot above a single runtime error is reported, and translated. Once the runtime_array configuration is resolved a new runtime error would appear about the runtime_ip_array dimension. Unfortunately ES will report a single error at the time, based on the order in the request, but the logic in this PR is flexible enough to handle also multiple errors in the future.

Note: due to tech challenges in the current Lens flow there's no easy way to highlight the specific dimension component for the error yet. This is a known limitation of this fix.

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space. Low High Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. High Low Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled. Medium High Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

@dej611 dej611 added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens labels Jan 17, 2023
@kibana-ci
Copy link
Collaborator

kibana-ci commented Jan 17, 2023

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #11 / last_value buildColumn should set showArrayValues if field is scripted or comes from existing params
  • [job] [logs] Jest Tests #11 / last_value onFieldChange should set show array values if field is a runtime field
  • [job] [logs] FTR Configs #48 / lens app - group 1 lens text based language tests should allow adding an text based languages chart to a dashboard
  • [job] [logs] FTR Configs #48 / lens app - group 1 lens text based language tests should allow adding an text based languages chart to a dashboard
  • [job] [logs] FTR Configs #48 / lens app - group 1 lens text based language tests should allow adding and using a field
  • [job] [logs] FTR Configs #48 / lens app - group 1 lens text based language tests should allow adding and using a field
  • [job] [logs] FTR Configs #48 / lens app - group 1 lens text based language tests should allow saving the text based languages chart into a saved object
  • [job] [logs] FTR Configs #48 / lens app - group 1 lens text based language tests should allow saving the text based languages chart into a saved object
  • [job] [logs] FTR Configs #48 / lens app - group 1 lens text based language tests should allow switching to another chart
  • [job] [logs] FTR Configs #48 / lens app - group 1 lens text based language tests should allow switching to another chart
  • [job] [logs] FTR Configs #48 / lens app - group 1 lens text based language tests should allow using an index pattern that is not translated to a dataview
  • [job] [logs] FTR Configs #48 / lens app - group 1 lens text based language tests should allow using an index pattern that is not translated to a dataview
  • [job] [logs] FTR Configs #10 / machine learning - short tests model management trained models for ML power user with imported models stops deployment of the imported model pt_tiny_pass_through
  • [job] [logs] Jest Tests #11 / workspace_panel should base saveability on working changes when auto-apply disabled
  • [job] [logs] Jest Tests #11 / workspace_panel should call getTriggerCompatibleActions on hasCompatibleActions call from within renderer
  • [job] [logs] Jest Tests #11 / workspace_panel should execute a trigger on expression event
  • [job] [logs] Jest Tests #11 / workspace_panel should give user control when auto-apply disabled
  • [job] [logs] Jest Tests #11 / workspace_panel should not attempt to run the expression again if it does not change
  • [job] [logs] Jest Tests #11 / workspace_panel should NOT display errors for unapplied changes
  • [job] [logs] Jest Tests #11 / workspace_panel should not show the management action in case of missing indexpattern and no indexPattern specific permissions
  • [job] [logs] Jest Tests #11 / workspace_panel should not show the management action in case of missing indexpattern and no navigation permissions
  • [job] [logs] Jest Tests #11 / workspace_panel should push add current data table to state on data$ emitting value
  • [job] [logs] Jest Tests #11 / workspace_panel should render the resulting expression using the expression renderer
  • [job] [logs] Jest Tests #11 / workspace_panel should run the expression again if the date range changes
  • [job] [logs] Jest Tests #11 / workspace_panel should run the expression again if the filters change
  • [job] [logs] Jest Tests #11 / workspace_panel should show an error message if the expression fails to parse
  • [job] [logs] Jest Tests #11 / workspace_panel should show an error message if there are missing indexpatterns in the visualization
  • [job] [logs] Jest Tests #11 / workspace_panel should show an error message if validation on both datasource and visualization do not pass
  • [job] [logs] Jest Tests #11 / workspace_panel should show an error message if validation on datasource does not pass
  • [job] [logs] Jest Tests #11 / workspace_panel should show an error message if validation on visualization does not pass
  • [job] [logs] Jest Tests #11 / workspace_panel should show proper workspace prompts when auto-apply disabled

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
lens 1.3MB 1.3MB +1.8KB

History

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

@dej611
Copy link
Contributor Author

dej611 commented Jan 17, 2023

Closing in favour of #149025

@dej611 dej611 closed this Jan 17, 2023
dej611 added a commit that referenced this pull request Jan 18, 2023
… fields (#149025)

## Summary

Fixes #148613 

After investigating the issue via a different approach #149011 I've
decided to handle it in this minimal way.
This PR will automatically switch the `show_array` toggle for non
numeric fields avoiding as much as possible direct appearance of the
error to the user.
If the user decides to explicitly disable the toggle, then the runtime
error will be shown (the message there can still be improved).

### Checklist

Delete any items that are not applicable to this PR.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [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
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)


### Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to
identify risks that should be tested prior to the change/feature
release.

When forming the risk matrix, consider some of the following examples
and how they may potentially impact the change:

| Risk | Probability | Severity | Mitigation/Notes |

|---------------------------|-------------|----------|-------------------------|
| Multiple Spaces—unexpected behavior in non-default Kibana Space.
| Low | High | Integration tests will verify that all features are still
supported in non-default Kibana Space and when user switches between
spaces. |
| Multiple nodes—Elasticsearch polling might have race conditions
when multiple Kibana nodes are polling for the same tasks. | High | Low
| Tasks are idempotent, so executing them multiple times will not result
in logical error, but will degrade performance. To test for this case we
add plenty of unit tests around this logic and document manual testing
procedure. |
| Code should gracefully handle cases when feature X or plugin Y are
disabled. | Medium | High | Unit tests will verify that any feature flag
or plugin combination still results in our service operational. |
| [See more potential risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) |


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Runtime fields returning array returns a Request error when using last_value operation
2 participants