Skip to content

[ML] Single Metric Viewer: Fix time bounds with custom strings.#55045

Merged
walterra merged 3 commits intoelastic:masterfrom
walterra:ml-fix-appstate-time-bounds
Jan 17, 2020
Merged

[ML] Single Metric Viewer: Fix time bounds with custom strings.#55045
walterra merged 3 commits intoelastic:masterfrom
walterra:ml-fix-appstate-time-bounds

Conversation

@walterra
Copy link
Copy Markdown
Contributor

@walterra walterra commented Jan 16, 2020

Summary

Fixes #54903.
Part of #52986.

Makes sure to set bounds via timefilter.getBounds() again and not infer directly from globalState to correctly consider custom strings like now-15m.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@walterra walterra added bug Fixes for quality problems that affect the customer experience regression :ml Feature:Anomaly Detection ML anomaly detection v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v7.6.0 labels Jan 16, 2020
@walterra walterra requested a review from a team as a code owner January 16, 2020 14:22
@walterra walterra self-assigned this Jan 16, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Copy Markdown
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 and confirmed the view is now opening correctly when passed a relative date in the time range, such as

http://localhost:5601/brj/app/ml#/timeseriesexplorer?_g=(ml:(jobIds:!(gallery_total_bytes)),refreshInterval:(pause:true,value:'0'),time:(from:now-1h,to:now))

Copy link
Copy Markdown
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

@walterra
Copy link
Copy Markdown
Contributor Author

Thanks @jgowdyelastic @peteharverson for the review! I added another commit 5f4d4c1 after I found out that time strings which are also invalid for timefilter (e.g. something like now-asdf15min) would also break the page. This is now fixed by not passing on an undefined for min/max of bounds to the inner components. I also added a unit test for TimeSeriesExplorerUrlStateManager. The test isn't able to cover the particular issue that this PR is solving but at least it's a first test to run the component properly. The code also adds a few checks against timefilter methods when they could return undefined.

Copy link
Copy Markdown
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

LGTM ⚡️

Copy link
Copy Markdown
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 edits and LGTM

@walterra walterra merged commit f13adfa into elastic:master Jan 17, 2020
@walterra walterra deleted the ml-fix-appstate-time-bounds branch January 17, 2020 12:16
walterra added a commit to walterra/kibana that referenced this pull request Jan 17, 2020
…tic#55045)

Makes sure to set bounds via timefilter.getBounds() again and not infer directly from globalState to correctly consider custom strings like now-15m.
walterra added a commit to walterra/kibana that referenced this pull request Jan 17, 2020
…tic#55045)

Makes sure to set bounds via timefilter.getBounds() again and not infer directly from globalState to correctly consider custom strings like now-15m.
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 17, 2020
…t-of-legacy

* 'master' of github.com:elastic/kibana: (142 commits)
  [Vis] Move Timelion Vis to vis_type_timelion (elastic#52069)
  Deprecate `chrome.navlinks.update` and add documentation (elastic#54893)
  [ML] Single Metric Viewer: Fix time bounds with custom strings. (elastic#55045)
  [Vis: Default editor] EUIficate and Reactify the sidebar (elastic#49864)
  [Mappings editor] Fix cannot set boolean value for "null_value" param (elastic#55015)
  [SIEM] Adds support for apm-* to the network map (elastic#54876)
  [Reporting] Define shims of legacy dependencies (elastic#54082)
  Resolver is overflow: hidden to prevent obscured elements from showing up (elastic#55076)
  Upgraded EUI to 18.2.1 (elastic#55090)
  [Maps] Support styles on agg fields with _of_ in name (elastic#54965)
  Remove xpack_main requirement, it's no longer in use (elastic#55060)
  Fix Snapshots Policies Alignment Issue in IE11 (elastic#54866)
  first rule cuts (elastic#54990)
  [DOCS] Adds geocentroid note to coordinate map (elastic#54389)
  [Canvas] Fixes the Copy Post Url link (elastic#54831)
  Fixes bugs with full screen filters (elastic#54792)
  [ML] Fix decoding in the URL state  (elastic#54915)
  Remove redundant `x-pack/typings`. (elastic#55042)
  [SIEM][Detection Engine] Adds critical missing status route to prepackaged rules
  Generate legacy vars when rendering all applications (elastic#54768)
  ...

# Conflicts:
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json
walterra added a commit that referenced this pull request Jan 17, 2020
…) (#55164)

Makes sure to set bounds via timefilter.getBounds() again and not infer directly from globalState to correctly consider custom strings like now-15m.
walterra added a commit that referenced this pull request Jan 17, 2020
…) (#55163)

Makes sure to set bounds via timefilter.getBounds() again and not infer directly from globalState to correctly consider custom strings like now-15m.
jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 17, 2020
…tic#55045)

Makes sure to set bounds via timefilter.getBounds() again and not infer directly from globalState to correctly consider custom strings like now-15m.
@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

History

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

gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 20, 2020
* upstream/master: (83 commits)
  [Reporting] Fix map tiles not loading by using Chrome's Remote Protocol (elastic#55137)
  [Data Plugin] combine autocomplete provider and suggestions provider (elastic#54451)
  resolves elastic#53038 - remove references to specific license levels (elastic#53858)
  highlighting rules should still know about url parts when in sql state (elastic#55200)
  [Metric] convert mocha tests to jest (elastic#54054)
  [skip-ci] Update vector styling docs for 7.6 UI changes and new features (elastic#55087)
  Fix enable API to schedule task after alert is updated (elastic#55095)
  chore(NA): add 7.6 branch to the list of backport branches (elastic#54998)
  Convert tests to jest in vis_type_timeseries/public & common folders (elastic#55023)
  [ML] Accessibility fix for structural markup on table rows (elastic#55075)
  [Mappings editor] include/exclude fields only support custom options (elastic#54949)
  [Vis] Move Timelion Vis to vis_type_timelion (elastic#52069)
  Deprecate `chrome.navlinks.update` and add documentation (elastic#54893)
  [ML] Single Metric Viewer: Fix time bounds with custom strings. (elastic#55045)
  [Vis: Default editor] EUIficate and Reactify the sidebar (elastic#49864)
  [Mappings editor] Fix cannot set boolean value for "null_value" param (elastic#55015)
  [SIEM] Adds support for apm-* to the network map (elastic#54876)
  [Reporting] Define shims of legacy dependencies (elastic#54082)
  Resolver is overflow: hidden to prevent obscured elements from showing up (elastic#55076)
  Upgraded EUI to 18.2.1 (elastic#55090)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Fixes for quality problems that affect the customer experience Feature:Anomaly Detection ML anomaly detection :ml regression release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v7.7.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[APM] ML integration: "View job" link is broken, doesn't show job data in single metric viewer in ML

6 participants