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

Fix KQL value suggestions for nested fields #54820

Merged
merged 8 commits into from
Jan 18, 2020

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Jan 14, 2020

Summary

Fixes #54642

In #51774 I flagged nested fields as non-aggregatable in the index pattern so that they would not show up in the Visualize editor until we have implemented nested field support for aggregations. This had the unintended effect of turning off value suggestions for nested fields in KQL because we don't retrieve suggestions for non-aggregatable fields. So in hindsight marking nested fields as non-aggregatable was too broad of a change. I've reverted that change and instead I'm now filtering out nested fields from Visualize at the agg param level.

I really think we should get this into 7.6 because if we don't, users will have to manually refresh their index patterns in 7.7 to fix the value suggestion bug.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [] This was checked for cross-browser compatibility, including a check against IE11
- [ ] 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

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately

@Bargs Bargs added Feature:KQL KQL v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Jan 14, 2020
@Bargs Bargs requested a review from timroes January 14, 2020 21:26
@Bargs Bargs requested a review from a team as a code owner January 14, 2020 21:26
@timroes
Copy link
Contributor

timroes commented Jan 15, 2020

While I agree that it's stupid, if the user need to refresh their index pattern in general, if we implement aggregations on nested fields, we are lacking a couple of places here where we manually need to exclude nested fields for now:

  • Lens
  • TSVB
  • Maps
  • Canvas
  • ???

I am not sure if there are more then that, but currently couldn't come up with any other.

Started to look into Lens, and there are still a couple of type mismatches, while not every of this places might be trivial to fix. So we basically have a couple of options now (and none makes me really happy :D):

  1. We mark it as aggregatable right now (as this PR does), and try to limit down all the places manually that would consume the aggregatable flag but don't support nested fields yet. Might cause more work than we expect right now, and I am not 100% certain we'll think about all the places.
  2. We keep it as unaggregatable in the index pattern, and just adjust the KQL logic to ignore the flag for nested fields manually, and load auto completion anyway. Means we need a special hack in the auto completion provider + the issue, that we'll require users with nested fields to refresh their index pattern, once we support nested fields in aggregations.
  3. We just ignore autocompletion on nested field values for now and take it as a limitation we know. Will not require any dirty hacks in the auto completion provider, but will have the same issue still, users will need to refresh their index pattern once we support aggregating over nested fields.

None of those solutions make me really happy. From a user perspective I think (1) would be the best, but I am a bit afraid given the amount of changes we still need to make to different apps for this, given our release time line.

Update: During fixing some of those places it seems that not only switching the aggregatable flag for the child nodes is causing issues in some apps, but also that the fields are marked as sortable, and that the nested type fields exist at all make them selectable in a range of places (and then fail).

@Bargs
Copy link
Contributor Author

Bargs commented Jan 15, 2020

Yeah I agree none of those are ideal. One thing to consider is that prior to #51774 we displayed nested fields in all these unsupported places for years, probably back to 4.x at least. So it's an annoyance, but probably one most nested field users are already used to. Nested field support in KQL is new, so shipping a bug with it that makes it look slightly broken may leave a bad initial impression.

So to add to your list of options, we could also just revert #51774 and leave things as they have been.

Actually, as I was typing this, I also realized that when we add nested support to Visualize we will need to mark these fields as aggregatable, and then we will start showing nested fields in all the other unsupported places again, so I think marking them as unaggregatable now is not a good idea.

@Bargs
Copy link
Contributor Author

Bargs commented Jan 15, 2020

@timroes I reverted your last TSVB commit so it can be made in a separate PR like we talked about. Let me know if there's anything else you think we need in this PR before merging.

@Bargs Bargs added the v7.7.0 label Jan 15, 2020
Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Tested on Chrome Linux, works as expected. Will create a followup PR disabled nested fields in more apps/places for selection.

@timroes
Copy link
Contributor

timroes commented Jan 18, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@timroes timroes merged commit 8ae51ab into elastic:master Jan 18, 2020
timroes added a commit to timroes/kibana that referenced this pull request Jan 18, 2020
* Revert "Flag nested fields as non-aggregatable (elastic#51774)"

This reverts commit c7046a0

* Filter out nested fields at the agg param level

* Forbid nested fields in TSVB

* Revert "Forbid nested fields in TSVB"

