Fixing DateProcessor when the format is epoch_millis

[masseyke]

As described in https://discuss.elastic.co/t/difference-between-unix-ms-and-epoch-millis-in-date-processor/333048, if you create a DateProcessor with a format of epoch_milllis, we oddly capture the year and nanoseconds for @timestamp, but not the month, day, hour, second, etc. It looks like the problem is that problem is that the ChronoField that we get from the DateFormatter created with epoch_millis that has all of that information is INSTANT_SECONDS. We don't look at INSTANT_SECONDS though, so all of that is lost.

[elasticsearchmachine]

Hi @masseyke, I've created a changelog YAML for you.

[joegallo]

I'd love to see something like along the lines of this test (with a real name, of course), and maybe using some fixed moment in time rather than .now():

joegallo@galactic:~/Code/elastic/elasticsearch $ git diff diff --git a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/DateFormatTests.java b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/DateFormatTests.java index f7c94e24881..70e9aa9f777 100644 --- a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/DateFormatTests.java +++ b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/DateFormatTests.java @@ -10,6 +10,7 @@ package org.elasticsearch.ingest.common; import org.elasticsearch.common.time.DateFormatter; import org.elasticsearch.common.time.DateUtils; +import org.elasticsearch.common.time.FormatNames; import org.elasticsearch.test.ESTestCase; import java.time.ZoneId; @@ -203,4 +204,21 @@ public class DateFormatTests extends ESTestCase { assertThat(DateFormat.fromString("tai64n"), equalTo(DateFormat.Java)); assertThat(DateFormat.fromString("prefix-" + randomAlphaOfLengthBetween(1, 10)), equalTo(DateFormat.Java)); } + + public void testFoobar() { + ZonedDateTime now = ZonedDateTime.now(ZoneOffset.UTC); + + for (FormatNames formatName : FormatNames.values()) { + String name = formatName.getName(); + + DateFormatter formatter = DateFormatter.forPattern(name); + String formatted = formatter.format(now); + + DateFormat dateFormat = DateFormat.fromString(name); + ZonedDateTime parsed = dateFormat.getFunction(name, ZoneOffset.UTC, Locale.ROOT).apply(formatted); + + String formatted2 = formatter.format(parsed); + assertThat(name, formatted2, equalTo(formatted)); + } + } }

The test in DateProcessorTests is certainly nice for a regression test, but I'd love to see something a little more proactive, too. My read of things is that the above generic test would have failed on this (well, technically on epoch_second, but for the same reason), so we could have caught this earlier if we'd had a test along these lines.

Of course that's still only testing all the named formats from FormatNames, and it's entirely possible we still have bugs if somebody supplies a non-named format String.

[elasticsearchmachine]

Pinging @elastic/es-data-management (Team:Data Management)

[joegallo]

The change looks reasonable, and you've added both a regression test for this specific issue and a more generic test for this class of issues.

Ship it!