-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Discover][Alerting] Use Discover locator for alert results link #146403
[Discover][Alerting] Use Discover locator for alert results link #146403
Conversation
@elasticmachine merge upstream |
expected head sha didn’t match current head ref. |
…cover-locator-for-alert-results # Conflicts: # src/plugins/discover/public/application/main/components/sidebar/discover_sidebar.test.tsx # src/plugins/discover/public/application/main/components/sidebar/discover_sidebar.tsx # src/plugins/discover/public/application/main/components/sidebar/discover_sidebar_responsive.test.tsx # src/plugins/discover/public/application/main/components/sidebar/discover_sidebar_responsive.tsx
…cover-locator-for-alert-results # Conflicts: # src/plugins/kibana_utils/common/state_management/state_hash.test.ts # src/plugins/kibana_utils/public/state_management/state_encoder/encode_decode_state.ts
services.share.url.locators.get<DiscoverAppLocatorParams>('DISCOVER_APP_LOCATOR')!, | ||
services.dataViews, | ||
index, | ||
dateStart, |
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.
So here's the hopefully last thing I've found. When using dataStart
to generate the link, this doesn't take
excludeHitsFromPreviousRun
into consideration (L120)
updateSearchSource
on L95 returns dataStart
always the full time range. But given excludeHitsFromPreviousRun
is set, there can be an additional filter to prevent to have documents counted twice. This leads to the following mismatch of count when clicking the link in the notification:
Correct me if I'm wrong but I think this was already broken before this PR?
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.
ok, this ain't an issue of this PR, just tried out on a previous version, works the same ... we should open an issue for this
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.
Created #148282
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.
LGTM! Tested again on the cloud. The issue I've found, that the timerange of the notification doesn't take Exclude matches from previous runs
into consideration, works the same on previous versions, so it can be fixed in a follow up
Great idea and work to use locators for the link 👍
…cover-locator-for-alert-results # Conflicts: # src/plugins/discover/public/application/main/components/layout/discover_layout.tsx # src/plugins/discover/public/application/main/components/sidebar/discover_sidebar.test.tsx # src/plugins/discover/public/application/main/components/sidebar/discover_sidebar_responsive.tsx # src/plugins/discover/public/application/main/discover_main_route.tsx # src/plugins/discover/public/application/main/services/discover_state.ts
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: cc @dimaanj |
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.
Response Ops changes LGTM
- Addresses elastic#158262 ## Summary This PR makes alert links shorter by removing redundant props from the encoded state. We should trim it down more in the future. Backporting a small fix for now. For testing: Please follow instructions from this PR description elastic#146403 (cherry picked from commit ef07c97)
# Backport This will backport the following commits from `main` to `8.8`: - [[Discover][Alerts] Make alert links shorter (#158582)](#158582) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Julia Rechkunova","email":"julia.rechkunova@elastic.co"},"sourceCommit":{"committedDate":"2023-05-31T05:28:35Z","message":"[Discover][Alerts] Make alert links shorter (#158582)\n\n- Addresses https://github.com/elastic/kibana/issues/158262\r\n\r\n## Summary\r\n\r\nThis PR makes alert links shorter by removing redundant props from the\r\nencoded state. We should trim it down more in the future. Backporting a\r\nsmall fix for now.\r\n\r\nFor testing:\r\nPlease follow instructions from this PR description\r\nhttps://github.com//pull/146403","sha":"ef07c978689872d2ae3037aa06a0f2f7b23c3582","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:DataDiscovery","backport:prev-minor","v8.9.0"],"number":158582,"url":"https://github.com/elastic/kibana/pull/158582","mergeCommit":{"message":"[Discover][Alerts] Make alert links shorter (#158582)\n\n- Addresses https://github.com/elastic/kibana/issues/158262\r\n\r\n## Summary\r\n\r\nThis PR makes alert links shorter by removing redundant props from the\r\nencoded state. We should trim it down more in the future. Backporting a\r\nsmall fix for now.\r\n\r\nFor testing:\r\nPlease follow instructions from this PR description\r\nhttps://github.com//pull/146403","sha":"ef07c978689872d2ae3037aa06a0f2f7b23c3582"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/158582","number":158582,"mergeCommit":{"message":"[Discover][Alerts] Make alert links shorter (#158582)\n\n- Addresses https://github.com/elastic/kibana/issues/158262\r\n\r\n## Summary\r\n\r\nThis PR makes alert links shorter by removing redundant props from the\r\nencoded state. We should trim it down more in the future. Backporting a\r\nsmall fix for now.\r\n\r\nFor testing:\r\nPlease follow instructions from this PR description\r\nhttps://github.com//pull/146403","sha":"ef07c978689872d2ae3037aa06a0f2f7b23c3582"}}]}] BACKPORT--> Co-authored-by: Julia Rechkunova <julia.rechkunova@elastic.co>
Summary
Closes #145815, #134232
setStateToKbnUrl
which is used in locator. New one in common are lostHashedItemStore
support, since sessions storage are actual only for browserAlert rule has changed
,Data View has changed
removedrule params
anddata view state
which were at the time of invocationHow to create rule
test
Query to use
test
indexElasticsearch query
alert inKQL or Lucene
mode or just using DiscoverAlerts
buttonIS ABOVE: 1
,FOR THE LAST: 30 min
Test query
. It should match some resultsHow to test
Elasticsearch query
rule inKQL or Lucene
mode like described abovetest
data view in Discover. There should be a link to results incontext_message
field. Save the link somewhereView in app
, you should see actual state of ruleChecklist