This reverts commit afcaa34

Co-authored-by: Tim Roes <mail@timroes.de>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
timroes added a commit to timroes/kibana that referenced this pull request Jan 18, 2020
* Revert "Flag nested fields as non-aggregatable (elastic#51774)"

This reverts commit c7046a0

* Filter out nested fields at the agg param level

* Forbid nested fields in TSVB

* Revert "Forbid nested fields in TSVB"

This reverts commit afcaa34

Co-authored-by: Tim Roes <mail@timroes.de>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
timroes pushed a commit that referenced this pull request Jan 18, 2020
* Revert "Flag nested fields as non-aggregatable (#51774)"

This reverts commit c7046a0

* Filter out nested fields at the agg param level

* Forbid nested fields in TSVB

* Revert "Forbid nested fields in TSVB"

This reverts commit afcaa34

Co-authored-by: Tim Roes <mail@timroes.de>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Matt Bargar <mbargar@gmail.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
timroes pushed a commit that referenced this pull request Jan 18, 2020
* Revert "Flag nested fields as non-aggregatable (#51774)"

This reverts commit c7046a0

* Filter out nested fields at the agg param level

* Forbid nested fields in TSVB

* Revert "Forbid nested fields in TSVB"

This reverts commit afcaa34

Co-authored-by: Tim Roes <mail@timroes.de>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Matt Bargar <mbargar@gmail.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 20, 2020
* upstream/master: (24 commits)
  Show error page when accessing unavailable app (elastic#54656)
  [ML] Improving job wizards with datafeed aggregations (elastic#55180)
  remove flaly assetion. a license presence tested anyway (elastic#55289)
  fix commonly used ranges uptime (elastic#54930)
  [SIEM] Use proper icons on Detections view (elastic#55215)
  Fix: invalid translation referenced (elastic#54901)
  [State Management] Remove AppState from edit_index_pattern page (elastic#54104)
  Implements `getStartServices` on server-side (elastic#55156)
  Move vis_vega_type/data_model tests to jest (elastic#55186)
  [SIEM] [Detection Engine] Update status on rule details page (elastic#55201)
  Fix KQL value suggestions for nested fields (elastic#54820)
  Enforce camelCase format for a plugin id (elastic#53759)
  [SIEM] Detection engine cleanup for rule details/creation/edit page (elastic#55069)
  Remove nested root from index pattern (elastic#54978)
  [Reporting/Migration] ReportingSetup, LegacySetup (elastic#54198)
  [SIEM] [Detection Engine] Fixes duplicate rule action (elastic#55252)
  [SIEM] Detections add alert & signal tab (elastic#55127)
  Management API - redirect on disabled app path (elastic#55136)
  [SIEM][Detection Engine] Fixes critical regression on the backend with immutable and tags
  update local (elastic#55177)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 20, 2020
* master: (108 commits)
  [ML] Single Metric Viewer: Fix job check. (elastic#55191)
  Show error page when accessing unavailable app (elastic#54656)
  [ML] Improving job wizards with datafeed aggregations (elastic#55180)
  remove flaly assetion. a license presence tested anyway (elastic#55289)
  fix commonly used ranges uptime (elastic#54930)
  [SIEM] Use proper icons on Detections view (elastic#55215)
  Fix: invalid translation referenced (elastic#54901)
  [State Management] Remove AppState from edit_index_pattern page (elastic#54104)
  Implements `getStartServices` on server-side (elastic#55156)
  Move vis_vega_type/data_model tests to jest (elastic#55186)
  [SIEM] [Detection Engine] Update status on rule details page (elastic#55201)
  Fix KQL value suggestions for nested fields (elastic#54820)
  Enforce camelCase format for a plugin id (elastic#53759)
  [SIEM] Detection engine cleanup for rule details/creation/edit page (elastic#55069)
  Remove nested root from index pattern (elastic#54978)
  [Reporting/Migration] ReportingSetup, LegacySetup (elastic#54198)
  [SIEM] [Detection Engine] Fixes duplicate rule action (elastic#55252)
  [SIEM] Detections add alert & signal tab (elastic#55127)
  Management API - redirect on disabled app path (elastic#55136)
  [SIEM][Detection Engine] Fixes critical regression on the backend with immutable and tags
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:KQL KQL release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nested fields autocomplete values doesn't appear
4 participants