Skip to content

Conversation

@walterra
Copy link
Contributor

Summary

Fixes applying saved searches in the transform wizard. Previously, upon initializing the transform wizard's state, we would miss passing on the initialized data from kibanaContext. The resulting bug was that saved search were not applied in the generated transform config and source preview table.

  • Introduces useKibanaContext custom hook, in combination with the RenderOnlyWithInitializedKibanaContext component we don't need to check in components further down if kibanaContext was already initialized. We already have a similar useKibanaContext in plugins/ml but plugins/transform was missing a similar custom hook.
  • Removes some unused commented code
  • Adds functional tests which create a transform based on a saved search.

Checklist

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

For maintainers

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@peteharverson
Copy link
Contributor

The saved search now seems to be getting applied in the wizard, but it looks to me like it isn't used in the preview table in the list page. Here my saved search matches against 12 instances from two regions, but the preview table lists all instances across all regions:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Typo - should refer to the avg(responsetime) agg I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in a2fc19a.

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Great stuff 🎉
Left a couple suggestions around the tests.

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Also, I've noticed that the preview in the row expansion is still not taking the saved search into account, e.g. this is filtered by airline:A*:
image

However, the actual data in the destination index is correct.

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Failed test: Error: expected 'Ahmed Al,Eddie,Robert' to equal 'Ahmed Al,Kamal,Robert'
It seems that the source preview doesn't load the same data each time. Probably because we're not sorting in the ES query and in that case ES doesn't give guarantees on the order.

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

Code LGTM once tests have been updated and typo fixed 👌

@walterra
Copy link
Contributor Author

@peteharverson I fixed the filtered preview in the transform list in 8c54cdb.

@walterra
Copy link
Contributor Author

@pheyos Addressed your comments. I changed the source table assertion for the index based tests to only check for the number of rows and columns. The preview on the management page should now be filtered correctly too.

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

LGTM
I think the validation for number of columns and rows could be also useful (in addition to the existing validations) for
a) the pivot preview table
b) the saved search transforms
but this can be done in a follow-up.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@walterra
Copy link
Contributor Author

retest

@walterra
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@walterra walterra force-pushed the ml-fix-transform-saved-search branch from d478e38 to 3a47245 Compare November 23, 2019 20:17
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@walterra walterra merged commit b5133b5 into elastic:master Nov 24, 2019
@walterra walterra deleted the ml-fix-transform-saved-search branch November 24, 2019 07:36
walterra added a commit to walterra/kibana that referenced this pull request Nov 24, 2019
Fixes applying saved searches in the transform wizard. Previously, upon initializing the transform wizard's state, we would miss passing on the initialized data from kibanaContext. The resulting bug was that saved search were not applied in the generated transform config and source preview table.
walterra added a commit that referenced this pull request Nov 24, 2019
…1555)

Fixes applying saved searches in the transform wizard. Previously, upon initializing the transform wizard's state, we would miss passing on the initialized data from kibanaContext. The resulting bug was that saved search were not applied in the generated transform config and source preview table.
walterra added a commit to walterra/kibana that referenced this pull request Dec 4, 2019
Fixes applying saved searches in the transform wizard. Previously, upon initializing the transform wizard's state, we would miss passing on the initialized data from kibanaContext. The resulting bug was that saved search were not applied in the generated transform config and source preview table.
peteharverson pushed a commit that referenced this pull request Dec 5, 2019
… (#52210)

* [ML] Transform: Fix use of saved search in pivot wizard. (#51079)

Fixes applying saved searches in the transform wizard. Previously, upon initializing the transform wizard's state, we would miss passing on the initialized data from kibanaContext. The resulting bug was that saved search were not applied in the generated transform config and source preview table.

* [ML] Fix search for Transforms and Analytics tables (#52163)

* [ML] fix TransformTable init

* [ML] fix Analytics table

* [ML] Fix table imports.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants