-
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
Alert creation and freeform selection #111883
Conversation
Pinging @elastic/apm-ui (Team:apm) |
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.
Looks great!
import * as stories from './error_count_alert_trigger.stories'; | ||
import { composeStories } from '@storybook/testing-react'; | ||
|
||
const { CreatingInApmFromService } = composeStories(stories); |
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.
Yay! Great to see storybook and testing-library playing well together. What's your impression of this so far?
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.
It's pretty great. Centralizes most of the boilerplate and makes injecting data easy.
isLoading={status === FETCH_STATUS.LOADING} | ||
onChange={handleChange} | ||
onCreateOption={handleCreateOption} | ||
onSearchChange={debounce(setSearchValue, 500)} |
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 we want to use throttle
to allow continuous results to come through even as the user is typing.
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 debounce
is more appropriate here. throttle
will just keep making requests. I found https://redd.one/blog/debounce-vs-throttle useful.
deprecations: ({ deprecate }) => [deprecate('enabled', '8.0.0')], | ||
deprecations: ({ deprecate, renameFromRoot }) => [ | ||
deprecate('enabled', '8.0.0'), | ||
renameFromRoot( |
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.
deprecate('enabled', '8.0.0'), | ||
renameFromRoot( | ||
'xpack.apm.maxServiceEnvironments', | ||
`uiSettings.overrides[${maxSuggestions}]` |
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 does this actually do? It looks like we are not using xpack.apm.maxServiceEnvironments
anywhere in the ui anymore.
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.
nvm, I understand now that we are basically renaming the config setting but allowing users to keep using the old name.
import { createApmServerRouteRepository } from './create_apm_server_route_repository'; | ||
|
||
const suggestionsRoute = createApmServerRoute({ | ||
endpoint: 'GET /internal/apm/suggestions', |
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 creating this as an internal api.
I recall there was an issue for doing this in Kibana but can't find it now. I think we should have an APM specific issue so we have it captured on our board too.
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.
Created an issue for internal apis here: #113383
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: |
Allow selecting any service name, transaction type (where appropriate), and environment when creating and editing rules, both in APM and Stack Management. - Create /internal/apm/suggestions endpoint that uses `terms_enum` - Use combo box for environment, service name, and transaction type with suggestions endpoint on all alerts - Remove "Go to APM" callouts on new alert creation - Wrap calls to `createCallApmApi` in alert triggers with `useEffect` - Use `getEnvironmentLabel` for value in environment field expression - Make all `AlertParams` fields optional (except in latency threshold alert) - Add e2e tests for creating an alert - Remove `NewAlertEmptyPrompt` component and `isNewApmRuleFromStackManagement` helper - Replace `maxServiceEnvironments` and `maxServiceSelections` config options with `maxSuggestions` advanced setting. ![CleanShot 2021-09-28 at 10 35 58](https://user-images.githubusercontent.com/9912/135119948-e247615a-d235-4feb-b197-b803f165ad1e.gif) Fixes elastic#106786
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Allow selecting any service name, transaction type (where appropriate), and environment when creating and editing rules, both in APM and Stack Management. - Create /internal/apm/suggestions endpoint that uses `terms_enum` - Use combo box for environment, service name, and transaction type with suggestions endpoint on all alerts - Remove "Go to APM" callouts on new alert creation - Wrap calls to `createCallApmApi` in alert triggers with `useEffect` - Use `getEnvironmentLabel` for value in environment field expression - Make all `AlertParams` fields optional (except in latency threshold alert) - Add e2e tests for creating an alert - Remove `NewAlertEmptyPrompt` component and `isNewApmRuleFromStackManagement` helper - Replace `maxServiceEnvironments` and `maxServiceSelections` config options with `maxSuggestions` advanced setting. ![CleanShot 2021-09-28 at 10 35 58](https://user-images.githubusercontent.com/9912/135119948-e247615a-d235-4feb-b197-b803f165ad1e.gif) Fixes #106786 Co-authored-by: Nathan L Smith <[email protected]>
Allow selecting any service name, transaction type (where appropriate), and environment when creating and editing rules, both in APM and Stack Management.
terms_enum
createCallApmApi
in alert triggers withuseEffect
getEnvironmentLabel
for value in environment field expressionAlertParams
fields optional (except in latency threshold alert)NewAlertEmptyPrompt
component andisNewApmRuleFromStackManagement
helpermaxServiceEnvironments
andmaxServiceSelections
config options withmaxSuggestions
advanced setting.Fixes #106786