Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Time is serialized not consistently in MsgPack and Json, missing millis and nanos in MsgPack #517

Closed
rhajek opened this issue Sep 10, 2018 · 3 comments
Milestone

Comments

@rhajek
Copy link
Contributor

rhajek commented Sep 10, 2018

It is not possible to simply switch json and mspack response encoding implementation. Json (Mochi) serializes time as a String in RFC but MsgPack encodes time into Long. Miliseconds/Nanoseconds are lost in the case of MessagePack. It looks like bug in MsgPack encoder on server side .

If the EPOCH param is specified in query, ex. client.query(query,TimeUnit.NANOSECONDS) then json encodes time as Double (why not long?) and MsgPack as a Long (that looks ok).

Another issue is that there is no way how to get time with milis/nanos precission with async streaming in MsgPack. There is no async API with EPOCH parameter.

Here is failing test:

package org.influxdb.msgpack;

import java.time.Instant;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.TimeUnit;
import okhttp3.OkHttpClient;
import org.influxdb.InfluxDB;
import org.influxdb.InfluxDBFactory;
import org.influxdb.dto.BoundParameterQuery;
import org.influxdb.dto.Point;
import org.influxdb.dto.Query;
import org.junit.Assert;
import org.junit.jupiter.api.Test;

class MsgPackTimestampTest {


    @Test
    void testMsgPackJsonCompatibility() throws Exception {

        OkHttpClient.Builder builder = new OkHttpClient.Builder();
        InfluxDB clientJson = InfluxDBFactory.connect("http://127.0.0.1:8086", "admin", "admin", builder, InfluxDB.ResponseFormat.JSON);
        InfluxDB clientMsgPack = InfluxDBFactory.connect("http://127.0.0.1:8086", "admin", "admin", builder, InfluxDB.ResponseFormat.MSGPACK);

        //JSON response encoding
        Object jsonTime = executeTimeQuery(clientJson);
        System.out.println(jsonTime + " " + jsonTime.getClass() + " - JSON");
        Instant instant = Instant.parse(jsonTime.toString());
        Assert.assertEquals(1485273600L,instant.getEpochSecond());
        Assert.assertEquals(100,instant.getNano());

        //MsgPack response encoding
        Object msgPackTime = executeTimeQuery(clientMsgPack);
        //java.lang.Long in epoch seconds
        System.out.println(msgPackTime + " " + msgPackTime.getClass() + " - MSGPACK");

        ///this should be the same!!!
        Assert.assertEquals(jsonTime, msgPackTime);
    }


    private Object executeTimeQuery(InfluxDB client) throws Exception {

        String dbName = "precision_unittest_" + System.currentTimeMillis();
        client.createDatabase(dbName);
        client.setDatabase(dbName);
        String measurement = "test_measurement"+System.currentTimeMillis();

        long t1 = 1485273600000000100L;
        Point p1 = Point
                .measurement(measurement)
                .addField("foo", 1d)
                .tag("device", "one")
                .time(t1, TimeUnit.NANOSECONDS).build(); // 2017-01-27T16:00:00.000000100Z
        client.write(p1);

        Thread.sleep(1000);

        // List to be filled
        List<Object> entries = new ArrayList<>();

        BoundParameterQuery.QueryBuilder queryBuilder = BoundParameterQuery
                .QueryBuilder.newQuery("Select * from "+measurement).forDatabase(dbName);

        Query query = queryBuilder.create();

        //get time value
        Object timeObject = client.query(query).getResults()
                .get(0).getSeries().get(0).getValues().get(0).get(0);
        client.deleteDatabase(dbName);

        // return entries to the UI.
        return timeObject;
    }
}
@lxhoan
Copy link
Contributor

lxhoan commented Sep 11, 2018

Hi @rhajek ,
If we want to query timestamp value in nano, need EPOCH param

client.query(query, TimeUnit.NANOSECONDS).getResults().get(0).getSeries().get(0).getValues().get(0).get(0);

If the EPOCH param is specified in query, ex. client.query(query,TimeUnit.NANOSECONDS) then json encodes time as Double (why not long?) and MsgPack as a Long (that looks ok).

JSON parser have no idea if a string representation of a number is encoding an integer or a floating point value (However MessagePack does since MessagePack encodes type of value element as well). So most of JSON parsers (I know at least Moshi and Jackson) decide to decode number as double value by default (I see from Moshi 1.6 we can have custom adapter but I haven't tried it)

Another issue is that there is no way how to get time with milis/nanos precission with async streaming in MsgPack. There is no async API with EPOCH parameter.

Not only MsgPack, there is already an open issue for this (issue #491)

@rhajek
Copy link
Contributor Author

rhajek commented Sep 11, 2018

If the EPOCH is specified in query, MsgPack timestamp is encoded to long correctly.

If we want to query timestamp value in nano, need EPOCH param
yes but, it is not necessary
see:
https://docs.influxdata.com/influxdb/v1.6/guides/querying_data/
Everything in InfluxDB is stored and reported in UTC. By default, timestamps are returned in RFC3339 UTC and have nanosecond precision, for example 2015-08-04T19:05:14.318570484Z

client.query(Query) without EPOCH should also always return time in RFC (with nanosecods, if they are written in DB)

Server sends timestamp encoded in MspPack EXTENSION type, but it looks like miliseconds and nonos are lost... I am not sure if the problem is in java msgpack library on or on server side.

 case EXTENSION:
        final byte msgPackTimeExtType = (byte) 5;
        final int timeOffset = 0;
        final int timeByteArrayLength = 8;
        extension = unpacker.unpackExtensionTypeHeader();
        if (extension.getType() == msgPackTimeExtType) {
          dst = new byte[extension.getLength()];
          unpacker.readPayload(dst);
          o = ByteBuffer.wrap(dst, timeOffset, extension.getLength()).getLong();  ## <- milis/nanos are not here

@lxhoan
Copy link
Contributor

lxhoan commented Sep 11, 2018

Not sure if I am fully understand you last comment
however feel free to refer to this GoLang msgpack library
https://github.com/tinylib/msgp/blob/master/msgp/write.go#L594

Time is encoded in 12 bytes so maybe what you need is lying in the remaining 4 byte ?

lxhoan added a commit to bonitoo-io/influxdb-java that referenced this issue Sep 11, 2018
lxhoan added a commit to bonitoo-io/influxdb-java that referenced this issue Sep 11, 2018
majst01 added a commit that referenced this issue Sep 11, 2018
fix issue #517 : missing millis and nanos in MsgPack
@ivankudibal ivankudibal added this to the 2.13 milestone Sep 12, 2018
ivankudibal added a commit that referenced this issue Sep 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants