SEARCH-8423 Fix time field type #34
Merged
Conversation
This file contains 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
dcheckoway
commented
Jan 27, 2025
| @@ -90,6 +90,8 @@ func makeEmptyConcreteTypeArray(val interface{}) (interface{}, error) { | |||
| return []float64{}, nil | |||
| case bool: | |||
| return []bool{}, nil | |||
| case time.Time: | |||
| return []time.Time{}, nil | |||
There was a problem hiding this comment.
This is really key. As you populate fields in a data frame, Grafana's data layer infers the field type from this array type.
dcheckoway
commented
Jan 27, 2025
| nanoSec := int64(math.Round((seconds-float64(wholeSec))*1000.0)) * 1000000 // ms precision | ||
| return true, time.Unix(wholeSec, nanoSec).UTC().Format(time.RFC3339Nano) | ||
| nanoSec := int64(math.Round((seconds-float64(wholeSec))*1000000.0)) * 1000 // microsec precision | ||
| return true, time.Unix(wholeSec, nanoSec).UTC() |
There was a problem hiding this comment.
NOTE: I changed this to microsec because we can, and it provides additional granularly should it be needed. I tried nanoseconds and was getting some floating point precision garbage (i.e. 122999 instead of 123000), so I figured this is good enough for ~100% of the likely scenarios.
itsalvinho
approved these changes
Jan 27, 2025
natecanfield822
approved these changes
Jan 27, 2025
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.
What Was Wrong
Grafana was saying
Data is missing a time fieldand wouldn't let you graph the data as a time series (or anything related to time). It was confusing, since clearly there's a Time field with valid timestamps.We were passing back the
timefield as an ISO string...which was more or less fine, but we were inadvertently setting the field type tostring-- implicitly because of the value's native type. Apparently the nametimealone and the ISO format wasn't enough for Grafana to recognize it.And the real problem is: Grafana's Go
datalibrary doesn't let you explicitly set thetypeon a field. This is kinda crummy API design, but they're trying to make it easy -- just set the value and the data layer figures out what it is. But when we pass a string, it thinks the type isstring.What's Changing
Use the
time.Timestruct so Grafana'sdatalayer knows that it's actually time.To Verify
Run a query that produces
timestats(or anything with time, really) and try to add it to a dashboard as a Time Series visualization.