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] Transaction type selector in rule flyout incorrectly lists "All" option #143861

Closed
Tracked by #143881
dgieselaar opened this issue Oct 24, 2022 · 9 comments · Fixed by #143881
Closed
Tracked by #143881

[APM] Transaction type selector in rule flyout incorrectly lists "All" option #143861

dgieselaar opened this issue Oct 24, 2022 · 9 comments · Fixed by #143881
Labels
apm:alerting bug Fixes for quality problems that affect the customer experience Team:APM - DEPRECATED Use Team:obs-ux-infra_services.

Comments

@dgieselaar
Copy link
Member

dgieselaar commented Oct 24, 2022

In various transaction-related rule types, we offer the user the choice to select a transaction type. The value should be required, as metrics from different transaction types usually don't mix. A good example of this is page-load transactions from the RUM agent vs route-change transactions. The former involve a lot more heavy lifting to bootstrap initial resources for a page. The latter is usually a lot quicker as it happens past the bootstrapping stage.

However, the transaction type selector that we use lists the "All" option, even though we don't enumerate over transaction type in some of the rule types. Additionally, once the user selects "All", the label is displayed as "ENVIRONMENT_ALL", which does not make sense in this context. Presumably this is because we use a shared component (SuggestionSelect) in an unintended manner. Once the user has created a rule, it will yield no results, as the value used for filtering on transaction type is the string "ENVIRONMENT_ALL".

We should make sure that:

  • the all option works similar to service.environment: ALL is interpreted as: don't apply a filter for transaction.type, but enumerate over all transaction types in the rule executor
  • the selected service name is taken into account when displaying suggestions for transaction type
@dgieselaar dgieselaar added bug Fixes for quality problems that affect the customer experience Team:APM - DEPRECATED Use Team:obs-ux-infra_services. labels Oct 24, 2022
@elasticmachine
Copy link
Contributor

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

@sorenlouv
Copy link
Member

sorenlouv commented Oct 24, 2022

the all option is removed for transaction type in all rule types

I don't think we need to remove the All option. Instead, it should work like the "All" option for environments and services, meaning, it should be added to the multi_terms agg, but not added as a filter.

@dgieselaar
Copy link
Member Author

@sqren that's fair, I've updated the description.

@sorenlouv
Copy link
Member

However, the transaction type selector that we use lists the "All" option, even though we don't enumerate over transaction type in some of the rule types.

I think this will be fixed in #143881

@sorenlouv
Copy link
Member

It's not just the transaction type selector that has a bug with the "All" option - it's all of the dropdowns.

alerting.bugs.mp4

Should we use this issue for fixing all of them, or create separate issues for each dropdown (service name and service environment are the other two)?

@sorenlouv
Copy link
Member

Another problem that affects all the selectors, is when the field is cleared. This should probably be equivalent of selecting "All" but now it's set to undefined, which causes an error for the preview.

environment.picker.mp4

ogupte added a commit to ogupte/kibana that referenced this issue Oct 29, 2022
@ogupte
Copy link
Contributor

ogupte commented Oct 29, 2022

However, the transaction type selector that we use lists the "All" option, even though we don't enumerate over transaction type in some of the rule types.

I think this will be fixed in #143881

I believe the only Rule type that we didn't enumerate over transaction type was latency threshold (transaction duration) which is now part of a multi_terms agg in my PR #143881. There, i've also fixed the issue when selecting 'All' would set ENVIRONMENT_ALL as the value, now an empty string is the value. I've also updated the termQuery utility function to optionally test for empty string.

ogupte added a commit that referenced this issue Oct 29, 2022
…143881)

* [APM] Support specific fields when creating service groups (#142201)

* add support to anomaly rule type to store supported service group fields in alert

* address PR feedback and fixes checks

* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* add API tests for field validation

* fixes linting

* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

* fixes multi_terms sort order paths, for each rule type query

* adds unit tests and moves some source files

* fixed back import path

* PR feedback

* improvements to kuery validation

* fixes selecting 'All' in service.name, transaction.type fields when creating/editing APM Rules (#143861)

Co-authored-by: kibanamachine <[email protected]>
@sorenlouv
Copy link
Member

@ogupte did you also fixed the bugs i mentioned in this issue or should I create new issues for those?

@ogupte
Copy link
Contributor

ogupte commented Nov 2, 2022

did you also fixed the bugs i mentioned in this issue or should I create new issues for those?

The PR fixed the issue when clearing field so that it would behave like selecting 'All' and when selecting 'All' it would show 'ENVIRONMENT_ALL'.
But it does not fix any issue of a broken preview.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:alerting bug Fixes for quality problems that affect the customer experience Team:APM - DEPRECATED Use Team:obs-ux-infra_services.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants