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
Add KQL filtering in APM rules #163307
Add KQL filtering in APM rules #163307
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
Pinging @elastic/actionable-observability (Team: Actionable Observability) |
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 good. Only big caveat is the custom kql ui component. I was hoping this already existed. We already have unified search and have kql search for service groups (I cannot remember how that is implemented).
...plugins/apm/public/components/alerting/ui_components/apm_rule_kql_filter/rule_kql_filter.tsx
Outdated
Show resolved
Hide resolved
...plugins/apm/public/components/alerting/ui_components/apm_rule_kql_filter/rule_kql_filter.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/server/routes/alerts/rule_types/error_count/get_error_count_chart_preview.ts
Outdated
Show resolved
Hide resolved
...plugins/apm/public/components/alerting/rule_types/transaction_error_rate_rule_type/index.tsx
Outdated
Show resolved
Hide resolved
...gins/apm/public/components/alerting/ui_components/apm_rule_kql_filter/autocomplete_field.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/public/components/alerting/ui_components/apm_rule_kql_filter/kuery_bar.tsx
Outdated
Show resolved
Hide resolved
...ck/plugins/apm/server/routes/alerts/rule_types/error_count/register_error_count_rule_type.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/server/routes/alerts/rule_types/utils/get_parsed_filtered_query.ts
Outdated
Show resolved
Hide resolved
Refactored the code to reuse KQL filter related components from |
x-pack/test/apm_api_integration/tests/alerts/preview_chart_error_count.spec.ts
Outdated
Show resolved
Hide resolved
Thanks Bena! |
@@ -91,7 +83,7 @@ export function MetricsExplorerKueryBar({ | |||
loadSuggestions={curryLoadSuggestions(loadSuggestions)} | |||
onChange={handleChange} | |||
onSubmit={onSubmit} | |||
placeholder={placeholder || defaultPlaceholder} | |||
placeholder={placeholder} |
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.
Would you please make the placeholder mandatory in the props (on line 30 of this file)?
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 placeholder can be empty sometimes, like in (even though it is set as ' '
) -
Line 96 in 933f2a5
placeholder={' '} |
Why do we want to make it mandatory?
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.
Hmm, I thought we had a default there to ensure we always have a placeholder, if there are valid cases that we don't want a placeholder, then having it optional is fine.
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 default placeholder was there with defaultPlaceholder
which contained text Search for infrastructure data… (e.g. host.name:host-1)
. Since we need different placeholder text in Threshold and APM rules, I moved it to respective rule files, so there is no default place holder now.
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.
AO changes LGTM!
I have a question overall about whether adding the KQL filter can cause confusion or not and what the expectation is for it.
For example, in the following case, I was able to add a filter for another service when adding the rule in the current service:
I think it will make the rule complicated, @benakansara is there any idea about how to avoid such cases?
return ( | ||
<ApmRuleParamsContainer | ||
minimumWindowSize={{ value: 5, unit: TIME_UNITS.MINUTE }} | ||
defaultParams={params} | ||
fields={fields} | ||
groupAlertsBy={groupAlertsBy} | ||
kqlFilter={kqlFilter} |
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.
@@ -17,6 +17,8 @@ export const errorCountParamsSchema = schema.object({ | |||
environment: schema.string(), | |||
groupBy: schema.maybe(schema.arrayOf(schema.string())), | |||
errorGroupingKey: schema.maybe(schema.string()), | |||
useKqlFilter: schema.maybe(schema.boolean()), | |||
kqlFilter: schema.maybe(schema.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.
@benakansara For the threshold rule, we use filterQuery
as the field name, and here we have it as kqlFilter
. Shall we use the same name for both rules? (Not sure which one to choose though 🤔 )
@maryam-saeidi That's a good point. I think it is upto the user to add right filters. We have similar behaviour when creating a rule from "Hosts" view. Even though user has selected a host, and started creating a rule, they can add different hosts in filter. The difference is that the rule name is not auto-generated in this case. @sqren What are your thoughts around this? |
Exactly, that's the difference that I see here. (Also the tags) Maybe I am missing something but I didn't see the filtering for the host when I created a rule from a host page, do they add something host specific to the rule when it is created within a host page? |
4bd8292
to
87760ca
Compare
Please close this and open a new PR if possible, this will keep pinging all the people, changing to draft doens't unsubs all the teams pinged |
@shahzad31 Sorry about that, something went wrong while merging to main. I have removed all the other teams from reviewers list, does it still ping them? |
💔 Build FailedFailed CI Steps
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @benakansara |
@maryam-saeidi @sqren Opening a new PR as this one pinged many teams after merging to main. Removing teams from reviewers list doesn't unsubscribe them from notifications. |
Resolves elastic/apm-dev#929 Adds KQL filtering to the following APM rules: - Latency threshold rule - Failed transaction rate rule - Error count threshold rule ### KQL Filter ON <img width="598" alt="Screenshot 2023-07-31 at 16 45 13" src="https://github.com/elastic/kibana/assets/69037875/277ac4c1-a542-4efe-bd0c-c2bccfac1a6c"> ### KQL Filter OFF <img width="602" alt="Screenshot 2023-07-31 at 16 47 23" src="https://github.com/elastic/kibana/assets/69037875/f790ed56-d83d-4732-aa3e-4d7778926fbb"> ### Note Opening a new PR as #163307 pinged many teams after merging to main. Removing teams from reviewers list doesn't unsubscribe them from notifications.
Resolves elastic/apm-dev#929 Adds KQL filtering to the following APM rules: - Latency threshold rule - Failed transaction rate rule - Error count threshold rule ### KQL Filter ON <img width="598" alt="Screenshot 2023-07-31 at 16 45 13" src="https://github.com/elastic/kibana/assets/69037875/277ac4c1-a542-4efe-bd0c-c2bccfac1a6c"> ### KQL Filter OFF <img width="602" alt="Screenshot 2023-07-31 at 16 47 23" src="https://github.com/elastic/kibana/assets/69037875/f790ed56-d83d-4732-aa3e-4d7778926fbb"> ### Note Opening a new PR as elastic#163307 pinged many teams after merging to main. Removing teams from reviewers list doesn't unsubscribe them from notifications.
Resolves elastic/apm-dev#929 Adds KQL filtering to the following APM rules: - Latency threshold rule - Failed transaction rate rule - Error count threshold rule ### KQL Filter ON <img width="598" alt="Screenshot 2023-07-31 at 16 45 13" src="https://github.com/elastic/kibana/assets/69037875/277ac4c1-a542-4efe-bd0c-c2bccfac1a6c"> ### KQL Filter OFF <img width="602" alt="Screenshot 2023-07-31 at 16 47 23" src="https://github.com/elastic/kibana/assets/69037875/f790ed56-d83d-4732-aa3e-4d7778926fbb"> ### Note Opening a new PR as #163307 pinged many teams after merging to main. Removing teams from reviewers list doesn't unsubscribe them from notifications.
Resolves https://github.com/elastic/apm-dev/issues/929
Adds KQL filtering to the following APM rules:
KQL Filter ON
KQL Filter OFF