-
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
[Ingest] Data streams list page #64134
Conversation
Pinging @elastic/ingest-management (Team:Ingest Management) |
must: [ | ||
{ | ||
exists: { | ||
field: 'fields.stream.namespace', |
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.
That it has a fields
prefix is actually a bug. As soon as elastic/beats#17858 is merged, this must be adjusted. Same for the other fields below.
@michalpristas Seeing your PR is approved, could you get it in so perhaps we can adjust this here before merging?
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.
Will adjust.
}, | ||
package: { | ||
terms: { | ||
field: 'event.module', |
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.
Why is this needed? Sorting? Perhaps prefix query could help here? event.module
is likely to disappear in the future.
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 is to detect the package to display in the Integrations column of the table. I see the same value in service.type
too? What should I use?
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.
None of both :-( This brings up the question: Is it the data that links to the package or does the ingest manager know which package belongs to the data? I would say it is the second one. Lets assume someone configure the agent to ship system metrics. He will not have added any information around packages as the agent does not know about packages. But the data will still be processed correctly, stored and works with the dashboards. Basically for each stream the ingest manager must check if it belongs to an installed package to show the link and then also show the dashboards ...
If your current solution works, lets keep it and then iterate on it.
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.
Ok, it sounds like the get the package info the "right" way will involve more work that overlaps with the work needed for linking to dashboard, so will defer this for now.
Does that mean this PR gets a 👍? 🙂
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.
The query gets a 👍 Did not really check the rest of the code or did it run locally.
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.
|
||
const dataStreams: DataStream[] = []; | ||
|
||
(indexResults as any[]).forEach(result => { |
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.
any reason of mapping directly results here to dataStream? const dataStreams: DataStream[] = (indexResults as any[]).map(result => { ...
;
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.
will update; was leftover logic from previous attempt :)
|
||
const DATA_STREAM_INDEX_PATTERN = 'logs-*-*,metrics-*-*'; | ||
|
||
export const getListHandler: RequestHandler = async (context, request, response) => { |
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.
Do you think we can cover this API by an API integration tests?
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.
will add
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 took a look into our api integration tests and we have setup for fleet and epm routes, but not the general ingest manager routes. This particular route will need ES data fixtures to get tested. As we have a ton of other things to get done by FF, I'm going to defer test coverage on this for now :(
@nchaulet The missing fields are due to changes on agent side that were recently merged, needs a build/snapshot with those changes: #64134 (comment) |
export const getListHandler: RequestHandler = async (context, request, response) => { | ||
const callCluster = context.core.elasticsearch.dataClient.callAsCurrentUser; | ||
|
||
try { |
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 handler is more knowledgeable than most. Can we move this out to a service or function(s) which accept callCluster
, etc?
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.
Blocking for this PR? This page needs to be iterated on with more functionality and we can move it to a service then.
// eslint-disable-next-line @typescript-eslint/no-empty-interface | ||
export interface GetFleetSetupRequest {} | ||
|
||
export interface CreateFleetSetupRequest { |
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.
Can you talk about these changes? It's not clear to me why we're also making changes to the Fleet setup behavior
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.
No behavioral changes, just cleaning up typings: e0e62b1
GetFleetSetupRequest, CreateFleetSetupRequest, and CreateFleetSetupResponse were duplicated in server/types/rest_spec/fleet_setup.ts
, so I removed that file and kept this one in /common
(to match with our existing structure).
GetFleetSetupRequest is empty so I removed it. CreateFleetSetupRequest is invalid as we no longer require username/password as part of the setup endpoint request, so it's also empty.
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 see. Thanks for the additional info. I wasn't aware of the duplication or that we don't require user/pass any longer for setup.
I feel like setting validate: false
is a step backwards (platform/security encourage them for a reason). I'd prefer to keep that but it's nothing to hold up this change.
* Clean up fleet setup request/response typings * Add data stream model and list route, handler, and request/response types * Initial pass at data streams list * Table styling fixes * Fix types, fix field names * Change forEach to map
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* master: (70 commits) KQL removes leading zero and breaks query (elastic#62748) [FieldFormats] Cleanup: rename IFieldFormatType -> FieldFormatInstanceType (elastic#64193) [ML] Changes transforms wizard UI text (elastic#64150) [Alerting] change server log action type .log to .server-log in README (elastic#64124) [Metrics UI] Design Refresh: Inventory View, Episode 1 (elastic#64026) chore(NA): reduce siem bundle size using babel-plugin-transfor… (elastic#63269) chore(NA): use core-js instead of babel-polyfill on canvas sha… (elastic#63486) skip flaky suite (elastic#61173) skip flaky suite (elastic#62497) Renamed ilm policy for event log so it is not prefixed with dot (elastic#64262) [eslint] no_restricted_paths config cleanup (elastic#63741) Add Oil Rig Icon from @elastic/maki (elastic#64364) [Maps] Migrate Maps embeddables to NP (elastic#63976) [Ingest] Data streams list page (elastic#64134) chore(NA): add file-loader into jest moduleNameMapper (elastic#64330) [DOCS] Added images to automating report generation (elastic#64333) [SIEM][CASE] Api Integration Tests: Configuration (elastic#63948) Expose ability to check if API Keys are enabled (elastic#63454) [DOCS] Fixes formatting in alerting doc (elastic#64338) [data.search.aggs]: Create agg types function for terms agg. (elastic#63541) ...
Resolves #60814
Summary
This PR adds the initial shell page for data streams. A new
DataStream
type and list API was added to support this, along with corresponding response types.GET /api/ingest_manager/data_streams
returns an array ofDataStream
s sorted by last activity (desc) similar to:Deferred
The following tasks still needs to be done in subsequent PRs for this page:
Screenshots