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

Make time_zone parameter properly volatile #35536

Merged
merged 3 commits into from
Apr 24, 2019

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Apr 24, 2019

Summary

Fixes #35494

In #34795 we wanted to make the time_zone parameter of date_histogram volatile and no longer save it in the saved object (since that was a severe bug). Unfortunately the serialize method of an aggregation param is called from AggConfig.toJSON which was used in Vis.getSerializableState and Vis.getCurrentState to clone those objects, thus the time_zone was actually also removed on the way to the query and the timezone the rollup plugin set didn't effect the query.

This PR now moves all the logic for the time_zone parameter into its write method (which is used purely when writing out the DSL for that parameter). This method will now check the typeMeta on the index pattern itself (which is set for roll up indexes). If that has a time_zone it's used. Otherwise the time_zone from the kibana settings is used. You can also still technically set the time_zone explicitly, which we currently don't use anywhere (but might be exposed later on in the visualization expression). This means the parameter itself will always be undefined in places it's passed around in the code, and really only be taken care of once written to DSL.

Since the bug has not yet been released I marked this as skip for the release notes.

Testing:

This PR should fix #35494 and now allow to visualize over a rolled-up index pattern while your Kibana is in a different time_zone then the index was rolled up. Also it should not break #34784 again and the time_zone should still not be saved in the visualizations saved object.

Checklist

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

For maintainers

@timroes timroes added bug Fixes for quality problems that affect the customer experience blocker Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 :AppArch v7.2.0 v7.0.1 v6.7.2 labels Apr 24, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@timroes timroes added release_note:skip Skip the PR/issue when compiling release notes WIP Work in progress labels Apr 24, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jen-huang jen-huang self-requested a review April 24, 2019 16:06
@timroes timroes removed the WIP Work in progress label Apr 24, 2019
@flash1293
Copy link
Contributor

Tested and works for me, timezones switch with the configured one on non-rollup vis, always use the rollup timezone for rollup vis.

The Save button being clickable even if nothing has changed seems to be a general thing, it might even be intentional, not sure if we even should "fix" this.

@timroes
Copy link
Contributor Author

timroes commented Apr 24, 2019

@flash1293 yeah I think that's actually desired behavior so far and we only want to block saving if the user is in a "touched but not applied" state. But as long as the state is valid we don't prevent them from saving. Actually it would also be the only way right now to use "Save as new visualization" even if you haven't made any changes.

@flash1293
Copy link
Contributor

@timroes That's what I had in mind. There is probably some code which can be thrown away because of this, but definitely a low prio task which could be tackled during EUI-ification.

Copy link
Member

@ppisljar ppisljar 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

Copy link
Member

@lukeelmers lukeelmers 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 too! Tested Chrome OSX and verified the issue appears to be fixed

@elasticmachine

This comment has been minimized.

Copy link
Contributor

@jen-huang jen-huang 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 and I tested locally with US/Hawaii timezone.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes timroes merged commit 40fad35 into elastic:master Apr 24, 2019
@timroes timroes deleted the volatile-agg-params branch April 24, 2019 19:13
timroes added a commit to timroes/kibana that referenced this pull request Apr 24, 2019
* Move all time_zone logic to write method

* Add more comments
timroes added a commit to timroes/kibana that referenced this pull request Apr 24, 2019
* Move all time_zone logic to write method

* Add more comments
timroes added a commit to timroes/kibana that referenced this pull request Apr 24, 2019
* Move all time_zone logic to write method

* Add more comments
lizozom pushed a commit to lizozom/kibana that referenced this pull request Apr 29, 2019
* Move all time_zone logic to write method

* Add more comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker bug Fixes for quality problems that affect the customer experience Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v6.7.2 v7.0.1 v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Roll-up broken on timefields not rolled up in the same time zone as Kibana
6 participants