Skip to content
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 #163825

Merged
merged 28 commits into from Aug 15, 2023

Conversation

benakansara
Copy link
Contributor

Resolves https://github.com/elastic/apm-dev/issues/929

Adds KQL filtering to the following APM rules:

  • Latency threshold rule
  • Failed transaction rate rule
  • Error count threshold rule

KQL Filter ON

Screenshot 2023-07-31 at 16 45 13

KQL Filter OFF

Screenshot 2023-07-31 at 16 47 23

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.

@benakansara benakansara added Team:APM All issues that need APM UI Team support release_note:feature Makes this part of the condensed release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.10.0 labels Aug 14, 2023
@benakansara benakansara self-assigned this Aug 14, 2023
@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@benakansara benakansara marked this pull request as ready for review August 14, 2023 14:44
@benakansara benakansara requested review from a team as code owners August 14, 2023 14:44
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:APM)

@elasticmachine
Copy link
Contributor

Pinging @elastic/actionable-observability (Team: Actionable Observability)

@benakansara
Copy link
Contributor Author

@maryam-saeidi I have addressed this in this PR.

nit: Can we make the space before Use KQL filter a bit less? (Maybe before and after of this component can have the same spacing?)

@benakansara
Copy link
Contributor Author

@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 to continue discussion from slack, do we need to change the naming in this rule? Or we would change it in Threshold rule?

@benakansara
Copy link
Contributor Author

benakansara commented Aug 14, 2023

Copying comment from previous PR: (comment)

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?

@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?

@benakansara
Copy link
Contributor Author

benakansara commented Aug 14, 2023

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?

@maryam-saeidi Ah, when creating a rule from the service view, it automatically adds service name as filter too. That is not the case with the infra rules.

Copy link
Member

@maryam-saeidi maryam-saeidi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! 👏🏻

Thanks for coming up with a solution to use searchConfiguration parameter name everywhere 🎉

@maryam-saeidi
Copy link
Member

Regarding my comment on the previous PR, I talked to @sqren and @boriskirov about it.

The result of the discussion was that we have the same issue now as well, when a user tries to create a rule in one service and then changes the service in the rule parameters, the auto-generated name should be adjusted along the way. But at the moment, this decision is made based on where the user is coming from and is not adjusted automatically afterward.

The other argument considered users responsible for adjusting the rule name and tag if they create a rule for other services from another service's page.

@benakansara
Copy link
Contributor Author

Regarding my comment on the previous PR, I talked to @sqren and @boriskirov about it.

The result of the discussion was that we have the same issue now as well, when a user tries to create a rule in one service and then changes the service in the rule parameters, the auto-generated name should be adjusted along the way. But at the moment, this decision is made based on where the user is coming from and is not adjusted automatically afterward.

The other argument considered users responsible for adjusting the rule name and tag if they create a rule for other services from another service's page.

Thanks for sharing, @maryam-saeidi !

@benakansara benakansara enabled auto-merge (squash) August 15, 2023 14:22
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #63 / Cloud Security Posture Findings Page Table Filters add negated cell value filter

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1472 1473 +1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
observability 518 532 +14

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 3.7MB 3.7MB +7.3KB
observability 1.0MB 1023.8KB -39.6KB
total -32.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
observability 59.4KB 99.3KB +39.9KB
Unknown metric groups

API count

id before after diff
observability 525 541 +16

ESLint disabled line counts

id before after diff
apm 74 75 +1

Total ESLint disabled count

id before after diff
apm 87 88 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @benakansara

@benakansara benakansara merged commit c180112 into elastic:main Aug 15, 2023
23 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 15, 2023
hop-dev pushed a commit to hop-dev/kibana that referenced this pull request Aug 16, 2023
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.
bryce-b pushed a commit that referenced this pull request Aug 22, 2023
Resolves https://github.com/elastic/apm-dev/issues/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.
@benakansara benakansara deleted the feat/kql-filter-in-apm-rules branch October 23, 2023 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:feature Makes this part of the condensed release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" Team:APM All issues that need APM UI Team support v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants