-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Graphite logs in seconds, not milliseconds #2080
Conversation
+1. I don't know if Graphite ever sends fractional timestamps, but this should work if the timestamps are always integers. I guess we can revisit if it does. |
@@ -94,7 +94,7 @@ func (p *Parser) Parse(line string) (influxdb.Point, error) { | |||
return influxdb.Point{}, err | |||
} | |||
|
|||
timestamp := time.Unix(0, unixTime*int64(time.Millisecond)) | |||
timestamp := time.Unix(unixTime, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested sending fractional seconds to a real graphite instance and it does accept them. Don't think this is fully correct as is. Probably need to use strconv.ParseFloat
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I had this vague memory of seeing Graphite data with fractional timestamps. Thanks for looking into it @jwilder
So the way graphite apparently works is depending on how you set a retention policy, it can either average your points, or use the last point, for the "second" you wrote data. However, it can accept fractional seconds. So, to make our endpoint compatible, I'm accepting fractional time, and storing it raw (not averaging or taking the last). So, our behavior isn't exactly as graphite will behave, but I believe this is the correct behavior so that we don't lose data. |
@@ -89,12 +89,21 @@ func (p *Parser) Parse(line string) (influxdb.Point, error) { | |||
fieldValues[name] = v | |||
|
|||
// Parse timestamp. | |||
unixTime, err := strconv.ParseInt(fields[2], 10, 64) | |||
//unixTime, err := strconv.ParseInt(fields[2], 10, 64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you actually remove this line?
Change makes sense, but I do have some feedback. +1 from me once those changes are in place. |
a3e590b
to
6cc3cda
Compare
Refactors complete based on comments. Merging on green. |
6cc3cda
to
f5d38dd
Compare
Graphite logs in seconds, not milliseconds
Was incorrectly decoding as milliseconds, should be seconds.
Fixes #2077.