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

[Custom threshold] Add group by filters to the custom threshold view in app URL #177016

Merged
merged 5 commits into from
Feb 22, 2024
Merged

[Custom threshold] Add group by filters to the custom threshold view in app URL #177016

merged 5 commits into from
Feb 22, 2024

Conversation

maryam-saeidi
Copy link
Member

Resolves #173713

Summary

This PR adds group by filters to the view in the app URL for the custom threshold rule:

image

I also moved some types to the common folder and adjusted them to match the reality.

🧪 How to test

  • Create a custom threshold rule with group by
    • One with persisted data view
    • One with an ad-hoc data view
  • Check the view in app link in the alerts table, you should also see the group filters there.
  • Check the view in app URL from the actions, it should also include the group filters.

@maryam-saeidi maryam-saeidi added release_note:skip Skip the PR/issue when compiling release notes Feature: Custom threshold Observability custom threshold rule type labels Feb 15, 2024
@maryam-saeidi maryam-saeidi self-assigned this Feb 15, 2024
@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.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@maryam-saeidi
Copy link
Member Author

/ci

@maryam-saeidi
Copy link
Member Author

/ci

@maryam-saeidi maryam-saeidi marked this pull request as ready for review February 15, 2024 16:30
@maryam-saeidi maryam-saeidi requested review from a team as code owners February 15, 2024 16:30
@botelastic botelastic bot added the Team:obs-ux-management Observability Management User Experience Team label Feb 19, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

maryam-saeidi
maryam-saeidi commented Feb 19, 2024
View reviewed changes
packages/kbn-alerts-as-data-utils/src/field_maps/legacy_experimental_field_map.ts
@@ -37,12 +37,12 @@ export const legacyExperimentalFieldMap = {
},
[ALERT_GROUP_FIELD]: {
type: 'keyword',
array: false,
array: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

@ymao1 Is it safe to fix this field mapping? array: false was wrong and it should have been true.

@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
observability 634.5KB 630.5KB -4.0KB

Page load bundle

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

id before after diff
observability 103.3KB 106.4KB +3.0KB

History

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

cc @maryam-saeidi

@benakansara benakansara self-requested a review February 21, 2024 21:17
benakansara
benakansara reviewed Feb 21, 2024
View reviewed changes
x-pack/plugins/observability/common/custom_threshold_rule/helpers/get_group.ts
[groupFieldName || group.field]: {
value: group.value,
},
match_phrase: {
Copy link
Contributor

Choose a reason for hiding this comment

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

@maryam-saeidi why do we need match_phrase instead of term?

Copy link
Member Author

Choose a reason for hiding this comment

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

Filters use match_phrase by default, so I used them here, too.

benakansara
benakansara reviewed Feb 21, 2024
View reviewed changes
Copy link
Contributor

@benakansara benakansara left a comment

Choose a reason for hiding this comment

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

@maryam-saeidi Do you know what this notification is about? It appears when removing filter in case of using ad-hoc dataview while creating rule.

Screenshot 2024-02-21 at 22 38 56

benakansara
benakansara approved these changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@benakansara benakansara left a comment

Choose a reason for hiding this comment

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

LGTM

@maryam-saeidi
Copy link
Member Author

@maryam-saeidi Do you know what this notification is about? It appears when removing filter in case of using ad-hoc dataview while creating rule.

Screenshot 2024-02-21 at 22 38 56

I checked locally and the same happens in case I create an ad-hoc data view directly in Discover, not sure what exactly it means. (I asked this question in Slack)

@maryam-saeidi maryam-saeidi merged commit 637de1d into elastic:main Feb 22, 2024
36 checks passed
@maryam-saeidi maryam-saeidi deleted the 173713-include-group-by branch February 22, 2024 16:24
@kibanamachine kibanamachine added v8.14.0 backport:skip This commit does not require backporting labels Feb 22, 2024
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
@maryam-saeidi @fkanout
[Custom threshold] Add group by filters to the custom threshold view …
aff1efe 
…in app URL (elastic#177016)

Resolves elastic#173713

## Summary

This PR adds group by filters to the view in the app URL for the custom
threshold rule:


![image](https://github.com/elastic/kibana/assets/12370520/c84eecfc-b196-4268-acdd-57bae86ac4c4)

I also moved some types to the common folder and adjusted them to match
the reality.

## 🧪 How to test
- Create a custom threshold rule with group by
   - One with persisted data view
   - One with an ad-hoc data view
- Check the view in app link in the alerts table, you should also see
the group filters there.
- Check the view in app URL from the actions, it should also include the
group filters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benakansara benakansara benakansara approved these changes

@XavierM XavierM XavierM approved these changes

Assignees

@maryam-saeidi maryam-saeidi

Labels
backport:skip This commit does not require backporting Feature: Custom threshold Observability custom threshold rule type release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team v8.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Custom threshold] Include group by information in the view in app URL
7 participants
@maryam-saeidi @apmmachine @elasticmachine @kibana-ci @XavierM @benakansara @kibanamachine