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

[APM] Add transaction name filter in latency threshold rule #154241

Merged
merged 24 commits into from
Apr 20, 2023

Conversation

kpatticha
Copy link
Contributor

@kpatticha kpatticha commented Apr 3, 2023

Part of the #152329

  1. Adds a synthrace scenario that generates many transactions per service
  2. Fixes the duration chart preview when selecting All option - [APM] Chart preview in alert doesn't show data when transaction type is all  #152195
  3. Introduces the Transaction name filter in the rule type.

Pages loading the rule type

  1. APM
  2. Alert
  3. Management rule

Consider

Creating a rule

Screen.Recording.2023-04-13.at.12.55.00.mov

Updating a rule

default.mov

Before

Screen.Recording.2023-04-13.at.13.16.17.mov

Notes

Feedback

The default action messages don't include any links. I will create a follow-up ticket to improve the messages with actionable links.

Related bugs but out of the scope of the PR

@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.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@kpatticha
Copy link
Contributor Author

/oblt-deploy

@kpatticha kpatticha force-pushed the 152329-alert-transaction-name branch from b48c760 to f5af7a5 Compare April 6, 2023 11:05
@kpatticha
Copy link
Contributor Author

/oblt-deploy

@kpatticha kpatticha changed the title [APM] Add transaction name as field latency alert [APM] Add transaction name filter in latency threshold rule Apr 13, 2023
@kpatticha kpatticha marked this pull request as ready for review April 13, 2023 11:19
@kpatticha kpatticha requested a review from a team as a code owner April 13, 2023 11:19
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Apr 13, 2023
@elasticmachine
Copy link
Contributor

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

@kpatticha kpatticha added v8.8.0 release_note:enhancement release_note:skip Skip the PR/issue when compiling release notes and removed Team:APM All issues that need APM UI Team support release_note:skip Skip the PR/issue when compiling release notes labels Apr 13, 2023
@kpatticha kpatticha requested a review from a team April 13, 2023 11:21
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Apr 13, 2023
@kpatticha
Copy link
Contributor Author

kpatticha commented Apr 19, 2023

The reason doesn't include the transaction name.

Avg. latency is 1,000 ms in the last 50 mins for synth-go-0. Alert when > 500 ms.

can be:
Avg latency for GET /orders/1 transaction is 1,000 ms in the last 50 minutes for synth-go-0

@benakansara
Copy link
Contributor

The reason doesn't include the transaction name.

Avg. latency is 1,000 ms in the last 50 mins for synth-go-0. Alert when > 500 ms.

can be: Avg latency for GET /orders/1 transaction is 1,000 ms in the last 50 minutes for synth-go-0

@kpatticha I have updated reason message in this PR. The new reason message will be for example -
Avg. latency is 1,000 ms in the last 50 mins for synth-go-0, development, request, GET /orders. Alert when > 500 ms.

I have updated this for all 3 rules - Latency threshold, Failed transaction, Error count rules

@benakansara
Copy link
Contributor

/oblt-deploy

@kpatticha kpatticha force-pushed the 152329-alert-transaction-name branch from 10231f9 to 56c7d24 Compare April 19, 2023 17:51
@kpatticha
Copy link
Contributor Author

@elasticmachine merge upstream

@kpatticha
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

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.4MB 3.4MB +2.9KB

Page load bundle

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

id before after diff
apm 32.1KB 32.2KB +69.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 394 397 +3

Total ESLint disabled count

id before after diff
securitySolution 474 477 +3

History

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

@sorenlouv
Copy link
Member

/oblt-deploy

Comment on lines +140 to +141
x: 1627974600000,
y: 18485.85714285714,
Copy link
Member

Choose a reason for hiding this comment

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

Ah, these are archives. We should replace it with Synthtrace at some point, but at least we capture the current value so we don't unintentionally change it.

Copy link
Member

Choose a reason for hiding this comment

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

We might be able to tackle it during test plan for 8.8. I've created an issue to capture this and the other tests that still rely on archives: #155359


return (
<PopoverExpression value={currentValue || allOptionText} title={label}>
<SuggestionsSelect
Copy link
Member

Choose a reason for hiding this comment

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

We may want to sort the suggestions. Currently they are ordered by volume (afaict)

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sqren do you mean sort alphabetically? The suggestions dropdown uses _terms_enum API which I'm not 100% how it returns the results

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I mean alphabetically. Sorting on the client is fine.

@sorenlouv
Copy link
Member

The alert details page should show the transaction name when it's set similar to how we show service name and environment

image

@kpatticha will you create an issue for this?

@kpatticha
Copy link
Contributor Author

similar to how we show service name and environment
good catch, created
#155364

@kpatticha kpatticha merged commit 7c70508 into elastic:main Apr 20, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 20, 2023
kpatticha added a commit that referenced this pull request Apr 24, 2023
#155405)

part of #152329
related work #154241

Introduces the Transaction name filter in the failed transaction rate
rule type
 



https://user-images.githubusercontent.com/3369346/233386404-1875b283-0321-4bf1-a7d3-66327f7d4ec5.mov


## Fixes

The regression introduces in a previous
[PR](fce4ef8)

Existing rule types can have empty string in their params so we need to
make sure we don't filter empty values as it will yield no results.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
nikitaindik pushed a commit to nikitaindik/kibana that referenced this pull request Apr 25, 2023
…154241)

Part of the elastic#152329

1. Adds a synthrace scenario that generates many transactions per
service
2. Fixes the duration chart preview when selecting All option -
elastic#152195
3. Introduces the `Transaction name` filter in the rule type. 


### Pages loading the rule type
1. APM
2. Alert
3. Management rule 


### Consider 
- [ ] Update/Adding documentation example
https://www.elastic.co/guide/en/kibana/master/apm-alerts.html#apm-create-transaction-alert
## Creating a rule

https://user-images.githubusercontent.com/3369346/231740745-425c8eb8-6798-4ce4-b375-4ef96afdb334.mov

## Updating a rule


https://user-images.githubusercontent.com/3369346/231742035-28f50dfd-64bb-475d-b037-331eea4188d7.mov


### Before



https://user-images.githubusercontent.com/3369346/232799038-2edaa199-b970-48f2-b3ca-728273e4bf44.mov



### Notes

#### Feedback
The default action messages don't include any links. I will create a
follow-up ticket to improve the messages with actionable links.

Related bugs but out of the scope of the PR

- elastic#154818
- elastic#154704

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
nikitaindik pushed a commit to nikitaindik/kibana that referenced this pull request Apr 25, 2023
elastic#155405)

part of elastic#152329
related work elastic#154241

Introduces the Transaction name filter in the failed transaction rate
rule type
 



https://user-images.githubusercontent.com/3369346/233386404-1875b283-0321-4bf1-a7d3-66327f7d4ec5.mov


## Fixes

The regression introduces in a previous
[PR](elastic@fce4ef8)

Existing rule types can have empty string in their params so we need to
make sure we don't filter empty values as it will yield no results.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
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:enhancement Team:APM All issues that need APM UI Team support v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants