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

Transformations: Allow Timeseries to table transformation to handle multiple time series #76801

Merged
merged 21 commits into from
Oct 27, 2023

Conversation

codeincarnate
Copy link
Contributor

What is this feature?

This PR extends the capabilities of the Timeseries to table transformation to support multiple time series, even if those time series are embedded within a single data series. That is, it will be able to use a time field in combination with number fields and labeled data to breakdown time series data into distinct table rows.

Additionally, this also handles the case in which multiple series are present. Currently it doesn't modify multiple series which is actually a departure in behavior from how this transform worked previously in which multiple series would be mapped to a single one (i.e. even with multiple series the series selector for the table wouldn't appear).

I'm currently planning on adding an option (and making it the default) in order to preserve this behavior. However, it feels like we should update this in the future as currently multiple data series in the table panel will show a series selector. The default behavior of this transformation breaks that underlying structure

Beyond that, it currently assumes that the first time field found is the one that should be used to determine the time series. This isn't necessarily correct, and it's quite possible multiple time series could be present in the incoming data (e.g. created times vs update times for records is a straightforward example). I'll be adding an option for that.

Why do we need this feature?

This significantly enhances the capabilities of the Timeseries to table transformation to handle multiple data scenarios. In particular it should allow for this transformation to work across time series datasources, as well as other data sources with time data (e.g. SQL, App Analytics, etc.). This will allow multiple time series datums to be displayed together in a straightforward manner.

Who is this feature for?

Users that have multiple time series frames being returned from a single datasource.

Which issue(s) does this PR fix?:

Fixes #76291

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@codeincarnate
Copy link
Contributor Author

Have this mostly working at this point. Working on one bug in which merging off all series from multiple queries isn't working right. Think I know how to solve that one.

@codeincarnate
Copy link
Contributor Author

Did some cleanups and fixes and also fixed the test as the overall output has changed. Taking this out of draft so feedback can be incorporated. As far as I know the main thing missing at this point is a migration for the transformation settings.

@codeincarnate codeincarnate marked this pull request as ready for review October 24, 2023 06:54
@codeincarnate codeincarnate requested review from a team as code owners October 24, 2023 06:54
@codeincarnate codeincarnate requested a review from a team October 26, 2023 11:09
@codeincarnate
Copy link
Contributor Author

I found the problem, the options for other frames were being copied but not the options for the frame being modified 😭. That being said this should be fixed now 😄

Copy link
Contributor

@mdvictor mdvictor left a comment

Choose a reason for hiding this comment

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

Tested out the changes and LGTM!

@codeincarnate
Copy link
Contributor Author

Currently looking into the GenAI tests failures so I don't blow up something important there. Otherwise though I think this is mostly ready to go.

@codeincarnate codeincarnate merged commit 71e3814 into main Oct 27, 2023
13 checks passed
@codeincarnate codeincarnate deleted the transform/time-multi-fix branch October 27, 2023 15:30
@torkelo
Copy link
Member

torkelo commented Oct 31, 2023

@codeincarnate would have been good if @domasx2 had reviewed this it looks to be a breaking change For app11y app that uses this

@torkelo
Copy link
Member

torkelo commented Oct 31, 2023

Ah, see we have #77415 already 👍

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

@codeincarnate I could not find any new tests that cover the new behavior, only modified tests that show a very big change in behavior from before, so hard to understand what the goal was here.

@codeincarnate
Copy link
Contributor Author

Definitely a miss on this on my part, there were some parts of the original behavior I misunderstood. Working with @domasx2 on this now.

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.

Transformations: TimeseriesToTable Transformation Error with Multiple Timeseries
6 participants