Add a multi timeseries return format #106
Conversation
iwysiu
requested review from
a team,
sarahzinger and
njvrzm
and removed request for
a team
| frame.Meta.PreferredVisualization = data.VisTypeTrace | ||
| return data.Frames{frame}, nil | ||
| } | ||
| case FormatOptionMulti: | ||
| if frame.TimeSeriesSchema().Type == data.TimeSeriesTypeLong { |
There was a problem hiding this comment.
I'm guessing the 3-plus-year-old deprecation warning on this method was... optimistic. (There are lots of other recent uses too.)
There was a problem hiding this comment.
Yeah, the dataplane repo has a new experimental implementation of timeseries that they're working on, but it's not there yet.
| return nil, err | ||
| } | ||
| return frames.Frames(), nil | ||
| } |
There was a problem hiding this comment.
Should it be an error to specifiy FormatOptionMulti with a different type here? (I don't expect so, mostly asking for my own edification)
There was a problem hiding this comment.
No, I don't think it's an error. (Though we're not supporting wide to multi, I suspect that's not a use case we expect)
| if query.Format == FormatOptionTable { | ||
| frame.Meta.PreferredVisualization = data.VisTypeTable | ||
| return data.Frames{frame}, nil | ||
| count, err := frame.RowLen() |
There was a problem hiding this comment.
We could move this even higher, right, like right after the first error check?
|
|
||
| require.Equal(t, frame.Meta.Type, data.FrameTypeTimeSeriesLong) | ||
| require.Equal(t, frame.Meta.TypeVersion, data.FrameTypeVersion{0, 1}) | ||
| }) |
There was a problem hiding this comment.
Maybe add a test case for the error return on null values?
| if query.Format == FormatOptionTable { | ||
| frame.Meta.PreferredVisualization = data.VisTypeTable | ||
| return data.Frames{frame}, nil | ||
| count, err := frame.RowLen() |
| frame.Meta.PreferredVisualization = data.VisTypeTrace | ||
| return data.Frames{frame}, nil | ||
| } | ||
| case FormatOptionMulti: | ||
| if frame.TimeSeriesSchema().Type == data.TimeSeriesTypeLong { |
There was a problem hiding this comment.
Yeah, the dataplane repo has a new experimental implementation of timeseries that they're working on, but it's not there yet.
| return nil, err | ||
| } | ||
| return frames.Frames(), nil | ||
| } |
There was a problem hiding this comment.
No, I don't think it's an error. (Though we're not supporting wide to multi, I suspect that's not a use case we expect)
|
|
||
| require.Equal(t, frame.Meta.Type, data.FrameTypeTimeSeriesLong) | ||
| require.Equal(t, frame.Meta.TypeVersion, data.FrameTypeVersion{0, 1}) | ||
| }) |
There was a problem hiding this comment.
Ok 2 things:
- does something being a log, trace, or table affect it's long/wide/multi-ness? I am concerned we might be losing preferred visualizations potentially? It seems like were kind of combining concepts that maybe shouldn't be combined? Maybe frame.TimeSeriesSchema().Type should equal multi rather than adding this to the formatoptiontypes the way we have TimeSeriesTypeLong? Feel free to tell me I'm wrong about that haha.
- I think we should make tests here for all the other option combos. Sorry I know it's annoying, but it seems really easy to accidentally make a breaking change when it's like an if case inside a switch case kind of thing.
There was a problem hiding this comment.
Hi @sarahzinger ! I went over your questions, but I was a little confused on some of them so it may make sense for us to do a quick meeting to go over it at some point? (Sorry, I think it’s Monday morning brain)
- Logs and traces are returned in the format that they’re originally passed in from the datasource in (which the code assumes is time series long). I’m a little confused on the losing preferred visualization question, logs and traces I think still expect longs, and table will display all of them. The multi timeseries format existed before this change, and visualizations already had to account for them. I would be down if we wanted to have wide vs multi as subformats for timeseries, it may be easier for users to follow
- Just double checking my understanding: you mean having tests for the timeseries, table, logs, and trace format? I can add those.
Add support for returning data in the multi format for timeseries. The plan has shifted a bit: instead of return the multi format for timeseries instead of wide (what's described in the issue) I'm instead adding a multi return format for users who want a timeseries format for data with a lot of labels that don't have points for every time. This avoids creating a massive change for other users of the library.
Part of #96