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] Support index pattern runtime fields in existence and field stats API #90600

Merged
merged 12 commits into from
Feb 18, 2021

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Feb 8, 2021

Fixes #90175 This PR reads the index pattern and includes index pattern level runtime fields for the requests of field existence and field stats API

To test:

  1. Download and import index pattern including runtime field definition index-pattern-so.ndjson.txt
  2. Go to Lens and select imported index pattern
  3. Runtime field what_a_fielding should work correctly

To simplify a little, this PR also changes the field stats API to include the index pattern id and field name instead of the index pattern title and field definition (as these things can be looked up on the server side). I don't have a strong opinion on this, but it looks like a simpler API to me (which probably wasn't possible in earlier versions because index patterns were not available on the server)

@flash1293 flash1293 added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.12.0 labels Feb 8, 2021
@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293 flash1293 marked this pull request as ready for review February 10, 2021 10:28
@flash1293 flash1293 requested a review from a team February 10, 2021 10:28
@elasticmachine
Copy link
Contributor

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

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

track_total_hits: true,
body: {
query,
aggs,
runtime_mappings:
!field.isMapped && field.runtimeField ? { [fieldName]: field.runtimeField } : {},
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not 100% sure if this is the "correct" behavior for runtime fields shadowing mapped fields. @mattkime Will runtime fields shadowing mapped fields report as isMapped: true and runtimeField: true (that was my understanding so far). If it's the case we should not check on isMapped here and always write the script if runtimeField === true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point Tim, fixed this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tim's understanding is correct.

Copy link
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.

Tested with Chrome and Safari, but I am a bit lost on what the expected result of this PR.

The only difference I noticed with master is this + button feature enabled:

Screenshot 2021-02-15 at 12 38 16

The error notification is still showing up and the data popover is still empty (or in general it does not show any extra information).

Screenshot 2021-02-15 at 12 40 35

Edit: also with this PR everytime I open the data popover I see this error on the server (before there was none):

server   error  [12:49:52.958]  Error: Internal Server Error
    at HapiResponseAdapter.toError (~/kibana/src/core/server/http/router/response_adapter.ts:122:19)
    at HapiResponseAdapter.toHapiResponse (~/kibana/src/core/server/http/router/response_adapter.ts:72:19)
    at HapiResponseAdapter.handle (~/kibana/src/core/server/http/router/response_adapter.ts:67:17)
    at Router.handle (~/kibana/src/core/server/http/router/router.ts:273:34)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at handler (~/kibana/src/core/server/http/router/router.ts:227:11)
    at exports.Manager.execute (~/kibana/node_modules/@hapi/hapi/lib/toolkit.js:60:28)
    at Object.internals.handler (~/kibana/node_modules/@hapi/hapi/lib/handler.js:46:20)
    at exports.execute (~/kibana/node_modules/@hapi/hapi/lib/handler.js:31:20)
    at Request._lifecycle (~/kibana/node_modules/@hapi/hapi/lib/request.js:370:32)
    at Request._execute (~/kibana/node_modules/@hapi/hapi/lib/request.js:279:9)

Copy link
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.

Tested in Chrome, the described test case works as expected. Code looks good to me.

@dej611
Copy link
Contributor

dej611 commented Feb 15, 2021

The error notification is still showing up and the data popover is still empty (or in general it does not show any extra information).

Screenshot 2021-02-15 at 12 40 35

Edit: also with this PR everytime I open the data popover I see this error on the server (before there was none):

server   error  [12:49:52.958]  Error: Internal Server Error
    at HapiResponseAdapter.toError (~/kibana/src/core/server/http/router/response_adapter.ts:122:19)
    at HapiResponseAdapter.toHapiResponse (~/kibana/src/core/server/http/router/response_adapter.ts:72:19)
    at HapiResponseAdapter.handle (~/kibana/src/core/server/http/router/response_adapter.ts:67:17)
    at Router.handle (~/kibana/src/core/server/http/router/router.ts:273:34)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at handler (~/kibana/src/core/server/http/router/router.ts:227:11)
    at exports.Manager.execute (~/kibana/node_modules/@hapi/hapi/lib/toolkit.js:60:28)
    at Object.internals.handler (~/kibana/node_modules/@hapi/hapi/lib/handler.js:46:20)
    at exports.execute (~/kibana/node_modules/@hapi/hapi/lib/handler.js:31:20)
    at Request._lifecycle (~/kibana/node_modules/@hapi/hapi/lib/request.js:370:32)
    at Request._execute (~/kibana/node_modules/@hapi/hapi/lib/request.js:279:9)

Installed the ecommerce dataset and all is fixed now.

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

These changes make sense and I tested that they work as expected. It seems like there are some missing pieces that are pretty important:

  1. No functional test
  2. There is still no UI for managing these, I assume this is coming soon?

@@ -32,17 +32,14 @@ export default ({ getService }: FtrProviderContext) => {
describe('field distribution', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would appreciate a functional test for an index pattern runtime field, seems like it's missing

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293 flash1293 added v7.13.0 and removed v7.12.0 labels Feb 17, 2021
@flash1293
Copy link
Contributor Author

I'm going to postpone merging this PR until after feature freeze to reduce some risk as index pattern runtime fields won't land in 7.12 anyway.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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 907.9KB 907.9KB -35.0B

History

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

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 22, 2021
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

1 similar comment
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@flash1293
Copy link
Contributor Author

I'm getting there, kibana machine, I'm trying really hard

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 24, 2021
flash1293 added a commit that referenced this pull request Feb 24, 2021
…eld stats API (#90600) (#91872)

* [Lens] Support index pattern runtime fields in existence and field stats API (#90600)

* fix fixture

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Support index pattern based runtime fields in field existence API
8 participants