Skip to content

[Lens] fix do not submit invalid query in filtered metric#107542

Merged
alexwizp merged 10 commits intoelastic:masterfrom
alexwizp:95611
Aug 11, 2021
Merged

[Lens] fix do not submit invalid query in filtered metric#107542
alexwizp merged 10 commits intoelastic:masterfrom
alexwizp:95611

Conversation

@alexwizp
Copy link
Copy Markdown
Contributor

@alexwizp alexwizp commented Aug 3, 2021 •
edited
Loading

Closes: #95611

Summary

Issue description

If the query of a filtered metric is invalid, it's still attempting to render the chart which is causing a runtime error:
Screenshot 2021-03-27 at 14 33 38

This should be improved. In case of the filters function an invalid query is implicitly turned into a * query matching all documents - we could do the same thing here, but maybe we should do something more sensible in both cases like not submitting the query as long as the input is open and on close failing the validation and highlighting the problematic dimension.

Implementation notes

  • in case of invalid query is implicitly turned into a * query matching all documents
  • we do not store the wrong value and the user sees (empty) instead
  • an error message has been added when entering an invalid query

Screens

Screen.Recording.2021-08-04.at.2.47.15.PM.mov

@alexwizp alexwizp changed the title [Lens] Do not submit invalid query in filtered metric [Lens] fix Do not submit invalid query in filtered metric Aug 4, 2021
@alexwizp alexwizp changed the title [Lens] fix Do not submit invalid query in filtered metric [Lens] fix do not submit invalid query in filtered metric Aug 4, 2021
@elastic elastic deleted a comment from kibanamachine Aug 4, 2021
@alexwizp alexwizp self-assigned this Aug 4, 2021
@alexwizp alexwizp added v7.15.0 v8.0.0 Feature:Lens Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// release_note:fix labels Aug 4, 2021
@alexwizp alexwizp marked this pull request as ready for review August 4, 2021 16:04
@alexwizp alexwizp requested a review from a team August 4, 2021 16:04
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Aug 4, 2021

Pinging @elastic/kibana-app (Team:KibanaApp)

dej611
dej611 reviewed Aug 4, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Overall it seems ok.
Would it be possible to leave the last "valid" state on the filter entry if available?

In this case I'm passing from a valid query to (empty) while the popup is still open.

filter_invalid

I think it's ok to clear the filter out if the user closes the popup/dimension panel.

BTW it's a minor enhancement here, not super important if too hard.

@alexwizp
Copy link
Copy Markdown
Contributor Author

alexwizp commented Aug 4, 2021

Would it be possible to leave the last "valid" state on the filter entry if available?

@dej611 np, done

@alexwizp alexwizp requested a review from dej611 August 5, 2021 10:22
@alexwizp
Copy link
Copy Markdown
Contributor Author

alexwizp commented Aug 5, 2021

@elasticmachine merge upstream

@alexwizp
Copy link
Copy Markdown
Contributor Author

alexwizp commented Aug 9, 2021

@elasticmachine merge upstream

@alexwizp alexwizp requested a review from VladLasitsa August 9, 2021 09:13
VladLasitsa
VladLasitsa approved these changes Aug 9, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@VladLasitsa VladLasitsa left a comment

Choose a reason for hiding this comment

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

LGTM, tested locally!

@mbondyra
Copy link
Copy Markdown
Contributor

mbondyra commented Aug 10, 2021 •
edited
Loading

This is definitely an improvement. The issue also mentions and on close failing the validation and highlighting the problematic dimension. Something like validation for formula:

Screenshot 2021-08-10 at 09 27 23

Haven't tried it so not sure if it's possible, but maybe you could extend the getErrorMessage for operations that are filterable and have incorrect filter?

It would be also nice to highlight the element in the dimension flyout, but still, unsure how complicated it is:
Screenshot 2021-08-10 at 09 34 07

@alexwizp
Copy link
Copy Markdown
Contributor Author

alexwizp commented Aug 10, 2021

