remove FastDateFormat #365

Jimmy-653 · 2019-10-09T06:14:33Z

Replace FastDateFormat with DateTimeFormatter and Instant from java.time package

philsttr

Thanks for the PR! I really appreciate contributions.

Unfortunately, I cannot accept these changes as-is. This change has removed the ability for logstash-logback-encoder to output timestamps as a json number (for number of milliseconds since unix epoch), and to output timestamps as a json string (representing the number of milliseconds since unix epoch), as documented here. Both of these are backwards-incompatible changes. I would rather not remove this functionality.

The switch from FastDateFormat to DateTimeFormatter is also a backwards-incompatible change due to the subtle differences between the patterns used by the two formatters, but I'm willing to accept this change in the next major release of logstash-logback-encoder. However, since logback does not support higher-precision timestamps yet, there is no rush to including this change in logstash-logback-encoder yet.

philsttr · 2019-10-11T16:18:55Z

src/main/java/net/logstash/logback/composite/FormattedTimestampJsonProvider.java

@@ -160,11 +96,11 @@ protected String getFormattedTimestamp(Event event) {
     */
    private void updateTimestampWriter() {
        if (UNIX_TIMESTAMP_AS_NUMBER.equals(pattern)) {
-            timestampWriter = new NumberTimestampWriter();
+            timestampWriter = null;


This is a backwards incompatible change that I won't be able to accept.

logstash-logback-encoder still needs to optionally support writing timestamps as numbers indicating the number of milliseconds since unix epoch

For this one, you have to take a close look at the overall change, or maybe I should remove UNIX_TIMESTAMP_AS_NUMBER completely because currently its only used in 2 test cases for it....
However, what was done previously, you would take the timestamp as milliseconds, convert them to a String, and then write it to the Json (inside the NumberTimestampWriter).

So actually im not using it anymore, I will remove it to clear confusion.
(Nothing lost here)

We cannot remove the UNIX_TIMESTAMP_AS_NUMBER, or the NumberTimestampWriter.

The FormattedTimestampJsonProvider supports three scenarios:

UNIX_TIMESTAMP_AS_NUMBER will write the timestamp value as a json number indicating the number of milliseconds since unix epoch, like this:

{ "@timestamp" : 1570901563 }

UNIX_TIMESTAMP_AS_STRING will write the timestamp value as a json string indicating the number of milliseconds since unix epoch, like this:

{ "@timestamp" : "1570901563" }

And any other pattern will be used as a format string. e.g.

{ "@timestamp" : "2011-12-03T10:15:30+01:00" }

From what I can tell, the new code always writes the timestamp value as a json string, which has effectively removed support for UNIX_TIMESTAMP_AS_NUMBER.

philsttr · 2019-10-11T16:20:40Z

src/main/java/net/logstash/logback/composite/FormattedTimestampJsonProvider.java

        } else if (UNIX_TIMESTAMP_AS_STRING.equals(pattern)) {
-            timestampWriter = new StringTimestampWriter();
+            timestampWriter = DateTimeFormatter.ofPattern(DEFAULT_PATTERN).withZone(TimeZone.getDefault().toZoneId());


This is a backwards incompatible change that I won't be able to accept.

logstash-logback-encoder still needs to optionally support writing timestamps as strings indicating the number of milliseconds since unix epoch

I missunderstood the StringTimestampWriter, I will take a second look

philsttr · 2019-10-11T16:29:41Z

src/main/java/net/logstash/logback/composite/FormattedTimestampJsonProvider.java

        } else {
-            timestampWriter = new PatternTimestampWriter(FastDateFormat.getInstance(pattern, timeZone));
+            timestampWriter = DateTimeFormatter.ofPattern(pattern, Locale.getDefault()).withZone(TimeZone.getDefault().toZoneId());


Since there are subtle differences in the pattern strings used by DateTimeFormatter and FastDateFormat, this is a backwards incompatible change. I'm willing to accept this change, but this will need a major version bump of logstash-logback-encoder.

I'll probably hold off on this until logback supports timestamps with higher precision.

philsttr · 2019-10-11T16:32:16Z

src/test/java/net/logstash/logback/encoder/LogstashAccessEncoderTest.java

@@ -200,7 +200,7 @@ public void unixTimestampAsNumber() throws Exception {

        JsonNode node = MAPPER.readTree(encoded);

-        assertThat(node.get("@timestamp").numberValue()).isEqualTo(timestamp);
+        assertThat(node.get("@timestamp").asLong()).isEqualTo(timestamp);


Per my previous comment, this change will need to be reverted to ensure backwards compatibility. The json node should be a number value, not a string value that needs to be converted to a Long

Sorry, Long is a type of Number? Its just the correct one already.

They are not quite the same.

numberValue() will return the number if and only if the JsonNode represents a JSON number. This is the expected condition of this test case.

asLong() will convert String JsonNode values to a number. In this test case, the JsonNode is not expected contain a String value, so the test would incorrectly pass if the json node contained a string value.

In other words, the test should only pass if the json looks like this:

{ "@timestamp" : 1570901563 }

The test should fail if the json looks like this:

{ "@timestamp" : "1570901563" }

philsttr · 2019-10-11T16:34:49Z