-
Notifications
You must be signed in to change notification settings - Fork 5.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
feat(inputs.jti_openconfig_telemetry): set timestamp from data #12730
Conversation
Allows a user to use the _timestamp from the data rather than the collection time. fixes: influxdata#12695
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.
Thanks for the PR @powersj! I have one suggestion regarding the option value and one concern regarding the type-assertion...
## Set to 'current' for time of collection, and 'data' for using the time | ||
## provided by the _timestamp field. | ||
# timestamp_source = "current" |
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.
Maybe
## Set to 'current' for time of collection, and 'data' for using the time | |
## provided by the _timestamp field. | |
# timestamp_source = "current" | |
## Set to 'collection' for time of collection, and 'data' for using the time | |
## provided by the _timestamp field. | |
# timestamp_source = "collection" |
would be more clear?
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.
I like it - done
// Iterate through data groups and add them | ||
for _, group := range dgroups { | ||
if m.TimestampSource == "data" { | ||
// OpenConfig timestamp is in milliseconds since epoch | ||
timestamp = time.UnixMilli(int64(group.data["_timestamp"].(uint64))) |
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.
I would really like to check the assertion here to prevent a panic
timestamp = time.UnixMilli(int64(group.data["_timestamp"].(uint64))) | |
ts, ok := group.data["_timestamp"].(uint64) | |
if ok { | |
timestamp = time.UnixMilli(int64(ts)) | |
} else { | |
m.Log.Warnf("invalid type %T for _timestamp %v", group.data["_timestamp"], group.data["_timestamp"]) | |
} |
As background, my experience for GNMI tells me that behavior among vendors (or even between models) is not necessarily conforming to any expectations and might vary. So better be safe than sorry... Maybe the warning could be a "warn only once" thing...
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.
agreed on type conversion - done.
Given this is opt-in, let's log each time and see if we get any issues that come through.
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
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.
Thanks for the update @powersj!
Allows a user to use the _timestamp from the data rather than the collection time.
fixes: #12695