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

TimeSeries: Explicitly add transformer when timeseries-long exists #64092

Merged
merged 14 commits into from
Apr 28, 2023

Conversation

ryantxu
Copy link
Member

@ryantxu ryantxu commented Mar 3, 2023

This is revisiting the auto-magic transformation added in #62732

While nice, the result is that many subtle features do not work as expected because the field names do not exist for overrides and customization. The legend toggles also do not work.

What is this feature?

This replaces the automatic transform with a button that will add the transform explicitly.

Why do we need this feature?

Having explicit control of the shape will cause fewer surprises long term

2023-03-02_20-25-25 (1)

TODO:

#63901 will need something similar

@ryantxu ryantxu added add to changelog area/panel/timeseries The main time series Graph panel no-backport Skip backport of PR labels Mar 3, 2023
@ryantxu ryantxu added this to the 9.5.0 milestone Mar 3, 2023
@ryantxu ryantxu requested a review from kylebrandt March 3, 2023 04:26
@torkelo
Copy link
Member

torkelo commented Mar 3, 2023

Good catch, it think we have to do this, a transformation that adds/changes field names cannot be done inside the visualization.

One possible alternative would be that each panel plugin could also expose a transformation function that is called as part of the data processing (before applyFieldOverrides), but that could be really messy to implement (both in current arch and scenes) as data production (query runners) are pretty separate from visualizations. However there is actually link in the scenes framework (the VizPanel checks if there is a query runner on its own level and sets the MaxDataPoints based on viz width) , this could in theory also add a transformation callback specified in the plugin

@ryantxu
Copy link
Member Author

ryantxu commented Mar 3, 2023

🤔 -- long term, perhaps plugins could specific the set of DataFrameTypes it supports, and the PanelQueryRunner (or scenes) could insert a step to convert before calling overrides.

But I think this is a reasonable approach to get us there explicitly. The downside is when flipping panel types, the xform may not be ncessary. But i think that is a small price to pay for more consistency

@kylebrandt
Copy link
Contributor

@ryantxu can you expand a bit more on why this doesn't (or won't) work? Would like to understand why inserting the transformation is needed.

@leeoniya
Copy link
Contributor

leeoniya commented Mar 5, 2023

a possible issue here is that not all string fields may be relevant for partitioning. for instance, fields used for datalinks. e.g. #64088

the partitionByValues transformer UI offers a field selection so some fields can be opted out of, but the Prepare time series UI does not.

@ryantxu
Copy link
Member Author

ryantxu commented Mar 6, 2023

understand why inserting the transformation is needed.

If we are only using default settings, it can work fine -- however if the panel requires any configuration/field(series) then the infrastructure to assign that config happens before we run the long-to-wide conversion. This means odd things do not work like:

  • toggling visibility in the legend
  • applying field overrides
  • anywhere we have a field picker UI element

In #62732 @bohandley applies the transformation inside the panel, but this PR moves it so it will happen before the panel gets the data.

@bohandley
Copy link
Contributor

understand why inserting the transformation is needed.

If we are only using default settings, it can work fine -- however if the panel requires any configuration/field(series) then the infrastructure to assign that config happens before we run the long-to-wide conversion. This means odd things do not work like:

  • toggling visibility in the legend
  • applying field overrides
  • anywhere we have a field picker UI element

In #62732 @bohandley applies the transformation inside the panel, but this PR moves it so it will happen before the panel gets the data.

I like this! If we add it early enough, then the panel will have everything it needs and we won't have to explicitly bring in the transformation, right? I think if we are going to support time series long, we should do it implicitly, with no extra clicks. This is possible with the right amount of checks, right? @ryantxu @leeoniya @kylebrandt I remember my first try was way outside the panel and I think it might have worked. I can try that out again.

@grafanabot
Copy link
Contributor

This pull request was removed from the 9.5.0 milestone because 9.5.0 is currently being released.

@torkelo
Copy link
Member

torkelo commented Apr 9, 2023

@ryantxu @bohandley we should revert #62732 before 9.5 stable is released

@bohandley
Copy link
Contributor

bohandley commented Apr 12, 2023

@ryantxu @bohandley we should revert #62732 before 9.5 stable is released

@torkelo I just looked over reverting #62732 and tried to revert it remembering these changes have already been fixed. #62732 made changes in app/plugins/panel/timeseries/utils.ts and these changes were removed in this PR. The new changes move the functionality into the internal logic of the time series tranformer so that the transform can be applied explicitly by choosing to do so instead of automatically. This fixes the issue so the PR #62732 does not need to be reverted. Thanks @ryantxu!

@ryantxu ryantxu marked this pull request as ready for review April 27, 2023 18:32
@ryantxu ryantxu requested review from a team as code owners April 27, 2023 18:32
@ryantxu ryantxu requested review from joshhunt, JoaoSilvaGrafana, mckn, axelavargas and polibb and removed request for a team April 27, 2023 18:32
@ryantxu ryantxu added this to the 10.0.0 milestone Apr 27, 2023
@ryantxu
Copy link
Member Author

ryantxu commented Apr 27, 2023

@bohandley / @kylebrandt -- I think this is good to go now.

@ryantxu ryantxu enabled auto-merge (squash) April 28, 2023 02:59
@ryantxu ryantxu merged commit f5d97c6 into main Apr 28, 2023
9 checks passed
@ryantxu ryantxu deleted the explicit-conversion branch April 28, 2023 03:10
@zerok zerok modified the milestones: 10.0.0, 10.0.0-preview May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants