Skip to content
This repository has been archived by the owner on Oct 29, 2024. It is now read-only.

Fix unpacking of negative timestamps from msgpack #846

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bitoffdev
Copy link

@bitoffdev bitoffdev commented Aug 23, 2020

In #734, support was added to this python client for messagepack, but it does not match the serialization from influxdb for negative timestamps, because it interprets the timestamps as unsigned.

Influxdb uses the tinylib/msgp library to serialize timestamps. Here is an extract of how the timestamps are serialized.

Time is encoded as Unix time, which means that location (time zone) data is removed from the object. The encoded object itself is 12 bytes: 8 bytes for a big-endian 64-bit integer denoting seconds elapsed since "zero" Unix time, followed by 4 bytes for a big-endian 32-bit signed integer denoting the nanosecond offset of the time. This encoding is intended to ease portability across languages. msgp/write.go

This patch updates the python client to correctly use signed second and nanosecond fields.


Contributor checklist
  • Builds are passing
    • The only failing test is coverage, which is already failing on master. Fixing the existing issue with coverage is out of scope for this pull request. After rebasing onto master, coverage is also passing.
  • New tests have been added (for feature additions)
    • I modified an existing test to ensure that negative timestamps are parsed correctly when we get a msgpack response.

Extra Manual Verification

As a sanity check, I also tested the following script against influxdb (via Docker) before and after my changes, and the result was as expected: negative timestamps fail before this patch and work correctly following this patch.

from influxdb import InfluxDBClient
json_body = [
    {
        "measurement": "cpu_load_short",
        "tags": {
            "host": "server01",
            "region": "us-west"
        },
        "time": "1960-11-10T23:00:12.12345Z",
        "fields": {
            "value": 0.64
        }
    }
]
client = InfluxDBClient('localhost', 8086, 'root', 'root', 'example')
client.drop_database('example')
client.create_database('example')
client.write_points(json_body)
result = client.query('select value from cpu_load_short;')
assert next(result['cpu_load_short']) == {'time': '1960-11-10T23:00:12.123450Z', 'value': 0.64}
print('Negative timestamps were handled correctly!')

@bitoffdev
Copy link
Author

I rebased this PR onto master, which fixed the failing code coverage test. @sebito91, would you be able to take a look at this? My apologies for the at-mention, but I haven't been able to get any eyes on this patch. Let me know if there is a better way to request reviews for this project. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant