-
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
Added hidden filter to data streams tab #85028
Conversation
@elasticmachine merge upstream |
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
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.
Nice work @yuliacech! This is looking great. I left a few comments in the code. I think we might be able to make the api integration tests a little more robust; let me know what you think.
I'd also like to test the deep-linking before I approve. Can you remind me on how/where to test that? I checked the index templates UI, but from what I could see we are only indicating if there is an associated data stream and not actually linking to the data stream.
I also had a few UX questions/suggestions (some of which might be better suited for a separate PR):
- I have trouble understanding the difference between
managed
,system
andhidden
😄. Shouldhidden
actually bemanaged
, or is it a different behavior for data streams? For example, the ILM index template is actually marked asmanaged
in the UI, but the associated data stream ishidden
. - Could we also add the badge to the details panel of the data stream? You can check out index templates as an example.
- For
system
templates, we add an additional warning when deleting. Would this be applicable to hidden data streams?
@@ -6,10 +6,34 @@ | |||
|
|||
import { DataStream } from '../../../common'; | |||
|
|||
export const isManagedByIngestManager = (dataStream: DataStream): boolean => { | |||
export const isFleetManaged = (dataStream: DataStream): boolean => { | |||
// TODO check if the wording will change to 'fleet' |
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.
Were you planning on addressing this TODO as part of this PR?
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.
I'll remove this as soon as I get info from the Fleet team :)
dataStream: DataStream; | ||
} | ||
|
||
export const DataStreamsBadges: React.FunctionComponent<Props> = ({ dataStream }) => { |
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.
Is it possible that a fleet-managed data stream could also be hidden? Are there any UX concerns with displaying two badges side-by-side in a table row?
@@ -65,12 +65,24 @@ const enhanceDataStreams = ({ | |||
}); | |||
}; | |||
|
|||
const getDataStreams = (client: ElasticsearchClient, name = '') => { | |||
// TODO update when elasticsearch client has update requestParams for 'indices.getDataStream' |
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.
Is there an open issue that we can point to as part of this comment?
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.
There is no issue as elasticsearch-js client generates their typings for API automatically. I think we just need to wait for the next client update, so I left this comment. But let me know what you think :)
@@ -65,12 +65,24 @@ const enhanceDataStreams = ({ | |||
}); | |||
}; | |||
|
|||
const getDataStreams = (client: ElasticsearchClient, name = '') => { |
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.
Should we just not include name
in the path if it isn't defined?
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.
I think the code would not be much easier with a ternary operator for path
, so I'd prefer to have a default value. Or did you mean something else?
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.
My main concern is the path ends up resolving to /_data_stream//
is there is no name specified
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.
Thanks for bringing this up, @alisonelizabeth ! I haven't thought about it and that's a valid concern. I changed the empty string to a wildcard *
and deleted the closing forward slash. So if name isn't defined, the path would be /_data_stream/*
.
@@ -61,20 +61,44 @@ export default function ({ getService }: FtrProviderContext) { | |||
|
|||
describe('Data streams', function () { | |||
describe('Get', () => { | |||
const defaultHiddenDataStream = { |
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.
This seems a little brittle to provide values for the data stream generated by ES. For example, are we confident that the name will always beilm-history-5
?
@@ -101,11 +126,13 @@ export default function ({ getService }: FtrProviderContext) { | |||
.expect(200); | |||
|
|||
// ES determines these values so we'll just echo them back. | |||
const { name: indexName, uuid } = dataStreams[0].indices[0]; | |||
const { storageSize, ...dataStreamWithoutStorageSize } = dataStreams[0]; | |||
// dataStreams[0] is a hidden data stream created by ES |
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.
I wonder if the test would be more resilient if we used find
to find the datastream created in the test. I have concerns about the current implementation if ES were to add more hidden data streams. WDYT?
@elasticmachine merge upstream |
Hi @alisonelizabeth , thank you so much for your review! I addressed the feedback, would you mind having another look?
|
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.
Thanks for addressing my feedback @yuliacech! I think the CITs need to be updated to react to your most recent changes, but otherwise LGTM. Tested again and verified the cross-linking.
Also, the badge on the details panel is misaligned for me - possibly caused by using the EuiBadgeGroup
?
@@ -65,12 +65,24 @@ const enhanceDataStreams = ({ | |||
}); | |||
}; | |||
|
|||
const getDataStreams = (client: ElasticsearchClient, name = '') => { |
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.
My main concern is the path ends up resolving to /_data_stream//
is there is no name specified
👍
I'm not as familiar with this area, but if they are always managed by Fleet I think it would be helpful to update the label to be more specific. As a future enhancement, I think it could also be helpful to provide a tooltip or something to each badge that gives a better explanation of what it means. |
@elasticmachine merge upstream |
Thanks for re-reviewing @alisonelizabeth ! I fixed the alignment issue in the detail panel :) Will create a separate PR for wording of Fleet-managed data streams and a possible tooltip for the label. |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Distributable file count
Page load bundle
History
To update your PR or re-run it, just comment with: |
* Added hidden filter to data streams tab * Fix i18n import * Fixed tests * hidden ds pr feedback * Added includeHidden query to data streams list * Changed how badge group renders data streams badges Co-authored-by: Kibana Machine <[email protected]>
* Added hidden filter to data streams tab * Fix i18n import * Fixed tests * hidden ds pr feedback * Added includeHidden query to data streams list * Changed how badge group renders data streams badges Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
Fixes #83709.
This PR adds support for hidden data streams in Index Management app. A new view button allows user to toggle several filters for data streams. Currently, there are 'Fleet-managed data streams' and 'hidden data streams'. Fleet-managed data streams are enabled by default, hidden data streams are disabled by default.
The code reuses view filter button created for index template list.
Screenshots
There are some data streams, but all are filtered out: table is displayed with no rows.
Hidden data streams with a label
When there are no data streams at all, the empty prompt is shown. This UI has not changed
How to test
A hidden data stream is usually already preexisting
ilm-history-5
. To create more hidden data streams:Checklist
Release note
In Index Management app, hidden data streams are now displayed and indicated by a label.