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

[Index Management] Add data streams functionality to indices tab #67940

Merged
merged 11 commits into from
Jun 4, 2020

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Jun 2, 2020

Summary

  • Migrate the URL to be the source of truth for the includeHiddenIndices toggle
  • Add a new url param dataStreams that filters out indices based on the data stream they are backing

How to test

  1. Create a data stream in your ES cluster: PUT _data_stream/my-data-stream
  2. Go to Index Management plugin in Kibana (requires x-pack). There should be no indices visible if this is a fresh cluster.
  3. Press the include hidden toggle and all hidden indices should appear including my-data-stream-000001
  4. You should be able to navigate to the url you are on with &filter=dataStream:my-data-stream added to the hash portion of your url. This filters for just the data stream backing index my-data-stream-000001. Something like: /app/kibana#/management/data/index_management/indices?includeHiddenIndices=true&filter=dataStream:my-data-stream

Notes to reviewer

  • A refactor was done to remove the middleware for syncing the URL to the store. State was moved down to the index table where it is derived from the location object we receive from withRouter.
  • The selector getFilteredIndices still does the work of filtering over all indices but now also depends on the component props, specifically the location.search value to determine what all it should be filtering. More information on this here.
  • We can do the same for filter as we did for includeHiddenIndices; move the source of truth to the URL. This will be a slightly bigger refactor because of how it is currently implemented and can be set by other plugins (like ILM). The filter will need to be converted to a string value and parsed into Query by us. I would consider this follow up work, but it would be nice to have the actual text in the filter sync up with the URL as the user types.

Screenshots

Filtering based on hidden
Screenshot 2020-06-02 at 14 23 17

Filtering based on text filter and include hidden
Screenshot 2020-06-02 at 20 52 58

Checklist

For maintainers

- Added the data streams column
- Moved state down to component (using withRouter)
- Removed previous middleware for syncing url hash param
- call history.push to update the url in the component
@jloleysens jloleysens added Feature:Index Management Index and index templates UI v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jun 2, 2020
@jloleysens jloleysens requested a review from cjcenizal June 2, 2020 12:33
@jloleysens jloleysens requested a review from a team as a code owner June 2, 2020 12:33
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@jloleysens jloleysens changed the title Im/data streams column [Index Management] Add data streams functionality to indices tab Jun 2, 2020
@jloleysens
Copy link
Contributor Author

@cjcenizal This is not 100% ready for review as I thought. I just realised I missed something in this implementation, will re-ping when it s ready.

@jloleysens
Copy link
Contributor Author

@cjcenizal Ok, ready now!


