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

[ML] Single Metric Viewer: ensures chart displays correctly when opening from a job annotation #182176

Merged

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Apr 30, 2024

Summary

Fixes #181910

Ensures the position of the zoom slider gets set correctly when user navigates to the single metric viewer from the anomaly explorer page or jobs list from annotations -> job model snapshots -> single metric viewer.

This PR removes the previous forecast id comparison as it was never getting correctly set so the focusRange was getting reset even when it should not have been because it was already defined.

I believe this was introduced in #176969.

SMVfromAnnotationFix.mp4

Confirming the link with forecast id still works as expected:

ForecastLinkZoom.mp4

Checklist

Delete any items that are not applicable to this PR.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

zoom === undefined &&
(focusRange === undefined ||
this.previousSelectedForecastId !== this.props.selectedForecastId)
zoom === undefined ||
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fixing the bug when coming in from an annotation, but the issue fixed by #176969 is happening again where the zoom is not restored if the URL contains a forecastId and zoom parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed this is fixed by d614c66

@alvarezmelissa87
Copy link
Contributor Author

@elasticmachine merge upstream

@peteharverson peteharverson changed the title [ML] Single Metric Viewer: ensure chart is displayed correctly [ML] Single Metric Viewer: ensures chart displays correctly when opening from a job annotation May 1, 2024
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested latest changes and LGTM

@qn895
Copy link
Member

qn895 commented May 1, 2024

LGTM 🎉

@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
ml 4.2MB 4.2MB -53.0B

History

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

cc @alvarezmelissa87

