Skip to content
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

Dataplane: Support timeSeriesLong without transform #62732

Merged

Conversation

bohandley
Copy link
Contributor

@bohandley bohandley commented Feb 1, 2023

Fixes #57543

This work is part of the Dataplane project and is a WIP.

What is this feature?
This work applies a transform automatically to data of type TimeSeriesLong when viewed in a Time Series panel. The transformation applied partitions rows based on fields of type string and uses these as labels for the numeric fields.

Who is this feature for?
This feature is for anyone who is using a tabular datasource like SQL and is applying a transform to visualize the data as time series.

Why do we need this feature?
If a series is of type TimeSeriesLong it requires a transformation. If it has fields of type string these fields could be represented as labels for fields of numeric data. We don't want users to have to apply a transformation themselves.

TimeSeriesLong not supported
Screen Shot 2023-02-01 at 5 04 39 PM

TimeSeriesLong, supported, partitioned with labels
Screen Shot 2023-02-01 at 5 01 50 PM

Use this debug snapshot to test
debug-Panel Title-2023-02-24 17_37_52.json.txt

@bohandley bohandley added area/transformations area/panel/timeseries The main time series Graph panel area/dataplane Dataplane Project (Data type contract) labels Feb 1, 2023
@bohandley bohandley added this to the 10.0.0 milestone Feb 1, 2023
@@ -488,6 +489,27 @@ export class PanelStateWrapper extends PureComponent<Props, State> {
// Yes this is called ever render for a function that is triggered on every mouse move
this.eventFilter.onlyLocal = dashboard.graphTooltip === 0;

if (PanelComponent.name === 'TimeSeriesPanel' && data.series.every((df) => df.meta?.type === 'timeseries-long')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for sure not the right place for this.... if just TimeSeriesPanel.... maybe:

https://github.com/grafana/grafana/blob/main/public/app/plugins/panel/timeseries/utils.ts#L21

or perhaps in the prepareTimeSeries(...) transformer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prepareTimeSeries(...) transformer works too! It was suggested by @leeoniya that I use partitionByValues(...) but I am open to both suggestions. @ryantxu and @leeoniya how much different are these? Partition by Values creates a wide dataframe and Prepare Time Series allows you to choose wide or multi. According to the dataplane docs, converting TimeSeriesLong to wide modifies the data but converting to multi does not. I'm leaning towards Prepate Time Series now. @kylebrandt what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got some interesting side effects when I did the transformation in the prepareGraphableFields function
Screen Shot 2023-02-02 at 6 22 36 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think wide is best for this case. It is more performant with the time series panel as I understand it. Additionally because JavaScript provides undefined as a value for numbers (in addition to null), it doesn't have the same lossy data conversion properties that the backend does here.

Copy link
Contributor Author

@bohandley bohandley Feb 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryantxu I tried implementing Prepare Time Series with toTimeSeriesMulti and with the whole transformer. Both gave me the same results, grey lines and undefined y axis ticks.

Can you help me out with what I am missing?

  if (data.series.every((df) => df.meta?.type === 'timeseries-long')) {
    data.series = toTimeSeriesMulti(data.series)
    
    // const options = { format: timeSeriesFormat.TimeSeriesMulti }

    // const ctx = {
    //   interpolate: (value: string) => value,
    // }

    // data.series = prepareTimeSeriesTransformer.transformer(options, ctx)(data.series)
  }

public/app/plugins/panel/timeseries/utils.ts Outdated Show resolved Hide resolved
public/app/plugins/panel/timeseries/utils.ts Outdated Show resolved Hide resolved
public/app/plugins/panel/timeseries/utils.ts Outdated Show resolved Hide resolved
@bohandley bohandley marked this pull request as ready for review February 3, 2023 18:23
@bohandley bohandley requested review from a team, codeincarnate and zoltanbedi and removed request for a team February 3, 2023 18:23
@bohandley bohandley added add to changelog no-backport Skip backport of PR labels Feb 7, 2023
@bohandley bohandley modified the milestones: 10.0.0, 9.5.0 Feb 7, 2023
@bohandley bohandley requested a review from a team as a code owner February 21, 2023 17:05
@bohandley
Copy link
Contributor Author

bohandley commented Feb 21, 2023

@leeoniya and @kylebrandt I made a change to handle when there are two frames in the series with different field names. Now, the transform handles each frame individually and then returns the transformed series.

Screen Shot 2023-02-21 at 5 09 34 PM

Copy link
Contributor

@kylebrandt kylebrandt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, I'm not suited to review front end code, but things render as I would expect now, and the table view and query inspector show the frame still in long format (which is good).

Additionally, in the case where I had the transform in place, this still shows the same result, so doesn't seem to be breaking in that case.

@bohandley
Copy link
Contributor Author

Test with this debug snapshot
debug-Panel Title-2023-02-24 17_37_52.json.txt

@bohandley bohandley merged commit 79c9ab1 into main Feb 27, 2023
@bohandley bohandley deleted the bohandley/dataplane-support-timeserieslong-without-transform branch February 27, 2023 13:37
ryantxu pushed a commit that referenced this pull request Mar 2, 2023
* support timeSeriesLong without transform

* move timeserieslong transform to the right spot

* add review suggestions for using types and moving options inline

* handle frames with different field names

* remove extra options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/dataplane Dataplane Project (Data type contract) area/frontend area/panel/timeseries The main time series Graph panel area/transformations no-backport Skip backport of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Time Series Panel: Support "TimeSeriesLong" without Transform
5 participants