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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Timestamp points before queuing to preserve original time #211

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@vassilevsky
Contributor

vassilevsky commented Dec 1, 2017

Hi 馃枛

I thought about all these points that hang out in the queue while it is being filled. If the user did not specify timestamp, then they are stamped with server time upon arrival. This is not strictly very precise, eh?

I don't like the need for a separate method. Can something be done to avoid it?

WDYT? 馃

@dmke

This comment has been minimized.

Show comment
Hide comment
@dmke

dmke Dec 2, 2017

Member

This looks interesting. Two immediate thougts:

  1. I'm not a fan of mutating methods (i.e. writer.preprocess(data) modifying data) for pure readability reasons. preprocess should instead return a new array.
  2. The preprocessing should be independant of the writer (by moving it into the InfluxDB::Client class) and it should be opt-in (i.e. with a config option).
Member

dmke commented Dec 2, 2017

This looks interesting. Two immediate thougts:

  1. I'm not a fan of mutating methods (i.e. writer.preprocess(data) modifying data) for pure readability reasons. preprocess should instead return a new array.
  2. The preprocessing should be independant of the writer (by moving it into the InfluxDB::Client class) and it should be opt-in (i.e. with a config option).
@kaspergrubbe

This comment has been minimized.

Show comment
Hide comment
@kaspergrubbe

kaspergrubbe Jul 8, 2018

I was digging into the source code to understand what happens in the case of setting the clients timeprecision to ns but leaving it out when writing data, I think the README covers it by:

If you write data points with sub-second resolution, you have to configure your client instance with a more granular time_precision option and you need to provide timestamp values which reflect this precision. If you don't do this, your points will be squashed!

I believe @vassilevsky changes are very needed, I mean, why specify it in the client if the writes are ignoring this setting?

This forces us to write a wrapper class that handles this, but it would be nice if the client by itself did it.

The preprocessing should be independant of the writer (by moving it into the InfluxDB::Client class) and it should be opt-in (i.e. with a config option).

Couldn't the opt-in be setting the timeprecision setting to sub-second values? Are there a case for having the client set with a sub-second timeprecision without wanting sub-second precision?

kaspergrubbe commented Jul 8, 2018

I was digging into the source code to understand what happens in the case of setting the clients timeprecision to ns but leaving it out when writing data, I think the README covers it by:

If you write data points with sub-second resolution, you have to configure your client instance with a more granular time_precision option and you need to provide timestamp values which reflect this precision. If you don't do this, your points will be squashed!

I believe @vassilevsky changes are very needed, I mean, why specify it in the client if the writes are ignoring this setting?

This forces us to write a wrapper class that handles this, but it would be nice if the client by itself did it.

The preprocessing should be independant of the writer (by moving it into the InfluxDB::Client class) and it should be opt-in (i.e. with a config option).

Couldn't the opt-in be setting the timeprecision setting to sub-second values? Are there a case for having the client set with a sub-second timeprecision without wanting sub-second precision?

@kaspergrubbe

This comment has been minimized.

Show comment
Hide comment
@kaspergrubbe

kaspergrubbe Jul 8, 2018

@vassilevsky There seems to be a bit of a problem with how #clock_gettime deals with input. If it gets passed nil it will still work, so we might want to throw an error in case someone passes a time_precision that are not understood.

kaspergrubbe commented Jul 8, 2018

@vassilevsky There seems to be a bit of a problem with how #clock_gettime deals with input. If it gets passed nil it will still work, so we might want to throw an error in case someone passes a time_precision that are not understood.

@kaspergrubbe

This comment has been minimized.

Show comment
Hide comment
@kaspergrubbe

kaspergrubbe Jul 9, 2018

Hi @vassilevsky, I tried to use your code on OSX to test with, and the result didn't seem correct to me, I have written about it here: #219 , do you know if I am doing anything wrong?

kaspergrubbe commented Jul 9, 2018

Hi @vassilevsky, I tried to use your code on OSX to test with, and the result didn't seem correct to me, I have written about it here: #219 , do you know if I am doing anything wrong?

@vassilevsky

This comment has been minimized.

Show comment
Hide comment
@vassilevsky

vassilevsky Jul 22, 2018

Contributor

Thank you for feedback. I posted this as an idea in the first place, to get the conversation going.

Sadly, I don't have energy to make a polished solution. I switched from Ruby to BEAM languages recently. Feel free to use my code or roll your own.

Contributor

vassilevsky commented Jul 22, 2018

Thank you for feedback. I posted this as an idea in the first place, to get the conversation going.

Sadly, I don't have energy to make a polished solution. I switched from Ruby to BEAM languages recently. Feel free to use my code or roll your own.

@dmke dmke closed this in 72cfa27 Aug 23, 2018

@dmke

This comment has been minimized.

Show comment
Hide comment
@dmke

dmke Aug 23, 2018

Member

Oops. This was accidentally closed by a typo. The commit 72cfa27 ought to have referenced #221, not #211.

Member

dmke commented Aug 23, 2018

Oops. This was accidentally closed by a typo. The commit 72cfa27 ought to have referenced #221, not #211.

@dmke dmke reopened this Aug 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment