-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(streams): add significant events and queries API #216221
base: main
Are you sure you want to change the base?
Conversation
@kdelemme cheers, I think it's fine to remove the UI entirely then I can just diff my changes on top of main |
/ci |
…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --include-path /api/actions --include-path /api/security/role --include-path /api/spaces --include-path /api/streams --include-path /api/fleet --include-path /api/dashboards --update'
return []; | ||
} | ||
|
||
const searchRequests = assetQueries.flatMap((asset) => { |
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 do this in ESQL instead? Since very recently, CHANGE_POINT
is also an ESQL command
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.
Maybe a little too early? I'm not sure if it's available outside of snapshot builds, it has a limit of 1000 values and not sure if we can already group by values (which might not matter for now, but will later). There's also no _msearch equivalent for ES|QL queries. We don't need it, but just a couple of things that make me feel like it's too early.
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 strong opinion, but based on the early nature of this app and the way we want to go with ESQL, it might be good to put pressure on it where possible, to highlight problems.
.fetch('DELETE /api/streams/{name}/queries/{queryId} 2023-10-31', { | ||
params: { path: { name: STREAM_NAME, queryId } }, | ||
}) | ||
.expect(200) |
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 this should return a 404, similar to when you delete an index that does not exist
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 thought we wanted to handle them as a successful noop - will change 👍🏻
data: DataPublicPluginStart; | ||
dataViews: DataViewsPublicPluginStart; | ||
discover?: DiscoverStart; |
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.
We can probably make this required?
import { createServerRoute } from '../../create_server_route'; | ||
import { readSignificantEvents } from './read_significant_events'; | ||
|
||
const stringToDate = z.string().transform((arg) => new Date(arg)); |
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 we have an existing zod type for this?
const stringToDate = z.string().transform((arg) => new Date(arg)); | ||
|
||
export const readSignificantEventsRoute = createServerRoute({ | ||
endpoint: 'GET /api/streams/{name}/significant_events 2023-10-31', |
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.
@flash1293 any opinions on making these public vs internal?
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.
Good question, I'd say public since there is a good use case for people to integrate with this via API (while there isn't really for things like the sample APIs). To me it's basically CRUD
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Unknown metric groupsAPI count
History
|
Summary
Resolves #214374
This PR adds a significant events API that executes the saved queries from the streams and returns the histogram and change points values.
It also adds the upsert, delete and bulk queries API.
@dgieselaar let me know if you want me to clean it up: there are some UI works in there as well but I don't want to mess with your history since your other branch is based on this one...