@alvarezmelissa87 alvarezmelissa87 merged commit a1b990d into elastic:main May 1, 2024
19 checks passed
@alvarezmelissa87 alvarezmelissa87 deleted the ml-annotations-to-smv-fix branch May 1, 2024 17:29
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request May 1, 2024
…ing from a job annotation (elastic#182176)

## Summary

Fixes elastic#181910

Ensures the position of the zoom slider gets set correctly when user
navigates to the single metric viewer from the anomaly explorer page or
jobs list from annotations -> job model snapshots -> single metric
viewer.

This PR removes the previous forecast id comparison as it was never
getting correctly set so the `focusRange` was getting reset even when it
should not have been because it was already defined.

I believe this was introduced in
elastic#176969.

https://github.com/elastic/kibana/assets/6446462/cbd297ae-e3c2-43ab-ad06-27898ea06a3a

Confirming the link with forecast id still works as expected:

https://github.com/elastic/kibana/assets/6446462/9216c880-7bcf-4d46-b071-adb77f2db1e4

### Checklist

Delete any items that are not applicable to this PR.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit a1b990d)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.14

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request May 1, 2024
…en opening from a job annotation (#182176) (#182277)

# Backport

This will backport the following commits from `main` to `8.14`:
- [[ML] Single Metric Viewer: ensures chart displays correctly when
opening from a job annotation
(#182176)](#182176)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Melissa
Alvarez","email":"melissa.alvarez@elastic.co"},"sourceCommit":{"committedDate":"2024-05-01T17:29:28Z","message":"[ML]
Single Metric Viewer: ensures chart displays correctly when opening from
a job annotation (#182176)\n\n## Summary\r\n\r\nFixes
#181910 the
position of the zoom slider gets set correctly when user\r\nnavigates to
the single metric viewer from the anomaly explorer page or\r\njobs list
from annotations -> job model snapshots -> single
metric\r\nviewer.\r\n\r\nThis PR removes the previous forecast id
comparison as it was never\r\ngetting correctly set so the `focusRange`
was getting reset even when it\r\nshould not have been because it was
already defined.\r\n\r\nI believe this was introduced
in\r\nhttps://github.com//pull/176969.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/6446462/cbd297ae-e3c2-43ab-ad06-27898ea06a3a\r\n\r\nConfirming
the link with forecast id still works as
expected:\r\n\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/6446462/9216c880-7bcf-4d46-b071-adb77f2db1e4\r\n\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [ ] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [ ] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is
usable by keyboard only (learn more\r\nabout [keyboard
accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI
touched in this PR does not create any new axe failures\r\n(run axe in
browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n-
[ ] If a plugin configuration key changed, check if it needs to
be\r\nallowlisted in the cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[ ] This renders correctly on smaller devices using a
responsive\r\nlayout. (You can test this [in
your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n-
[ ] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n---------\r\n\r\nCo-authored-by:
Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"a1b990ddd6a58519ecf6da55260de22c21aa0055","branchLabelMapping":{"^v8.15.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix",":ml","Feature:Anomaly
Detection","v8.14.0","v8.15.0"],"title":"[ML] Single Metric Viewer:
ensures chart displays correctly when opening from a job
annotation","number":182176,"url":"#182176
Single Metric Viewer: ensures chart displays correctly when opening from
a job annotation (#182176)\n\n## Summary\r\n\r\nFixes
#181910 the
position of the zoom slider gets set correctly when user\r\nnavigates to
the single metric viewer from the anomaly explorer page or\r\njobs list
from annotations -> job model snapshots -> single
metric\r\nviewer.\r\n\r\nThis PR removes the previous forecast id
comparison as it was never\r\ngetting correctly set so the `focusRange`
was getting reset even when it\r\nshould not have been because it was
already defined.\r\n\r\nI believe this was introduced
in\r\nhttps://github.com//pull/176969.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/6446462/cbd297ae-e3c2-43ab-ad06-27898ea06a3a\r\n\r\nConfirming
the link with forecast id still works as
expected:\r\n\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/6446462/9216c880-7bcf-4d46-b071-adb77f2db1e4\r\n\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [ ] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [ ] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is
usable by keyboard only (learn more\r\nabout [keyboard
accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI
touched in this PR does not create any new axe failures\r\n(run axe in
browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n-
[ ] If a plugin configuration key changed, check if it needs to
be\r\nallowlisted in the cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[ ] This renders correctly on smaller devices using a
responsive\r\nlayout. (You can test this [in
your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n-
[ ] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n---------\r\n\r\nCo-authored-by:
Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"a1b990ddd6a58519ecf6da55260de22c21aa0055"}},"sourceBranch":"main","suggestedTargetBranches":["8.14"],"targetPullRequestStates":[{"branch":"8.14","label":"v8.14.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.15.0","branchLabelMappingKey":"^v8.15.0$","isSourceBranch":true,"state":"MERGED","url":"#182176
Single Metric Viewer: ensures chart displays correctly when opening from
a job annotation (#182176)\n\n## Summary\r\n\r\nFixes
#181910 the
position of the zoom slider gets set correctly when user\r\nnavigates to
the single metric viewer from the anomaly explorer page or\r\njobs list
from annotations -> job model snapshots -> single
metric\r\nviewer.\r\n\r\nThis PR removes the previous forecast id
comparison as it was never\r\ngetting correctly set so the `focusRange`
was getting reset even when it\r\nshould not have been because it was
already defined.\r\n\r\nI believe this was introduced
in\r\nhttps://github.com//pull/176969.\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/6446462/cbd297ae-e3c2-43ab-ad06-27898ea06a3a\r\n\r\nConfirming
the link with forecast id still works as
expected:\r\n\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/6446462/9216c880-7bcf-4d46-b071-adb77f2db1e4\r\n\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [ ] Any text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [ ] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n- [ ] Any UI touched in this PR is
usable by keyboard only (learn more\r\nabout [keyboard
accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI
touched in this PR does not create any new axe failures\r\n(run axe in
browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n-
[ ] If a plugin configuration key changed, check if it needs to
be\r\nallowlisted in the cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[ ] This renders correctly on smaller devices using a
responsive\r\nlayout. (You can test this [in
your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n-
[ ] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n---------\r\n\r\nCo-authored-by:
Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"a1b990ddd6a58519ecf6da55260de22c21aa0055"}}]}]
BACKPORT-->

Co-authored-by: Melissa Alvarez <melissa.alvarez@elastic.co>
TinLe added a commit to TinLe/kibana that referenced this pull request May 1, 2024
* master: (1654 commits)
  Bump ejs from 3.1.9 to 3.1.10
  Don't render exceptions flyout if data is loading (elastic#181588)
  Enable value list modal (elastic#181593)
  skip flaky suite (elastic#181777)
  skip failing test suite (elastic#182263)
  [Mappings Editor] Disable _source field in serverless (elastic#181712)
  [data.search] Fix unhandled promise rejections (elastic#181785)
  [Fleet] Fix logic for detecting first time Elastic Agent users (elastic#182214)
  [ML] Decouple data_visualizer from MapEmbeddable (elastic#181928)
  [ES|QL] Sorting accepts expressions (elastic#181916)
  [ML] Single Metric Viewer: ensures chart displays correctly when opening from a job annotation (elastic#182176)
  Adding optional Description field to Roles APIs (elastic#182039)
  Upgrade Markdown-it to 14.1.0 (elastic#182244)
  Bump xml-crypto from 5.0.0 to 6.0.0
  [DOCS] Fix docs and screenshots for rule creation changes (elastic#181925)
  Update dependency elastic-apm-node to ^4.5.3 (main) (elastic#182236)
  [Obs AI Assistant] register alert details context in observability plugin (elastic#181501)
  Add `@typescript-eslint/no-floating-promises` (elastic#181456)
  [Playground] Propagate Error message into FE (elastic#182201)
  [ES|QL] Rename the setting to a more generic one and move to the general section (elastic#182074)
  ...
yuliacech pushed a commit to yuliacech/kibana that referenced this pull request May 3, 2024
…ing from a job annotation (elastic#182176)

## Summary

Fixes elastic#181910

Ensures the position of the zoom slider gets set correctly when user
navigates to the single metric viewer from the anomaly explorer page or
jobs list from annotations -> job model snapshots -> single metric
viewer.

This PR removes the previous forecast id comparison as it was never
getting correctly set so the `focusRange` was getting reset even when it
should not have been because it was already defined.

I believe this was introduced in
elastic#176969.


https://github.com/elastic/kibana/assets/6446462/cbd297ae-e3c2-43ab-ad06-27898ea06a3a

Confirming the link with forecast id still works as expected:



https://github.com/elastic/kibana/assets/6446462/9216c880-7bcf-4d46-b071-adb77f2db1e4


### Checklist

Delete any items that are not applicable to this PR.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants