-
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] Show recommended minimum size when reducing rule interval below 5 minutes #144170
[APM] Show recommended minimum size when reducing rule interval below 5 minutes #144170
Conversation
Pinging @elastic/apm-ui (Team:APM) |
import { ApmRuleParamsContainer } from '../../ui_components/apm_rule_params_container'; | ||
|
||
export interface RuleParams { | ||
windowSize?: number; | ||
windowUnit?: TimeUnit; | ||
windowUnit?: TIME_UNITS; |
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.
Replace our custom units with units from alerting.
@@ -65,7 +64,7 @@ export function ErrorCountRuleType(props: Props) { | |||
(callApmApi) => { | |||
const { interval, start, end } = getIntervalAndTimeRange({ | |||
windowSize: params.windowSize, | |||
windowUnit: params.windowUnit as TimeUnit, |
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.
Typecasting no longer needed because we use the same types as alerting
@@ -142,7 +142,7 @@ export { loadRule } from './application/lib/rule_api/get_rule'; | |||
export { loadAllActions } from './application/lib/action_connector_api'; | |||
export { suspendedComponentWithProps } from './application/lib/suspended_component_with_props'; | |||
export { loadActionTypes } from './application/lib/action_connector_api/connector_types'; | |||
export type { TIME_UNITS } from './application/constants'; | |||
export { TIME_UNITS } from './application/constants'; |
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 had to export this to access the runtime value
cc @elastic/mlr-docs |
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.
alerting change LGTM
Poking through the rest of the PR, noticing a type change on RuleParams::windowUnit
(TimeUnit
-> TIME_UNITS
). Which have the same values, but the types are different. Since this looks like params for a rule, which would be saved in the SO, need to consider migration issues.
I'm thinking this will not cause a migration, since the values remain unchanged, but thought I'd mention that sometimes changes similar to this could cause a migration - for instance, if the version was a value in the old type not in the new type, kinda thing. Does not look like that's the case here - both types support all the values.
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 some nits that don't block it.
}) { | ||
return ( | ||
<EuiCallOut | ||
title={`Please increase "For the last" to at least ${ |
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 about using i18n
and down below?!
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 point
@@ -33,6 +56,10 @@ export function ApmRuleParamsContainer(props: Props) { | |||
}, []); | |||
return ( | |||
<> | |||
{showMinimumWindowSizeWarning && minimumWindowSize && ( |
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 like useShowMinimumWindowSize
already checks if minimumWindowSize
is defined and if not it returns false so I don't think you need these two checks.
{showMinimumWindowSizeWarning && minimumWindowSize && ( | |
{showMinimumWindowSizeWarning && ( |
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.
afair this is needed to satisfy Typescript because minimumWindowSize
is optional. I'll see what I can do though
); | ||
} | ||
|
||
function useShowMinimumWindowSize({ |
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.
Does it have to be a hook at all? It looks like a normal function that returns true/false?!
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.
Are you suggesting to inline this? I think it's better to extract it but I don't feel strongly about it.
1836766
to
06e8dbd
Compare
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
Page load bundle
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
* main: (43 commits) [Synthetics] Step details page screenshot (elastic#143452) [Lens] Datatable expression types improvement. (elastic#144173) [packages/kbn-journeys] start apm after browser start and stop after browser is closed (elastic#144267) [Files] Make files namespace agnostic (elastic#144019) Implement base browser-side logging system (elastic#144107) Correct wrong multiplier for byte conversion (elastic#143751) [Monaco] Add JSON syntax support to the Monaco editor (elastic#143739) CCS Smoke Test for Remote Clusters and Index Management (elastic#142423) [api-docs] Daily api_docs build (elastic#144294) chore(NA): include progress on Bazel tasks (elastic#144275) [RAM] Allow users to see event logs from all spaces they have access to (elastic#140449) [APM] Show recommended minimum size when going below 5 minutes (elastic#144170) [typecheck] delete temporary target_types dirs in packages (elastic#144271) [Security Solution][Endpoint] adds new alert loading utility and un-skip FTR test for endpoint (elastic#144133) [performance/journeys] revert data_stress_test_lens.ts journey step (elastic#144261) [TIP] Use search strategies in Threat Intelligence (elastic#143267) Optimize react-query dependencies (elastic#144206) [babel/node] invalidate cache when synth pkg map is updated (elastic#144258) [APM] AWS lambda estimated cost (elastic#143986) [Maps] layer group wizard (elastic#144129) ...
We've seen that users create rules where the ingest delay is larger than the look back window. This means that alerts will not fire as expected because there's no data available when evaluating the rule.
This PR increases the default window size to 5 minutes, and adds a warning if the window size is lowered below this level.