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

Fix brushing chart in TSVB #60296

Merged
merged 1 commit into from
Mar 17, 2020
Merged

Fix brushing chart in TSVB #60296

merged 1 commit into from
Mar 17, 2020

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Mar 16, 2020 •
edited
Loading

Summary

Fix #59654

This makes brushing over the chart works in TSVB again to change the timefilter.

This broke in #56763 when the former import from ui/public was replaced with using the data service. But actually the class we need is exported via data.query.timefilter.timefilter (since data.query.timefilter still has the history on it).

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@timroes timroes added bug Fixes for quality problems that affect the customer experience Feature:TSVB TSVB (Time Series Visual Builder) Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Mar 16, 2020
@timroes timroes requested review from kertal and mbondyra March 16, 2020 18:34
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

wylieconlon
wylieconlon reviewed Mar 16, 2020
View reviewed changes
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

I know we don't have great test coverage of TSVB, but is this something that we could add a functional test for?

kertal
kertal approved these changes Mar 17, 2020
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested locally in Chrome, fixes the regression, a functional test for this would be 🍰

@timroes timroes merged commit a755e55 into elastic:master Mar 17, 2020
@timroes timroes deleted the fix/tsvb-brush branch March 17, 2020 11:16
@timroes
Copy link
Contributor Author

timroes commented Mar 17, 2020

We tried writing brushing tests for TSVB in the pasts already and they turned out to be SUPER flaky (like even compared to our regular flakyness), why we basically decided not to keep them right now. But I agree with the general notion, that we should try to provide those tests if we find a good way for them.

@timroes timroes mentioned this pull request Mar 17, 2020
Merged
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 17, 2020
@gmmorris
Merge branch 'master' into alerting/tls-warning
ff11eb0 
* master: (30 commits)
  [TSVB] fix text color when using custom background color (elastic#60261)
  Fix import to timefilter from in TSVB (elastic#60296)
  [NP] Get rid of usage redirectWhenMissing service (elastic#59777)
  [SIEM] Fix Timeline footer styling (elastic#59587)
  [ML] Fixes to error handling for analytics jobs and file data viz (elastic#60249)
  Give better stack traces for Unhandled Promise Rejection warnings (elastic#60235)
  resolves elastic#58905 (elastic#60120)
  Added variables button for text fields in Pagerduty component. (elastic#60189)
  adds test that action vars are rendered for alert action parms (elastic#60310)
  Closes 59786 by removing the update toast (elastic#60172)
  [EPM] Packages list tabs (elastic#60167)
  Added message variables button for Webhook body form field (elastic#60174)
  Revert "adds new test (elastic#60064)"
  [Maps] move MapSavedObject type out of telemetry (elastic#60127)
  [Reporting] Fix error handling for job handler in route (elastic#60161)
  [Endpoint] TEST: verify alerts page header says 'Alerts' (elastic#60206)
  EMT-248: implement ack resource to accept event payload to acknowledge agent actions (elastic#60218)
  Migrate dual validated range (elastic#59689)
  Embeddable triggers (elastic#58440)
  [Endpoint] Sample data generator CLI script (elastic#59952)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 17, 2020
@gmmorris
Merge branch 'master' into alerting/view-in-app
3c7bea5 
* master: (51 commits)
  do not update cell background if is label cell (elastic#60308)
  FTR configurable test users (elastic#52431)
  [Reporting] Wholesale moves client to newest-platform (elastic#58945)
  [Ingest] Support `show_user` package registry flag (elastic#60338)
  [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380)
  [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108)
  [Fleet] Add config revision to fleet agents (elastic#60292)
  Allow kbn-config-schema to ignore unknown keys (elastic#59560)
  [ML] Functional tests - disable df analytics clone tests
  skip flaky suite (elastic#58643) (elastic#58991)
  [FTR] Add support for --include and --exclude files via tags (elastic#60123)
  [SIEM] Fix link on overview page (elastic#60348)
  skip flaky test (elastic#60369)
  [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242)
  [Lens] Simplify state management from visualization (elastic#58279)
  Changing default type to start and allowing it to be configured by the event category (elastic#60323)
  [ML] Adds the class_assignment_objective to classification (elastic#60358)
  [TSVB] fix text color when using custom background color (elastic#60261)
  Fix import to timefilter from in TSVB (elastic#60296)
  [NP] Get rid of usage redirectWhenMissing service (elastic#59777)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 18, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

timroes pushed a commit that referenced this pull request Mar 18, 2020
Fix import to timefilter from in TSVB (#60296) (#60363)
dfd9666
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wylieconlon wylieconlon wylieconlon left review comments

@kertal kertal kertal approved these changes

@mbondyra mbondyra Awaiting requested review from mbondyra

Assignees
No one assigned
Labels
bug Fixes for quality problems that affect the customer experience Feature:TSVB TSVB (Time Series Visual Builder) release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TSVB timeseries chart brushing does not work
5 participants
@timroes @elasticmachine @kibanamachine @kertal @wylieconlon