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

[Vis Default editor] Fix issue with Rollup #42430

Merged
merged 10 commits into from
Aug 13, 2019

Conversation

maryia-lapata
Copy link
Contributor

@maryia-lapata maryia-lapata commented Aug 1, 2019

Summary

  1. There is an issue with Rollup Index pattern. When Date Histogram agg is selected, the code is broken and Buckets group disappeared:

Aug-01-2019 12-11-06

  1. I found one more issue. Rollup Index pattern -> Date Histogram -> interval control should be set in 1m (default value from Editor Config of Rollup), but it's set in auto (due to the hook is launched before editorConfig is changed)

What was done:

  • Reverted fieldsFetcher.fetch to fieldsFetcher.fetchForWildcard for displaying correct Time Filter field name during Rollup Index Pattern creation (instead of timestamp.date_histogram.timestamp):

  • Added a condition for the effect which sets default params from editor config (it resolves 2).

  • When getInterval is invoked with undefined value (e.g. when a user cleans the interval field), for Rollup Index Pattern esInterval is calculated based on original value (undefined) rather than default value (auto) as for simple Index pattern. It causes an exeption in parseEsInterval. That's why now makeLabel is called safety.

With the changes:

Aug-09-2019 14-41-54

Checklist

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

For maintainers

@maryia-lapata maryia-lapata added the release_note:skip Skip the PR/issue when compiling release notes label Aug 1, 2019
@maryia-lapata maryia-lapata requested a review from timroes August 1, 2019 09:17
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@maryia-lapata maryia-lapata requested a review from sulemanof August 1, 2019 14:21
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@maryia-lapata
Copy link
Contributor Author

After some debugging I found out that the root cause is deeper.
What we see: The method makeLabel breaks with the error 'field' is required.
The reason: The field should be set to a default value when the agg type is changed. It occurs in agg_config.js. The default value for Date Histogram field is agg.getIndexPattern().timeFieldName (for Rollup index pattern it equals timestamp.date_histogram.timestamp). And then in deserialize method of Field param it tries to get a field object by field name (timestamp.date_histogram.timestamp), but the index pattern doesn't have a field with such name, it has just timestamp field name.

image

This undefined field causes throwing an error. A default value for field params as well as for other params were not set, which in turn produces the error during getting agg description via makeLabel() call.

Since Rollup index pattern has composite timeFieldName it seems that it should be a different way for getting a field object by timeFieldName for Rollup index pattern.

@jen-huang if you know could you please explain how timeFieldName is formed and why it differs from simple index pattern?

@maryia-lapata maryia-lapata added v7.4.0 v8.0.0 WIP Work in progress labels Aug 5, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jen-huang
Copy link
Contributor

jen-huang commented Aug 7, 2019

@maryia-lapata Rollup documents are special in that they are not "real" documents, the field that you are seeing (timestamp.date_histogram.timestamp) will eventually be mapped to the original time field when the rollup indices are queried by rollup search.

I am seeing an issue when it comes to creating rollup index pattern:
image

The fields here should already be timestamp, not timestamp.date_histogram.timestamp, I think something got lost with typescript conversion. This might be why you are seeing the same field in visualizations. I'm investigating further..

@jen-huang
Copy link
Contributor

This is the culprit for the rollup index pattern:
ba8a453#diff-d1e2b5c0839b8d02c31d0270350e9387R92

@ppisljar Do you recall why this line was changed from fieldsFetcher.fetchForWildcard to fieldsFetcher.fetch? Is it possible to change this back?

@maryia-lapata I tested replacing that line with:

indexPattern.fieldsFetcher.fetchForWildcard(pattern, getFetchForWildcardOptions())

and the time field is now displaying correctly in visualizations and displaying results:

image

Could you try adding that change to this fix and see if that unblocks you?

@maryia-lapata
Copy link
Contributor Author

@jen-huang thank you so much!

I added the fix for Time Filter field name and updated the PR description.

@ppisljar @timroes @jen-huang I'd very appreciate it if you review these changes.

@maryia-lapata maryia-lapata requested a review from ppisljar August 9, 2019 12:19
@elasticmachine

This comment has been minimized.

@maryia-lapata maryia-lapata added Feature:Vis Editor Visualization editor issues Team:Visualizations Visualization editors, elastic-charts and infrastructure and removed WIP Work in progress labels Aug 9, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@ppisljar
Copy link
Member

i think this was changed by mistake from fetchForWildcard to fetch, while updating the method (before it existed on index pattern service and now its on index pattern and index pattern does not need to be passed in)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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. Seems to fix all described issues and Code LGTM

Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

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

Tested locally with a rollup pattern with a time field, and one without a time field selected. LGTM!

@maryia-lapata maryia-lapata merged commit 2d81859 into elastic:master Aug 13, 2019
@maryia-lapata maryia-lapata deleted the issue-agg-params-rollup branch August 13, 2019 07:51
maryia-lapata added a commit to maryia-lapata/kibana that referenced this pull request Aug 13, 2019
* Fix issue with Rollup

* Use useMemo for editorConfig

* Wrap makeLabel with try catch

* Revert fetch to fetchForWildcard

* Update useEffect dependencies
maryia-lapata added a commit that referenced this pull request Aug 13, 2019
* Fix issue with Rollup

* Use useMemo for editorConfig

* Wrap makeLabel with try catch

* Revert fetch to fetchForWildcard

* Update useEffect dependencies
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 13, 2019
…p-metrics-selectall

* 'master' of github.com:elastic/kibana: (27 commits)
  [ML] Data Frames: Analytics job creation. (elastic#43102)
  [Vis Default editor] Fix issue with Rollup (elastic#42430)
  [Vis: Default editor] EUIficate Markdown tab (elastic#42677)
  [New Platform Migration Phase I]: update dateHisogramInterval & parseEsInterval imports (elastic#42917)
  [Infra UI] Add AWS metrics to node detail page (elastic#42153)
  update apm index pattern (elastic#43106)
  [SIEM] Toggle Column / Code Coverage and Cypress (elastic#42766)
  skip failing test (elastic#43163)
  [code] Add option to turn the go dependency download on/off. (elastic#43096)
  disable visual regression jobs
  Removed dead code (elastic#42774)
  fixes csv export of saved searches that have _source field (elastic#43123)
  Export missing Context types (elastic#43051)
  Update dependency supports-color to v7 (elastic#43064)
  switch to icon type string instead of node (elastic#43111)
  [Maps] Enable borders for icon symbols (elastic#43066)
  [ftr] enable visualRegression jobs (elastic#42989)
  [ML] Converting single to multi metric job (elastic#42532)
  fix(NA): dont clean dll module if it is a package json file (elastic#42904)
  [Logs UI] Add link from the sample web logs to the Logs UI (elastic#42635)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:Vis Editor Visualization editor issues release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.4.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants