[Rollup] Validate timezones based on rules not string comparision #36237

polyfractal · 2018-12-04T22:26:31Z

The date_histogram internally converts obsolete timezones (such as "Canada/Mountain") into their modern equivalent ("America/Edmonton"). But rollup just stores the TZ as provided by the user in the config.

When checking the TZ for query validation we used a simple string comparison, which would fail due to the date_histo's upgrading behavior ("Canada/Mountain" != "America/Edmonton")

Instead, we should convert both to a TimeZone object and check if their rules are compatible. This commit also proactively upgrades the config's TZ to the modern equivalent for good measure, although this isn't strictly necessary and old rollup indices will be fixed just by the query validation tweak.

This has two side-effects:

We should not be adding a term filter on time_zone to the query. This was obsolete anyway due to filtering on job ID + individual msearch
Without the need for term filter on time_zone, there's no need for the request translator to add filtering clauses at all. This functionality was removed (although the filtered agg structure remains, can be cleaned up in a followup PR)

It also exposed a bug:

After verifying the timezones are the same, we need to set the timezone on the rewritten date_histo otherwise it will treat the data as UTC and shift results.

I think this is going to miss the boat for 6.5.3, but I'd like to backport it to 6.5.4 when the time comes. There isn't a workaround if the user indexed rollups with an "obsolete" TZ, and not setting the timezone on the query itself will mess up results.

Closes #36229

The date_histogram internally converts obsolete timezones (such as "Canada/Mountain") into their modern equivalent ("America/Edmonton"). But rollup just stored the TZ as provided by the user. When checking the TZ for query validation we used a string comparison, which would fail due to the date_histo's upgrading behavior. Instead, we should convert both to a TimeZone object and check if their rules are compatible. This commit also proactively upgrades the config's TZ to the modern equivalent for good measure, although this isn't strictly necessary and old rollup indices will be fixed just by the query validation tweak. This has two side-effects: - We should not be adding a term filter on `time_zone` to the query. This was obsolete anyway due to filtering on job ID + individual msearch - Without the need for term filter on `time_zone`, there's no need for the request translator to add filtering clauses at all. This functionality was removed (although the filtered agg structure remains, can be cleaned up in a followup PR) It also exposed a bug: - After verifying the timezones are the same, we need to set the timezone on the rewritten date_histo otherwise it will treat the data as UTC and shift results.

elasticmachine · 2018-12-04T22:26:34Z

Pinging @elastic/es-analytics-geo

Forgot to update these. :)

jimczi

LGTM

polyfractal · 2018-12-05T18:13:51Z

Jenkins, run the gradle build tests 2

rjernst · 2018-12-05T19:27:18Z

x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtils.java

@@ -97,11 +98,13 @@ private static void checkDateHisto(DateHistogramAggregationBuilder source, List<
                    if (agg.get(RollupField.AGG).equals(DateHistogramAggregationBuilder.NAME)) {
                        DateHistogramInterval interval = new DateHistogramInterval((String)agg.get(RollupField.INTERVAL));

-                        String thisTimezone  = (String)agg.get(DateHistogramGroupConfig.TIME_ZONE);
-                        String sourceTimeZone = source.timeZone() == null ? DateTimeZone.UTC.toString() : source.timeZone().toString();
+                        TimeZone thisTimezone = DateTimeZone.forID((String)agg.get(DateHistogramGroupConfig.TIME_ZONE)).toTimeZone();


I don't think we should use TimeZone, it is a legacy (ish) class in the jdk. Use ZoneId instead? If there are places needing DateTimeZone from joda, there is a conversion method in DateUtils.

Ah ok, will do. Thanks for the heads up

polyfractal · 2018-12-10T21:12:14Z

Just a note for posterity: ZoneId doesn't "convert" the string representation of obsolete TZ into the new version (like TimeZone did). So Rollup will continue to store whatever the user gives us (provided it is a valid ZoneId) and we'll do the rule comparison as before like we did with TimeZone.

rjernst · 2018-12-10T23:09:15Z

Should we log a deprecation warning to make the user change to the non-obsolete name of the timezone?

polyfractal · 2018-12-11T00:04:55Z

Do you know if there's a way to get that information from ZoneId or other new date/time classes? I looked around and couldn't find something that would tell me that a particular TZ is deprecated or not, short of using the older TimeZone class. ZoneId does have a map of "short name" aliases ("EST", etc) but not the longer deprecated forms like Japan.

I don't know the new date stuff well (or even the old stuff well), so I may have overlooked it.

I suppose we could add a static list somewhere ourselves, but that seems less good :)

rjernst · 2018-12-11T01:48:18Z

I suppose we could add a static list somewhere ourselves

That's what I was thinking. This is just to help with migrating. I don't think it is anything we should maintain for a long time, so it doesn't need to be programmatic just in case more timezone names are deprecated.

rjernst · 2018-12-11T01:49:31Z

Note that I did something similar already in DateUtils.DEPRECATED_SHORT_TZ_IDS

polyfractal · 2018-12-11T14:11:08Z

Sounds good, I can knock together something. In a followup PR I'll see about expanding the deprecation usage to other places that use Timezones too, like the date_histo agg

polyfractal · 2018-12-11T19:56:09Z

Added a deprecation warning when creating a Rollup job that uses a deprecated/obsolete timezone. Notably this is only when creating the job, not anything else that touches the job, or search. I think that's a much bigger task and better suited to a separate PR.

rjernst · 2018-12-13T19:31:28Z

x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtils.java

-                        String sourceTimeZone = source.timeZone() == null ? DateTimeZone.UTC.toString() : source.timeZone().toString();
+                        ZoneId thisTimezone = ZoneId.of(((String)agg.get(DateHistogramGroupConfig.TIME_ZONE)), ZoneId.SHORT_IDS);
+                        ZoneId sourceTimeZone = source.timeZone() == null
+                            ? ZoneId.of(DateHistogramGroupConfig.DEFAULT_TIMEZONE)


Using ZoneId.of("UTC") (as this effectively does) causes printing using formats with this ZoneId to be messed up. They will get a [UTC] appended to the end of the time, like 1970-01-02T10:17:36.789Z[UTC]. Instead, use ZoneOffset.UTC.

Urgh, yeah not what was intended. Fixing, thanks!

polyfractal · 2019-01-11T16:04:31Z

Ok, so I was becoming increasingly uncomfortable with how the timezone stuff was spreading everywhere. I think these changes are probably still needed at some point, but would rather contain that to a refactor PR.

So I backed out the last ZoneId commit, then introduced a new default constant for ZoneIds that is used for comparisons, while the old String default is used for strings. This keeps the "UTC"/"Z" thing from spreading everywhere.

polyfractal · 2019-02-05T21:57:08Z

Final update, yanked the last remaining pieces of Joda and tweaked the tests according to new master (using Javatime in date_histo gives slightly different results, depending on the timezone you specify).

polyfractal · 2019-02-05T22:05:28Z

@spinscale @pgomulka Might I bother one of you for a review of the javatime stuff, since Ryan is away this week? The rollup bits should be good, but would appreciate someone to verify the time shenanagins and make sure I didn't do anything silly.

We will address this in a separate bugfix pr

pgomulka

that looks good in my view, but I don't know rollups all that much so probably I am not the right person to approve

.../rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java

.../rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportRollupSearchAction.java

polyfractal · 2019-02-07T14:31:37Z

Thanks @pgomulka! Will work on the date/time stuff. Jim previously OK'd the rollup portion, we've just been stuck on the new javatime changes since then :)

polyfractal · 2019-04-15T21:29:29Z

x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupRequestTranslator.java

-            filterConditions.add(new TermQueryBuilder(RollupField.formatFieldName(source,
-                DateHistogramGroupConfig.TIME_ZONE), timezone));
+            ZoneId timeZone = source.timeZone() == null ? DateHistogramGroupConfig.DEFAULT_ZONEID_TIMEZONE : source.timeZone();
+            rolledDateHisto.timeZone(timeZone);


Note: this has the change of always setting a timezone. The only practical change is that UTC will be explicitly set now instead of implicit. Every other timezone will be the same.

I figured this would make debugging a bit easier since there will always be a timezone.

polyfractal · 2019-04-15T21:53:19Z

@jimczi Since it's been a while, would you mind giving this one last sanity check? I don't think anything substantial has changed since you first looked at it, but there were some minor tweaks along the way as we sorted out the java-time stuff.

The tl;dr: is that obsolete timezones throw a deprecation warning but are accepted, timezones are compared based on rules everywhere, and the code knows how to use obsolete or modern timezones for aggregating. And there are tests going both ways so we know if this ever breaks.

jimczi

LGTM, thanks for adding the tests

…6237) The date_histogram internally converts obsolete timezones (such as "Canada/Mountain") into their modern equivalent ("America/Edmonton"). But rollup just stored the TZ as provided by the user. When checking the TZ for query validation we used a string comparison, which would fail due to the date_histo's upgrading behavior. Instead, we should convert both to a TimeZone object and check if their rules are compatible.

…astic#36237) The date_histogram internally converts obsolete timezones (such as "Canada/Mountain") into their modern equivalent ("America/Edmonton"). But rollup just stored the TZ as provided by the user. When checking the TZ for query validation we used a string comparison, which would fail due to the date_histo's upgrading behavior. Instead, we should convert both to a TimeZone object and check if their rules are compatible.

$polyfractal$

$@polyfractal$ polyfractal added >bug v7.0.0 :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data v6.6.0 labels Dec 4, 2018

$@polyfractal$

Fix timestamps in test

cc6563e

Forgot to update these. :)

jimczi approved these changes Dec 5, 2018

View reviewed changes

rjernst reviewed Dec 5, 2018

View reviewed changes

polyfractal added 4 commits December 6, 2018 15:57

$@polyfractal$

Use ZoneId

4867d99

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_obsolete_tz

1ef04f0

$@polyfractal$

Fix test

da7ebe3

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_obsolete_tz

21a38f5

$@polyfractal$

Deprecation warnings when using obsolete timezones