@mbondyra good idea, thank you. I didn't know about that validation

Screen.Recording.2021-08-10.at.12.00.27.PM.mov

@mbondyra
Copy link
Copy Markdown
Contributor

mbondyra commented Aug 10, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Aug 10, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 739 803 +64

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.6MB 1.6MB +79.2KB
Unknown metric groups

References to deprecated APIs

id before after diff
lens 144 139 -5

History

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

cc @alexwizp

mbondyra
mbondyra approved these changes Aug 10, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

Works great, thanks a lot for the improvement with passing the invalid state from saved object 💯

@alexwizp alexwizp merged commit ee10819 into elastic:master Aug 11, 2021
alexwizp added a commit to alexwizp/kibana that referenced this pull request Aug 11, 2021
@alexwizp @kibanamachine
[Lens] fix do not submit invalid query in filtered metric (elastic#10…
5a96051 
…7542)

* [Lens] Do not submit invalid query in filtered metric

Closes: elastic#95611

* fix CI

* fix PR comments

* fix PR comments

* fix PR comment

* move closePopover to useCallback

* add filter validation to utils/isColumnInvalid

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@alexwizp alexwizp mentioned this pull request Aug 11, 2021
Merged
alexwizp added a commit that referenced this pull request Aug 11, 2021
@alexwizp @kibanamachine
[Lens] fix do not submit invalid query in filtered metric (#107542) (#…
b4b59d9 
…108144)

* [Lens] Do not submit invalid query in filtered metric

Closes: #95611

* fix CI

* fix PR comments

* fix PR comments

* fix PR comment

* move closePopover to useCallback

* add filter validation to utils/isColumnInvalid

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 11, 2021
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into reporting/new…
66273c7 
…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (101 commits)
  [ML] APM Latency Correlations: Field/value candidates prioritization (elastic#107370)
  [Reporting] Add lenience to a test on the order of asserted logs (elastic#108135)
  [Lens] fix do not submit invalid query in filtered metric (elastic#107542)
  skip flaky test (elastic#108043)
  fix newly introduced type error (elastic#107593)
  [Reporting] server side code clean up (elastic#106940)
  [build_ts_refs] improve caches, allow building a subset of projects (elastic#107981)
  [APM] Add new ftr_e2e to kibana CI and remove current e2e tests. (elastic#107593)
  add manage rules link to alerts dropdown (elastic#107950)
  [ML] Enable Index data visualizer document count chart to update time range query (elastic#106438)
  [Security Solutions][Detection Engine] Fixes "undefined" crash for author field by adding a migration for it (elastic#107230)
  [Actions UI] Fixed Jira Api token label. (elastic#107776)
  [Alerting UI] Fixed display permissions for edit/delete buttons when user has read only access. (elastic#107996)
  [Maps] fix code owners (elastic#108106)
  Update EMS landing page url (elastic#108102)
  Do not render page header for loading domains (elastic#108078)
  Update dependency @elastic/charts to v33.2.2 (elastic#107939)
  [APM] Display throughput as tps (instead of tpm) when bucket size < 60 seconds (elastic#107850)
  [Fleet] Fix all category count (elastic#108089)
  [Security Solution][Bug] - Disable alert table RBAC until fields sorted (elastic#108034)
  ...

# Conflicts:
#	x-pack/plugins/reporting/server/export_types/common/generate_png.ts
#	x-pack/plugins/reporting/server/lib/screenshots/index.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mbondyra mbondyra mbondyra approved these changes

@stratoula stratoula Awaiting requested review from stratoula

@dej611 dej611 Awaiting requested review from dej611

+1 more reviewer

@VladLasitsa VladLasitsa VladLasitsa approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@alexwizp alexwizp

Labels

Feature:Lens release_note:enhancement Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.15.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Lens] Do not submit invalid query in filtered metric

6 participants

@alexwizp @elasticmachine @mbondyra @kibanamachine @dej611 @VladLasitsa