Don't save the current timezone in visualizations #34795

timroes · 2019-04-09T14:54:16Z

Summary

Since the introduction of rollups we technically had the possibility to specify a time_zone for a date_histogram in the AggConfig. This was needed so the rollup plugin can overwrite the timezone by the one from the rollup job. Unfortunately that means the time_zone was also serialized and saved in the saved object when saving the visualization, meaning every visualization you created since 6.5 with a date histogram in it basically stored the time zone of the user creating it for the date_histogram.

This means if a user in a different time zone or if you change your Kibana's time_zone, you'll still see that saved visualization in the time zone it was saved in. That's a very bad behavior since it basically breaks collaboration over multiple time zones and changing your Kibana's time zone.

This PR fixes that behavior by making sure the time_zone parameter won't be saved in the saved object anymore, but only lives transient "in memory".

It also migrates all saved visualizations to remove the stored time_zone from it, since there is no legit state where you want to store it with the visualization.

@LeeDr and/or @bhavyarm since this fix is going back to 6.7 and we're migrating all saved object I would appreciate if one of you could give this a QA test too, before merging.

Steps to reproduce

Create any visualization with a date_histogram in it.
Change your Kibana's timezone in the advanced settings and reload the page.
Open the visualization again.
Failure: It will still show the data in the old time_zone. You can also check the saved object. It will have a params.time_zone in the date_histogram in visState.aggs which shouldn't be there.

If possible also test migrating of old documents. If you save an visualization before that fix, it will have that params.time_zone in it (and render in that time zone no matter what you configured in Kibana). If you apply that fix, your saved object should be fixed and the time_zone should be removed from all date_histogram aggregations.

Checklist

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

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~[ ] This was checked for breaking API changes and was labeled appropriately~~
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-04-09T14:54:22Z

Pinging @elastic/kibana-app

timroes · 2019-04-09T14:58:41Z

src/legacy/core_plugins/kibana/migrations.js

@@ -146,6 +175,7 @@ export const migrations = {
        throw new Error(`Failure attempting to migrate saved object '${doc.attributes.title}' - ${e}`);
      }
    },
+    '7.0.1': removeDateHistogramTimeZones,


@tylersmalley I already talked to Chris, but we wanted to clarify with you that our thoughts were correct:

In the backport to 6.7, I'll remove this 7.0.1 migration and just leave the 6.7 migration in. But since we already released 7.0, and potentially have now people on that version that should get the same fix, we need both 6.7.2 and 7.0.1 to do the migrations. Also just adding a 7.0.1 wouldn't be good, since if we backport this to 6.7 a user who runs the 7.0.1 migration on 6.7 would then, after upgrading to 7.0.1+, not get all the 7.0.0 migrations.

There should only be one - one of these needs to be removed. It doesn't necessarily have to follow the versioning of Kibana. On startup, it will see if there are any newer versions for the visualization type, we will run those migrations to get up to that version.

I think there needs to be 2, because some clients will have run the 7.0.0 migrations already, but some will not have (those running 6.x). And we want to fix both people without forcing the 6.x people to upgrade to 7.x. Does that make sense, @tylersmalley

We talked about that offline and it makes sense now to everyone. I updated the code to have a better explanation why we need it twice.

elasticmachine · 2019-04-09T15:45:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

bhavyarm · 2019-04-09T16:27:41Z

@timroes I will test it tomorrow! Cheers.

elasticmachine · 2019-04-09T17:07:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukeelmers

Reviewed code; still need to test. Overall LGTM, just a couple nits. Will try to get this tested tomorrow.

lukeelmers · 2019-04-09T23:07:28Z

src/legacy/core_plugins/kibana/migrations.js

@@ -57,6 +57,34 @@ function migrateIndexPattern(doc) {
  doc.attributes.kibanaSavedObjectMeta.searchSourceJSON = JSON.stringify(searchSource);
 }

+function removeDateHistogramTimeZones(doc) {
+  const newDoc = cloneDeep(doc);


nit: Not a huge thing, but you don't necessarily need to clone every doc that comes in since you are only going to be updating attributes.visState on date histograms. You could return early if the doc is not identified as needing to be changed, and only clone / reassign the visState on docs that you know you need to touch.

lukeelmers · 2019-04-09T23:13:56Z

src/legacy/core_plugins/kibana/migrations.js

+
+        if (get(agg, 'params.customBucket.type', null) === 'date_histogram' && agg.params.customBucket.params) {
+          delete agg.params.customBucket.params.time_zone;
+        }


nit: lodash omit could save you from some conditionals here, e.g.

if (agg.type === 'date_histogram' || get(agg, 'params.customBucket.type') === 'date_histogram') { agg.params = omit(agg.params, ['time_zone', 'customBucket.params.time_zone']); }

Would you prefer that? I find it actually harder to read, then the more explicit version :-)

I think either is fine. Go with what you think is most obvious.

I marked it as a nit for a reason 😉 Totally up to your preference here, I only suggested it because omit will automatically skip things that are undefined.

(And FWIW I had no issues groking the code as it is currently written)

lukeelmers · 2019-04-09T23:19:03Z

src/legacy/ui/public/agg_types/buckets/date_histogram.js

+        // If we would store this changing the time zone in Kibana would not affect any already saved visualizations
+        // anymore, which is not the desired behavior. So always returning undefined here, makes sure we're never
+        // saving that parameter and just keep it "transient".
+        return undefined;


Makes sense, glad you explained this to minimize "WTF moments" in the future.

