segmentio / analytics-kotlin

The hassle-free way to add Segment analytics to your Kotlin app (Android/JVM).
MIT License
45 stars 27 forks source link

Timestamp is formatting milliseconds incorrectly #200

Closed pcasaes closed 11 months ago

pcasaes commented 11 months ago

Describe the bug

When converting the unix timestamp in milliseconds 1700617928023 to ISO we are seeing 2023-11-22T01:52:08.23Z

But it should be 2023-11-22T01:52:08.023Z

To Reproduce Steps to reproduce the behavior:

From the code in dateTimeNowString

   val sdf = SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'.'Szzz", Locale.ROOT)
    val utc = TimeZone.getTimeZone("UTC");
    sdf.timeZone = utc;
    println(sdf.format(Date(1700617928023L)).replace("UTC", "Z"))

Expected behavior The output should always present 3 digits for milliseconds.

Platform (please complete the following information):

Additional context A quick fix would be to use the following pattern yyyy-MM-dd'T'HH:mm:ss'.'SSSzzz

But always instantiating a formatter most likely has performance implications.

Maybe use javax.time

Instant
                        .now()
                        .truncatedTo(ChronoUnit.MILLIS) // this guarantees only 3 decimals places
                        .toString()
wenxi-zeng commented 11 months ago

hi @pcasaes, thanks for reporting this issue. we will take a look. we purposely walked away from javax.time because it requires desugaring to some customers even though they don't need it. we felt it's a bad experience to ask users to add something they don't want just because they use our library. so we'll see if we have other options to fix this.

wenxi-zeng commented 11 months ago

@pcasaes this issue is fixed in 1.14.1. please have a try. we ended up with your quick fix, but made the formatter a property of a singleton to avoid instantiating it every time. this should eliminate the overhead (with trade off of negligible memory of course).

pcasaes commented 11 months ago

Hi, I took a look at the code and I see that the SimpleDateFormat instance is being reused. But that class is notoriously not thread safe. Has this been considered?

https://docs.oracle.com/javase/8/docs/api/java/text/SimpleDateFormat.html

Date formats are not synchronized. It is recommended to create separate format instances for each thread. If multiple threads access a format concurrently, it must be synchronized externally.

pcasaes commented 11 months ago

btw. I miswrote. Instant is from java.time not javax.time.

Instant
                        .now()
                        .truncatedTo(ChronoUnit.MILLIS) // this guarantees only 3 decimals places
                        .toString()

It's been available since Java 8.

wenxi-zeng commented 11 months ago

@pcasaes that's a good call out! no wonder people are staying away from SimpleDateFormat. we will have it fixed asap.

for Instant, please check out here. though Java 8+ APIs are supported, it requires desugaring in order to work in old platforms

wenxi-zeng commented 11 months ago

@pcasaes we made it thread safe in 1.14.2. please have a try.