[APM] Add transaction name filter in failed transaction rate rule type #155405
Conversation
|
Pinging @elastic/apm-ui (Team:APM) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
| @@ -102,7 +106,8 @@ export function TransactionErrorRateRuleType(props: Props) { | |||
| onChange={(value) => { | |||
| if (value !== params.serviceName) { | |||
| setRuleParams('serviceName', value); | |||
| setRuleParams('transactionType', ''); | |||
| setRuleParams('transactionType', undefined); | |||
There was a problem hiding this comment.
Was this mistakenly set to an empty string? What's the impact of this change?
There was a problem hiding this comment.
Yeah I'm also wondering what this means for eg updating existing rules?
There was a problem hiding this comment.
was added here: ad75d90 to reset the values when changing the service name but imo sending an empty string can lead to misleading results.
As for updating an existing rule, it will remove transactionType param from the list and it won't filter for the transaction.type
However, the biggest impact seems to be in the rule executor. Existing rules may have filter fields with empty strings, and filtering emptry string would yield no results. To address this issue, we need to pass queryEmptyString as false when using termQuery for these fields.
For example,
...termQuery(TRANSACTION_TYPE, ruleParams.transactionType, {
queryEmptyString: false,
}),
Fortunately, this issue has been fixed in v8.6, but there was a regression in my previous PR that caused the issue to resurface.
The tests we have in place were not sufficient to detect the regression. 😢
Next
I need to revert the change that removed the queryEmptyString parameter and ensure that empty strings are not filtered out
There was a problem hiding this comment.
but imo sending an empty string can lead to misleading results.
Agree, in most cases undefined is better than "" we just need to make sure that old rules with an empty string still works.
There was a problem hiding this comment.
The tests we have in place were not sufficient to detect the regression. 😢
Can you fix that? :)
There was a problem hiding this comment.
I'll start working on the tests
- Existing rule types might have empty string in their params
kpatticha
force-pushed
the
152329-alert-transaction-name-error-rate
branch
from
fd6c89a
to
a41a8f3
Compare
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
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>
part of #152329
related work #154241
Introduces the Transaction name filter in the failed transaction rate rule type
Screen.Recording.2023-04-20.at.15.38.45.mov
Fixes
The regression introduces in a previous PR
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.