elasticmachine · 2019-04-10T13:21:44Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

chrisdavies

LGTM

elasticmachine · 2019-04-10T17:05:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukeelmers

LGTM! Finally got the chance to test this (Chrome OSX)... I created a new vis with a date_histogram agg, saved it, manually updated the migrationVersion to something older, restarted the Kibana server, and verified the migration ran and correctly removed the time_zone param.

src/legacy/core_plugins/kibana/migrations.js

Co-Authored-By: timroes <[email protected]>

elasticmachine · 2019-04-11T07:31:19Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

bhavyarm · 2019-04-11T11:03:26Z

LGTM. Tested it on chrome OSX. No time_zone in date histogram in saved objects.

* Don't save the current timezone in visualizations * Add additional test * Add test and switch migration number * Don't clone object according to review feedback * Better documentation * Update src/legacy/core_plugins/kibana/migrations.js Co-Authored-By: timroes <[email protected]>

) * Don't save the current timezone in visualizations (#34795) * Don't save the current timezone in visualizations * Add additional test * Add test and switch migration number * Don't clone object according to review feedback * Better documentation * Update src/legacy/core_plugins/kibana/migrations.js Co-Authored-By: timroes <[email protected]> * Fix migrationVersion in tests

) * Don't save the current timezone in visualizations (#34795) * Don't save the current timezone in visualizations * Add additional test * Add test and switch migration number * Don't clone object according to review feedback * Better documentation * Update src/legacy/core_plugins/kibana/migrations.js Co-Authored-By: timroes <[email protected]> * Fix migrationVersion in integration tests * Fix tests * Fix migrationVersion in tests * Fix more tests

timroes added 2 commits April 9, 2019 16:39

Don't save the current timezone in visualizations

816d442

Add additional test

0ae6ed0

timroes added bug Fixes for quality problems that affect the customer experience Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.2.0 v7.0.1 v6.7.2 labels Apr 9, 2019

timroes requested review from chrisdavies, lukeelmers and ppisljar April 9, 2019 14:54

Add test and switch migration number

8c91d8e

timroes commented Apr 9, 2019

View reviewed changes

timroes requested a review from stacey-gammon April 9, 2019 14:59

Merge branch 'master' into fix-date-histogram-tz

217a9ad

lukeelmers reviewed Apr 9, 2019

View reviewed changes

Don't clone object according to review feedback

53cda84

chrisdavies approved these changes Apr 10, 2019

View reviewed changes

Better documentation

91beb2f

lukeelmers approved these changes Apr 10, 2019

View reviewed changes

src/legacy/core_plugins/kibana/migrations.js Outdated Show resolved Hide resolved

Update src/legacy/core_plugins/kibana/migrations.js

6a2dcd2

Co-Authored-By: timroes <[email protected]>

timroes merged commit aab98ae into elastic:master Apr 11, 2019

timroes deleted the fix-date-histogram-tz branch April 11, 2019 12:24

timroes mentioned this pull request Apr 11, 2019

[7.x] Don't save the current timezone in visualizations (#34795) #34924

Merged

timroes mentioned this pull request Apr 11, 2019

[7.0] Don't save the current timezone in visualizations (#34795) #34926

Merged

timroes mentioned this pull request Apr 11, 2019

[6.7] Don't save the current timezone in visualizations (#34795) #34927

Merged

This was referenced Apr 23, 2019

Roll-up broken on timefields not rolled up in the same time zone as Kibana #35494

Closed

Make time_zone parameter properly volatile #35536

Merged

tylersmalley mentioned this pull request Dec 10, 2019

Moving saved object management to an ES plugin #49764

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't save the current timezone in visualizations #34795

Don't save the current timezone in visualizations #34795

timroes commented Apr 9, 2019

elasticmachine commented Apr 9, 2019

timroes Apr 9, 2019

tylersmalley Apr 10, 2019

chrisdavies Apr 10, 2019

timroes Apr 10, 2019

elasticmachine commented Apr 9, 2019

bhavyarm commented Apr 9, 2019

elasticmachine commented Apr 9, 2019

lukeelmers left a comment

lukeelmers Apr 9, 2019

lukeelmers Apr 9, 2019

timroes Apr 10, 2019

chrisdavies Apr 10, 2019

lukeelmers Apr 10, 2019

lukeelmers Apr 10, 2019 •

edited

Loading

lukeelmers Apr 9, 2019

elasticmachine commented Apr 10, 2019

chrisdavies left a comment

elasticmachine commented Apr 10, 2019

lukeelmers left a comment

elasticmachine commented Apr 11, 2019

bhavyarm commented Apr 11, 2019

Don't save the current timezone in visualizations #34795

Don't save the current timezone in visualizations #34795

Conversation

timroes commented Apr 9, 2019

Summary

Checklist

For maintainers

elasticmachine commented Apr 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Apr 9, 2019

💔 Build Failed

bhavyarm commented Apr 9, 2019

elasticmachine commented Apr 9, 2019

💚 Build Succeeded

lukeelmers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukeelmers Apr 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Apr 10, 2019

💔 Build Failed

chrisdavies left a comment

Choose a reason for hiding this comment

elasticmachine commented Apr 10, 2019

💚 Build Succeeded

lukeelmers left a comment

Choose a reason for hiding this comment

elasticmachine commented Apr 11, 2019

💚 Build Succeeded

bhavyarm commented Apr 11, 2019

lukeelmers Apr 10, 2019 •

edited

Loading