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] Fix apm.transaction_duration alert to aggregrate over service environment #143238
[APM] Fix apm.transaction_duration alert to aggregrate over service environment #143238
Conversation
Pinging @elastic/apm-ui (Team:APM) |
x-pack/plugins/apm/server/routes/alerts/register_transaction_duration_alert_type.ts
Show resolved
Hide resolved
x-pack/plugins/apm/server/routes/alerts/register_transaction_duration_alert_type.ts
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
value: 5500000, | ||
}, | ||
}, | ||
], | ||
}, | ||
}, |
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 wish we had API test for our alerts:
- call an internal (apm) API to create a latency rule
- ingest synthtrace data that causes alerts to fire
- Retrieve alerts from alerts index and verify they have the expected dimensions associated.
There might be timing problems (I don't know how often the scheduler runs or how predictable it is). And currently we call the alerting API directly when creating alerts. In the ideal scenario we'd have our own api that would call the alerting api's underneath. This will also make it MUCH easier for end users to use the API to create specific apm rules.
x-pack/plugins/apm/server/routes/alerts/chart_preview/get_transaction_duration.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/server/routes/alerts/register_transaction_duration_alert_type.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/public/components/alerting/chart_preview/index.tsx
Outdated
Show resolved
Hide resolved
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.
Thanks for fixing the chart previews. We'll have to do this for the other rule types as well, although not blocking for this PR.
💚 Build Succeeded
Metrics [docs]Async chunks
Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: |
…nvironment (elastic#143238) * [APM] Fix apm.transaction_duration alert to aggregrate over service environment (elastic#143217) * PR feedback * updates the alert preview chart for Latency threshold to reflect the correct logic used in this fix * fixes linting errors * Adds shared helper function for avg or pct latencty aggregation * Renamed ChartPreview (multi)data props to `series` which is always an array * fixes test mock object * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * fixes status custom domain and type error * fixes type error * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * Default agg type fixes API test * fixes type error * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * fixes linting issue Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…nvironment (elastic#143238) * [APM] Fix apm.transaction_duration alert to aggregrate over service environment (elastic#143217) * PR feedback * updates the alert preview chart for Latency threshold to reflect the correct logic used in this fix * fixes linting errors * Adds shared helper function for avg or pct latencty aggregation * Renamed ChartPreview (multi)data props to `series` which is always an array * fixes test mock object * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * fixes status custom domain and type error * fixes type error * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * Default agg type fixes API test * fixes type error * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * fixes linting issue Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…nvironment (elastic#143238) * [APM] Fix apm.transaction_duration alert to aggregrate over service environment (elastic#143217) * PR feedback * updates the alert preview chart for Latency threshold to reflect the correct logic used in this fix * fixes linting errors * Adds shared helper function for avg or pct latencty aggregation * Renamed ChartPreview (multi)data props to `series` which is always an array * fixes test mock object * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * fixes status custom domain and type error * fixes type error * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * Default agg type fixes API test * fixes type error * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * fixes linting issue Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Closes #143217
Fixes the Latency Threshold alert type in APM so that each environment is checked separately if the alert should be triggered. If an alert triggers for multiple environments in the same interval, an alert is triggered for each.