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.

Sign up for GitHub

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

Jump to bottom

[APM]Latency threshold rule's threshold context variable should use milliseconds instead of microseconds #150234

Merged
merged 4 commits into from
Feb 8, 2023
Merged

[APM]Latency threshold rule's threshold context variable should use milliseconds instead of microseconds #150234

merged 4 commits into from
Feb 8, 2023

Conversation

cauemarcondes
Copy link
Contributor

closes #125323

I couldn't test it yet, since alerts are firing with any context variable. I'll hold on to merge this until I can fully test it.

@cauemarcondes cauemarcondes requested a review from a team as a code owner February 2, 2023 21:42
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Feb 2, 2023
@elasticmachine
Copy link
Contributor

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

sorenlouv
sorenlouv reviewed Feb 6, 2023
View reviewed changes
...outes/alerts/rule_types/transaction_duration/register_transaction_duration_rule_type.test.ts
@@ -65,7 +65,7 @@ describe('registerTransactionDurationRuleType', () => {
'Avg. latency is 5,500 ms in the last 5 mins for opbeans-java. Alert when > 3,000 ms.',
transactionType: 'request',
serviceName: 'opbeans-java',
threshold: 3000000,
threshold: 3000,
Copy link
Member

Choose a reason for hiding this comment

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

We did the reverse change in #112366. Please double check why we did so and if this change breaks something.

As discussed: I'd love to have an api test (or e2e test) that reproduces this problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We did the reverse change in #112366

I don't think we should show the alerts in microseconds, since the only option we offer when creating an alert is a threshold in milliseconds.

Copy link
Member

Choose a reason for hiding this comment

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

What was the reason for the change in #112366?

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 looks like what was done in this PR (#112366) was the opposite of what Cyrille was asking on this issue. He clearly says that the alert should NOT be shown with microseconds, but instead in milliseconds, which is what I'm changing in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

All right. Change looks fine. Did you find a way to test this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into it during test plan.

@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@cauemarcondes cauemarcondes enabled auto-merge (squash) February 8, 2023 13:38
@cauemarcondes cauemarcondes merged commit 5f43b49 into elastic:main Feb 8, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Feb 8, 2023
@benakansara benakansara mentioned this pull request Feb 9, 2023
Closed
@cauemarcondes cauemarcondes deleted the apm-125323-2 branch February 9, 2023 13:28
@cauemarcondes cauemarcondes self-assigned this Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorenlouv sorenlouv sorenlouv approved these changes

Assignees

@cauemarcondes cauemarcondes

Labels
backport:skip This commit does not require backporting release_note:fix Team:APM All issues that need APM UI Team support v8.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM]Latency threshold rule's threshold context variable should use milliseconds instead of microseconds
5 participants
@cauemarcondes @elasticmachine @kibana-ci @sorenlouv @kibanamachine