aadf68b

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_obsolete_tz

38ca541

rjernst reviewed Dec 13, 2018

View reviewed changes

polyfractal added 5 commits December 18, 2018 16:06

$@polyfractal$

Switch DateHistoConfig over to ZoneId

ef0f695

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_obsolete_tz

d41dda4

$@polyfractal$

Test fix: searching the wrong index

d049fea

$@polyfractal$

checkstyle

e9f42ef

$@polyfractal$

ML needs to use zones instead of strings too

1f7802f

jasontedor added the v6.7.0 label Dec 19, 2018

polyfractal added 3 commits January 10, 2019 14:30

$@polyfractal$

Add test for TZ validation in action

94abbeb

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_obsolete_tz

d573aef

$@polyfractal$

Yml test version updates

feba7e5

polyfractal added 2 commits February 5, 2019 13:55

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_obsolete_tz

9f4aada

$@polyfractal$

Remove last traces of Joda

de22b2f

$@polyfractal$ polyfractal mentioned this pull request Feb 5, 2019

XPack: core/ccr/Security-cli migration to java-time #38415

Merged

jasontedor added v8.0.0 and removed v7.0.0 labels Feb 6, 2019

$@polyfractal$

Do not set timezone on rollup request histo

b88091b

We will address this in a separate bugfix pr

danielmitterdorfer added v7.2.0 and removed v6.7.0 labels Feb 7, 2019

pgomulka reviewed Feb 7, 2019

View reviewed changes

.../rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportPutRollupJobAction.java Show resolved Hide resolved

.../rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportRollupSearchAction.java Outdated Show resolved Hide resolved

$@polyfractal$ polyfractal mentioned this pull request Mar 28, 2019

Rollup indices break on missing timezone in global timefilter elastic/kibana#33974

Closed

polyfractal added 4 commits April 8, 2019 12:56

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_obsolete_tz

d80e035

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_obsolete_tz

83bf9bf

$@polyfractal$

checkstyle

fbda167

$@polyfractal$

Fix tests, make more realistic

8822afd

$polyfractal$

polyfractal commented Apr 15, 2019

View reviewed changes

$@polyfractal$

Remove unnecessary changes

a9f2911

jimczi approved these changes Apr 16, 2019

View reviewed changes

$@polyfractal$ polyfractal merged commit 1f51f20 into elastic:master Apr 17, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Rollup] Validate timezones based on rules not string comparision #36237

[Rollup] Validate timezones based on rules not string comparision #36237

$@polyfractal$ polyfractal commented Dec 4, 2018 •

edited

Loading

elasticmachine commented Dec 4, 2018

jimczi left a comment

polyfractal commented Dec 5, 2018

rjernst Dec 5, 2018

$@polyfractal$ polyfractal Dec 5, 2018

polyfractal commented Dec 10, 2018

rjernst commented Dec 10, 2018

polyfractal commented Dec 11, 2018

rjernst commented Dec 11, 2018

rjernst commented Dec 11, 2018

polyfractal commented Dec 11, 2018

polyfractal commented Dec 11, 2018

rjernst Dec 13, 2018

$@polyfractal$ polyfractal Dec 18, 2018

polyfractal commented Jan 11, 2019

polyfractal commented Feb 5, 2019

polyfractal commented Feb 5, 2019

pgomulka left a comment

polyfractal commented Feb 7, 2019

$@polyfractal$ polyfractal Apr 15, 2019

polyfractal commented Apr 15, 2019

jimczi left a comment

[Rollup] Validate timezones based on rules not string comparision #36237

[Rollup] Validate timezones based on rules not string comparision #36237

Conversation

polyfractal commented Dec 4, 2018 • edited Loading

elasticmachine commented Dec 4, 2018

jimczi left a comment

Choose a reason for hiding this comment

polyfractal commented Dec 5, 2018

rjernst Dec 5, 2018

Choose a reason for hiding this comment

polyfractal Dec 5, 2018

Choose a reason for hiding this comment

polyfractal commented Dec 10, 2018

rjernst commented Dec 10, 2018

polyfractal commented Dec 11, 2018

rjernst commented Dec 11, 2018

rjernst commented Dec 11, 2018

polyfractal commented Dec 11, 2018

polyfractal commented Dec 11, 2018

rjernst Dec 13, 2018

Choose a reason for hiding this comment

polyfractal Dec 18, 2018

Choose a reason for hiding this comment

polyfractal commented Jan 11, 2019

polyfractal commented Feb 5, 2019

polyfractal commented Feb 5, 2019

pgomulka left a comment

Choose a reason for hiding this comment

polyfractal commented Feb 7, 2019

polyfractal Apr 15, 2019

Choose a reason for hiding this comment

polyfractal commented Apr 15, 2019

jimczi left a comment

Choose a reason for hiding this comment

$@polyfractal$ polyfractal commented Dec 4, 2018 •

edited

Loading

$@polyfractal$ polyfractal Dec 5, 2018

$@polyfractal$ polyfractal Dec 18, 2018

$@polyfractal$ polyfractal Apr 15, 2019