feature/add_influx_timestamp #24
Conversation
…nds. Convert to nanoseconds
| tags = encode_tags(tags) | ||
| fields = encode_fields(fields) | ||
| [header, encode_key(name), tags, ?\s, fields] | ||
|
|
||
| case is_nil(unix_timestamp_ms) do |
There was a problem hiding this comment.
if is a macro that just compiles down to a case statement, just went with the case instead.
There was a problem hiding this comment.
if would be more expressive in this case.
There was a problem hiding this comment.
@jbavari then why not use if if this will be the same after compilation? If you would use Credo it would warn you that this is bad practice.
|
|
||
| def write(measurement, tags, fields) when is_list(fields) do | ||
| def write(measurement, tags, fields, timestamp_milli_secs) when is_list(fields) do |
There was a problem hiding this comment.
That would be a braking change. Why not add default value for timestamp_milli_secs?
There was a problem hiding this comment.
@hauleth is right—we need to have a default value, otherwise all existing code will be broken.
There was a problem hiding this comment.
Thanks @jbavari and @ebostijancic.
It is definitely the great first step towards adding timestamp support.
Millisecond precision choice seems arbitrary, though for some systems nanosecond precision is vital.
I think we should go with the way similar to DateTime.from_unix/2 for example, that is having integer value and its unit.
|
|
||
| def write(measurement, tags, fields) when is_list(fields) do | ||
| def write(measurement, tags, fields, timestamp_milli_secs) when is_list(fields) do |
There was a problem hiding this comment.
@hauleth is right—we need to have a default value, otherwise all existing code will be broken.
| tags = encode_tags(tags) | ||
| fields = encode_fields(fields) | ||
| [header, encode_key(name), tags, ?\s, fields] | ||
|
|
||
| case is_nil(unix_timestamp_ms) do |
There was a problem hiding this comment.
if would be more expressive in this case.
|
edited: I've played around with this too and you can get a nanosecond timestamp with System.os_time() I think it makes the most sense to require passing through a nanosecond timestamp from the client |
@lexmag - first of all, thanks for this awesome library!
This PR addresses an issue posted here: #6. I saw the issue was tagged as 'help wanted', so here's your help!
@ebostijancic did all the hard work, I just deviated from his work a bit to specify the timestamp passed in as milliseconds and then converted to nanoseconds. He gets all the credit!
InfluxDB offers the ability to place items in the past with timestamps as per the line protocol reference.
Thanks in advance!