-
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
Fix KQL value suggestions for nested fields #54820
Conversation
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:
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):
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 |
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. |
This reverts commit afcaa34
@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. |
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.
Tested on Chrome Linux, works as expected. Will create a followup PR disabled nested fields in more apps/places for selection.
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* 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 <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
* 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 <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
* 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 <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Matt Bargar <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
* 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 <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Matt Bargar <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
* 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) ...
* 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 ...
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
strikethroughsto 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- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately