-
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
[APM] Integrate Alert Search bar in alert tab #149610
Conversation
Pinging @elastic/apm-ui (Team:APM) |
x-pack/plugins/apm/server/routes/services/get_services/get_service_alerts.ts
Show resolved
Hide resolved
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.
LGTM, just a couple of suggestions.
push(history, { | ||
query: { | ||
rangeFrom, | ||
}, | ||
}) | ||
} |
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 will be better to visualize. WDYT?
push(history, { | |
query: { | |
rangeFrom, | |
}, | |
}) | |
} | |
push(history, { query: { rangeFrom, }, }) } |
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.
what do you mean "visualize" ?
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 meant just to write it in a single line.
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.
updated here 1e899e0
onRangeToChange={(rangeTo) => | ||
push(history, { | ||
query: { | ||
rangeTo, | ||
}, | ||
}) | ||
} |
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 do something like that? I haven't tested it and feel free to ignore it 😅
function pushToHistory(value: any) {
push(history, { query: { [value]: value } });
}
onRangeToChange={(rangeTo) => | |
push(history, { | |
query: { | |
rangeTo, | |
}, | |
}) | |
} | |
onRangeToChange={pushToHistory} |
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.
let me try 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.
Ah, you'll probably do something like. Maybe not worth it.
function pushToHistory({key:string, value: any}) {
push(history, { query: { [key]: value } });
}
@@ -55,6 +55,18 @@ export function getEnvironmentEsField(environment: string) { | |||
return { [SERVICE_ENVIRONMENT]: environment }; | |||
} | |||
|
|||
export function getEnvironmentKuery(environment: string) { |
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't you use this function instead?
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 could use it like this:
const environmentValue = getEnvironmentEsField(environment)?.[SERVICE_ENVIRONMENT]
let query = `${SERVICE_NAME}:${serviceName}`;
if (environmentKuery) {
// HERE I need to use again `SERVICE_ENVIRONMENT`
query += ` AND ${SERVICE_ENVIRONMENT}: ${environmentValue}`;
}
so I thought it might be cleaner to have the function getEnvironmentKuery
responsible for returning the kuery
… into 146290-alert-search-bar
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
@@ -43,18 +44,30 @@ export const ENVIRONMENT_NOT_DEFINED = { | |||
label: getEnvironmentLabel(ENVIRONMENT_NOT_DEFINED_VALUE), | |||
}; | |||
|
|||
export function getEnvironmentEsField(environment: string) { | |||
if ( | |||
function isEnvironmentDefined(environment: string) { |
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.
@kpatticha Shouldn't this be called isEnvironmentUndefined
?
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.
## Summary this time I did make sure I pushed it 🐒 related to: #149610 (comment)
## Summary Closes: elastic#146290 - Integrate Alert search bar - Delete alerts status filters as it comes for free with the search bar - Respect time range when fetching alert counts on service inventory and service overview page - as the search bar now supports time ranges, it's required to display consistent data. ### Before ![image](https://user-images.githubusercontent.com/3369346/214894397-6850274f-f701-481a-a12c-688c144f4c32.png) ### After https://user-images.githubusercontent.com/3369346/214894788-5fcd42e2-b48f-434f-b38d-18579bfc280e.mov TODO - [x] [getServiceAlerts](https://github.com/elastic/kibana/pull/149610/files#diff-82ef341af674bd7f203551b4d75b73d221a49e6ae4169e0c396e96abb04902bcR59-R67 ) query doesn't include environment while the table respects that filter. Check what's the correct way. --------- Co-authored-by: kibanamachine <[email protected]>
## Summary this time I did make sure I pushed it 🐒 related to: elastic#149610 (comment)
Summary
Closes: #146290
Before
After
Screen.Recording.2023-01-26.at.17.37.44.mov
TODO