if (filterFromURI) {
const decodedFilter = decodeURIComponent(filterFromURI);
this.interval = setInterval(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note for our future selves: this is the code we want 4 (or more!) 👀 on when we get flakiness in our tests 😊

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi! I am the future self 😊 : don't worry, it's okay, there is jest. useFakeTimers() that gives us control over this.

Copy link
Contributor

@cjcenizal cjcenizal 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, code LGTM! Great work, JL! 👏 I found one bug that I think we should fix before merging, and one optimization I think we can address later.

Sort bug

Sorting for the data stream column is incorrect. We can fix this by adding a sorter to the sort_table.ts service.

dataStream: stringSort('dataStream'),

image

Filter optimization

Filter on the name is subject to unexpected behavior because it's possible that one data stream's name could contain another data stream's name. Here's a silly example but it demonstrates the result:

image

This is an edge case so I don't think we should spend too much time right now trying to get this right, but I imagine a solution would entail filtering on the indices' dataStream field.

…ms-column

* 'master' of github.com:elastic/kibana: (63 commits)
  remove scripts. prettire update has been done (elastic#68130)
  Closes elastic#68055 by detecting the local Kibana version and using that as (elastic#68198)
  [apm] docs: add deployment annotation example (elastic#67408)
  [ML] Extend population preview chart to show actual and typical value (elastic#67569)
  Refactor index management client integration tests for scalability (elastic#67917)
  Add generator function that creates multiple alerts (elastic#67713)
  chore(NA): remove config arg from os packages (elastic#67871)
  [Reporting] Move code out of Legacy (elastic#67904)
  [Metrics UI] Add overrides to Snapshot API to support alert previews (elastic#68125)
  [Security] [Cases] Manage timeline UI API (elastic#67719)
  [ENDPOINT][INGEST]Task/endpoint ingest update (elastic#67234)
  Fix code coverage for jest, upload merged reports (elastic#68149)
  Update documentation/examples of deprecated namespaceAgnostic field (elastic#68039)
  [DOCS] Updates Canvas docs with new menus (elastic#66061)
  chore(NA): avoids imports of server or public code into common (elastic#67231)
  [SIEM] Fix GetOneTimeline graphql type (elastic#68137)
  skip flaky suite (elastic#67838)
  [Uptime] Add loading message for monitor list no items (elastic#67378)
  [Ingest Manager] Update indexing strategy docs to use dataset.* (elastic#68068)
  [Ingest Manager] Fix datasource validation for streams without vars (elastic#67950)
  ...

# Conflicts:
#	x-pack/plugins/index_management/__jest__/client_integration/helpers/index.ts
#	x-pack/plugins/index_management/__jest__/client_integration/home.test.ts
#	x-pack/plugins/index_management/__jest__/client_integration/home/index_templates_tab.helpers.ts
@jloleysens
Copy link
Contributor Author

Thanks for the review @cjcenizal !

Sorting for the data stream column is incorrect.

I didn't realise we were specifying this imperatively! TIL :). I thought it better to present the user with data_stream as the key instead of dataStream. This will feel closer to the key conventions of ES.

Filter on the name is subject to unexpected behavior because it's possible that one data stream's name could contain another data stream's name

We should be able to address this by using the appropriate query syntax: data_stream=my-data-stream (this indicates a stricter check than :). (more info here)

The URL would become: /app/management/data/index_management/indices?filter=data_stream=my-data-stream&includeHiddenIndices=true.

This would match only data_stream fields that are strictly equal to the search term.

Since the changes I made are quite minor I'm going to merge once CI is green on this 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@jloleysens jloleysens merged commit c97f5fb into elastic:master Jun 4, 2020
@jloleysens jloleysens deleted the im/data-streams-column branch June 4, 2020 11:37
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 4, 2020
…stic#67940)

* First iteration of data streams in index management

- Added the data streams column
- Moved state down to component (using withRouter)
- Removed previous middleware for syncing url hash param
- call history.push to update the url in the component

* Updated jest tests

* refactor: includeHidden -> includeHiddenIndices

* Fix types

* Fix jest test and remove getting filter param from parent

* Small refactor to read url params in render function

* Clean up old data streams code

* Fix sorting on data stream field in table

* dataStream -> data_stream

* qs > * as qs
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 4, 2020
* master: (26 commits)
  [Console]remove completion for type for filter queries and aggs (elastic#68103)
  [ML] Transforms: Filter aggregation support (elastic#67591)
  [ES UI Shared] Monaco XJSON (elastic#67485)
  [Index Management] Add data streams functionality to indices tab (elastic#67940)
  [Discover] Fix renaming of saved search not displayed in breadcrumb (elastic#67577)
  [SECURITY] Rename siem plugin to security_solution (elastic#67902)
  [Uptime] Fix Telemetry Api flaky test (elastic#67358)
  [Data plugin] Add configuration property to enable / disable autocomplete (elastic#67847)
  remove scripts. prettire update has been done (elastic#68130)
  Closes elastic#68055 by detecting the local Kibana version and using that as (elastic#68198)
  [apm] docs: add deployment annotation example (elastic#67408)
  [ML] Extend population preview chart to show actual and typical value (elastic#67569)
  Refactor index management client integration tests for scalability (elastic#67917)
  Add generator function that creates multiple alerts (elastic#67713)
  chore(NA): remove config arg from os packages (elastic#67871)
  [Reporting] Move code out of Legacy (elastic#67904)
  [Metrics UI] Add overrides to Snapshot API to support alert previews (elastic#68125)
  [Security] [Cases] Manage timeline UI API (elastic#67719)
  [ENDPOINT][INGEST]Task/endpoint ingest update (elastic#67234)
  Fix code coverage for jest, upload merged reports (elastic#68149)
  ...
jloleysens added a commit that referenced this pull request Jun 4, 2020
) (#68233)

* First iteration of data streams in index management

- Added the data streams column
- Moved state down to component (using withRouter)
- Removed previous middleware for syncing url hash param
- call history.push to update the url in the component

* Updated jest tests

* refactor: includeHidden -> includeHiddenIndices

* Fix types

* Fix jest test and remove getting filter param from parent

* Small refactor to read url params in render function

* Clean up old data streams code

* Fix sorting on data stream field in table

* dataStream -> data_stream

* qs > * as qs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Index Management Index and index templates UI release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants