changed time format to no longer lose microseconds #3065
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
octo
requested changes
Feb 10, 2019
There was a problem hiding this comment.
Hi @btoneill, thank you very much for your PR! Also thank you for providing that reference.
Support for higher resolution time stamps appears to be new in RRDtool 1.1, and we need to (continue to) support the previous versions. Can you please make this configurable, with the second-precision time format being the default?
Also, you're change log entry has it backwards: this adds support for microsecond resolution, it says "no longer use" which is incorrect.
Thanks and best regards,
—octo
|
Doesn't rrdcached itself require rrdtool 1.4?
I'll get the log line fixed.
Thanks,
Brian
…On Sun, Feb 10, 2019, 1:04 AM Florian Forster ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Hi @btoneill <https://github.com/btoneill>, thank you very much for your
PR! Also thank you for providing that reference.
Support for higher resolution time stamps appears to be new in RRDtool
1.1, and we need to (continue to) support the previous versions. Can you
please make this configurable, with the second-precision time format being
the default?
Also, you're change log entry has it backwards: this *adds* support for
microsecond resolution, it says "no longer use" which is incorrect.
Thanks and best regards,
—octo
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3065 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ANB2mqwhtr8BhHiMzFhq7In-QbZu9bPpks5vL8RxgaJpZM4awoCs>
.
|
|
Hey, I didn't see that the email you referenced was from 2003. If this feature has been in RRDtool for ~16 years, then the option is not required. Since you didn't update the description yet, I'm going to go ahead and do that. Thanks and best regards, |
octo
reviewed
Feb 10, 2019
src/rrdcached.c
Outdated
| status = snprintf(buffer, buffer_len, "%lu", (unsigned long)t); | ||
| t = CDTIME_T_TO_DOUBLE(vl->time); | ||
|
|
||
| status = snprintf(buffer, buffer_len, "%f", t); |
There was a problem hiding this comment.
The PR description and the referenced email both talk about microseconds specifically, but this format may in fact be more precise than that. Please print as %.6f instead.
|
My understanding of the cdtime_t is that it only goes to microseconds. If I
am misunderstanding I can change it.
From utils_time.h:
/*
* "cdtime_t" is a 64bit unsigned integer. The time is stored at a 2^-30
second
* resolution, i.e. the most significant 34 bit are used to store the time
in
* seconds, the least significant bits store the sub-second part in
something
* very close to nanoseconds. *The* bing time in this
* manner is that comparing times and calculating differences is as simple
as
* it is with "time_t", i.e. a simple integer comparison / subtraction
works.
*/
…On Sun, Feb 10, 2019, 2:32 PM Florian Forster ***@***.***> wrote:
***@***.**** commented on this pull request.
Once the inline comment is fixed this is good to go. Thanks @btoneill
<https://github.com/btoneill>!
------------------------------
In src/rrdcached.c
<#3065 (comment)>:
> @@ -74,8 +74,9 @@ static int value_list_to_string(char *buffer, int buffer_len,
memset(buffer, '\0', buffer_len);
- t = CDTIME_T_TO_TIME_T(vl->time);
- status = snprintf(buffer, buffer_len, "%lu", (unsigned long)t);
+ t = CDTIME_T_TO_DOUBLE(vl->time);
+
+ status = snprintf(buffer, buffer_len, "%f", t);
The PR description and the referenced email both talk about microseconds
specifically, but this format may in fact be more precise than that. Please
print as %.6f instead.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3065 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ANB2muCUKumZ-WiT-a2elWLBT8R5nmAbks5vMIHngaJpZM4awoCs>
.
|
One microsecond is 1000 nanoseconds. |
|
I think I have updated with the requested change. |
octo
requested changes
Feb 11, 2019
src/rrdcached.c
Outdated
| @@ -74,8 +74,9 @@ static int value_list_to_string(char *buffer, int buffer_len, | |||
|
|
|||
| memset(buffer, '\0', buffer_len); | |||
|
|
|||
| t = CDTIME_T_TO_TIME_T(vl->time); | |||
| status = snprintf(buffer, buffer_len, "%lu", (unsigned long)t); | |||
| t = CDTIME_T_TO_DOUBLE(vl->time); | |||
There was a problem hiding this comment.
t has the wrong type now (should be double, is time_t. I'd recommend to remove t completely and inline the conversion:
status = snprintf(buffer, buffer_len, "%.6f", CDTIME_T_TO_DOUBLE(vl->time));|
Do, i forgot that in my change there. My original change was in a different
branch and I copied it over.
I'l fix.
…On Mon, Feb 11, 2019 at 10:57 AM Florian Forster ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Hi @btoneill <https://github.com/btoneill>,
the continuous integration is unhappy because there's a type mismatch.
I've left more information inline. Also, the %.6f format did not yet make
it to the repo.
Best regards,
—octo
------------------------------
In src/rrdcached.c
<#3065 (comment)>:
> @@ -74,8 +74,9 @@ static int value_list_to_string(char *buffer, int buffer_len,
memset(buffer, '\0', buffer_len);
- t = CDTIME_T_TO_TIME_T(vl->time);
- status = snprintf(buffer, buffer_len, "%lu", (unsigned long)t);
+ t = CDTIME_T_TO_DOUBLE(vl->time);
t has the wrong type now (should be double, is time_t. I'd recommend to
remove t completely and inline the conversion:
status = snprintf(buffer, buffer_len, "%.6f", CDTIME_T_TO_DOUBLE(vl->time));
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3065 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ANB2mvNKs62FnXc5oYnUEqus-wRQGuBXks5vMaEWgaJpZM4awoCs>
.
|
octo
previously approved these changes
Feb 19, 2019
|
Thanks again for your PR @btoneill! |
…when sending to rrdcached. See https://lists.oetiker.ch/pipermail/rrd-developers/2003-March/001007.html for we want this
|
Rebased on master |
octo
approved these changes
Feb 20, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ChangeLog: rrdcached plugin: Time resolution has been improved to microseconds.
Have been sending an int to rrdcached for the epoch value, it supports microseconds. You want to use microseconds.
See https://lists.oetiker.ch/pipermail/rrd-developers/2003-March/001007.html for we want this