SceneQueryRunner: decouple time range comparisons #587

sd2k · 2024-02-09T16:02:19Z

Prior to this commit, SceneQueryRunner had special handling for
SceneTimeRangeCompare objects, explicitly searching for them in the
scene graph, adding additional queries, and transforming resulting
queries. This made it difficult to re-use the general behaviour of
'running additional queries' in other objects.

This commit introduces a new interface, SceneRequestAdder, which
can be implemented to inform the query runner that it should run
additional requests (returned by getExtraRequests) and transform the
results in some fashion.

Instead of searching the graph for SceneTimeRangeCompare objects,
the query runner searches for implementors of SceneRequestAdder
and uses those instead. The specifics of how it searches for these
is a little bit fuzzy and should probably be improved: it walks up the
graph until it finds at least one adder at the current level or in
any children of the current level; adds any others at that level
or in the children; then returns.

SceneTimeRangeCompare has been refactored to make use of this new
interface. I've also got a separate object which also implements
it which is working well including when both are enabled.

Relates to this Slack thread.

packages/scenes/src/querying/SceneRequestAdder.ts

packages/scenes/src/querying/extraRequestProcessingOperator.ts

packages/scenes/src/querying/SceneRequestAdder.ts

torkelo · 2024-02-14T11:16:54Z

@sd2k I think this looks interesting, sorry for late review. so much going on now with dashboard => scenes migration, and a customer project. We will try to review this in depth shortly, I think with some minor refactoring and name changes it looks overall promising / close, but I just did a very quick code scan

sd2k · 2024-02-14T11:20:08Z

@sd2k I think this looks interesting, sorry for late review. so much going on now with dashboard => scenes migration, and a customer project. We will try to review this in depth shortly, I think with some minor refactoring and name changes it looks overall promising / close, but I just did a very quick code scan

No worries, it's not urgent, just wanted to make sure it didn't fall too far off the radar 👍

I'll try and add an example of how this can be used somewhere so it's a bit less abstract.

sd2k · 2024-02-16T14:31:07Z

FYI I've added an example of using this to the scenes-baseliner branch branch along with a demo, should be easy to run but you'll need to use a version of Grafana with grafana/grafana#82299 merged in to load the plugin correctly since it depends on a WASM module.

I don't think that'd necessarily make it into the core repo since it's quite heavyweight but it's a decent example!

Also a live demo of a custom application observability build with this enabled.

sd2k · 2024-03-12T14:43:58Z

One thing I bumped into while using this in other projects was that I can't see a way to turn off the extra SceneRequestAdders per-query runner. For example, in the live demo above the ScenesBaseliner runs for the various histogram panels as well as for the time series panels, but it can't really do much in those cases.

I imagine the solution would be to have a separate embedded scene with the baseline controls or something, but it might be nice to be able to say 'this SceneRequestAdder should only run for these queries' somehow 🤔

Edit: I just rebased on main and can see that #650 added the ability to opt out of time range comparisons; it's a tiny bit special-casey for now but I imagine we could do something similar for other RequestAdders (e.g. baselines: false).

sd2k · 2024-03-21T20:04:11Z

Hey @torkelo, do you think you (or someone else) will have time to take a look at this in the next week or so? Happy to walk through it if you like.

torkelo · 2024-03-22T06:52:35Z

@sd2k sorry, it's a bit of a bad time now with G11 / GrafanaCon crunch.

sd2k · 2024-04-24T13:53:58Z

I've tried to come up with some better names for the things I wasn't happy with ('adder' because it kinda sucks, and 'transform' because it's overused). The new names are maybe slightly better?

The best way to test this out, if anyone's interested, is to check out the Baselines demo in the demo app in the downstream scenes-baseliner branch which makes use of this PR to provide a second control similar to the time range comparison.

Prior to this commit, `SceneQueryRunner` had special handling for `SceneTimeRangeCompare` objects, explicitly searching for them in the scene graph, adding additional queries, and transforming resulting queries. This made it difficult to re-use the general behaviour of 'running additional queries' in other objects. This commit introduces a new interface, `SceneRequestAdder`, which can be implemented to inform the query runner that it should run additional requests (returned by `getExtraRequests`) and transform the results in some fashion. Instead of searching the graph for `SceneTimeRangeCompare` objects, the query runner searches for implementors of `SceneRequestAdder` and uses those instead. The specifics of how it searches for these is a little bit fuzzy and should probably be improved: it walks up the graph until it finds at least one adder at the current level or in any children of the current level; adds any others at that level or in the children; then returns. `SceneTimeRangeCompare` has been refactored to make use of this new interface. I've also got a separate object which also implements it which is working well including when both are enabled.

… of any is found

Not sure these names are much better but they're possibly less ambiguous.

PR #587 is quite a large change and introduces more concepts to scenes, so isn't easy to justify merging right now. As a workaround, I'd like to be able to subclass and override some methods of the SceneQueryRunner to implement some similar behaviour in a different library, but I need various fields and methods to be accessible to the subclass, which this PR does by making them protected rather than private. Note: I can _almost_ just completely copy/paste the class into my own code rather than extending it, but the [`getQueriesForVariables`] function looks for all _instances_ of SceneQueryRunner, which my copy/pasted class would not match (unlike a subclass, which should), so I think some ad-hoc variable support would break :( `getQueriesForVariables`: https://github.com/grafana/scenes/blob/07c5dc66746d36208cd121b938883db8ef2243f7/packages/scenes/src/variables/utils.ts#L88-L94

dprokop

@sd2k - I' really sorry for the late review, but your PR (#745) made me realise i forgot about this one.

Overall - I'm in favor of proceeding with this one, as opposite to #745 which literally invites inheritance over composition.

I can't see of any objections why we would not accept this refeactor, it's a good idea that I've been thinking when first implementing the time range comparison.

The minor thing I'm missing is generalised test for supplementary requests - all tests right now are performed in the context of time range comparison so they don't neessarily show the idea behind the supplementary requests when going through spec.

Also, please condsider my rename suggestion.

packages/scenes/src/querying/SceneQueryRunner.ts

packages/scenes/src/querying/SceneRequestAdder.ts

sd2k · 2024-05-24T10:48:14Z

@sd2k - I' really sorry for the late review, but your PR (#745) made me realise i forgot about this one.

Overall - I'm in favor of proceeding with this one, as opposite to #745 which literally invites inheritance over composition.

Agreed 👍 I think the main concerns I had were the introduction of even more interfaces and concepts to the library but I'll make sure they're documented.

I can't see of any objections why we would not accept this refeactor, it's a good idea that I've been thinking when first implementing the time range comparison.

The minor thing I'm missing is generalised test for supplementary requests - all tests right now are performed in the context of time range comparison so they don't neessarily show the idea behind the supplementary requests when going through spec.

Understood, I'll try and add something to this effect.

Also, please condsider my rename suggestion.

💯 - I much prefer yours!

One side note: in another branch I've been exploring actually using this functionality with some implementations of SceneRequestSupplementer that run some ML algorithms on the secondary requests. The controls for the components allow the user to adjust things like the confidence interval and other hyperparameters of the ML algorithms. When the user changes those, we need to rerun the processors, but we don't always need to rerun the queries.

My solution to this has been to change the shouldRerun method to return an object like { query: boolean; processor: boolean }, which the implementation can use to tell the query runner whether it should rerun the query and the processor or just the processor. This is pretty essential for some UXs like this:

2024-05-24.11-42-17.mp4

since otherwise the query would need to be reissued every time the processor wanted to rerun.

I mention this because I don't want to lock us into a specific API in SceneRequestSupplementer if/when I try to upstream those changes (in a separate PR). The options I see are:

Keep the current signature (shouldRerun(prev, next): boolean) in this PR, and don't ever allow implementations to specify which of query/processor to run
Keep the current signature (shouldRerun(prev, next): boolean) in this PR, and if we allow queries/processors to run separately, release an API-incompatible change by changing the signature to shouldRerun(prev, next): { query: boolean; processor: boolean; }
Keep the current signature (shouldRerun(prev, next): boolean) in this PR, and if we allow queries/processors to run separately, release an API-compatible change by changing the signature to shouldRerun(prev, next): boolean | { query: boolean; processor: boolean; }, which is a bit more complex but possibly easier for basic use cases
Switch the signature to either shouldRerun(prev, next): { query: boolean; processor: boolean; } or shouldRerun(prev, next): boolean | { query: boolean; processor: boolean; } now.

Sorry for the long response, I've spent quite a bit of time working on this other branch the last few days! 😄

We need to use a map during the search to avoid including duplicates of any given type of `SceneRequestSupplementer`, but there's no need to return the map itself; we only ever care about the values.

sd2k · 2024-05-24T12:10:43Z

I've done the renaming, simplified a couple of things, added more comments and added a couple of tests for the queryrunner's handling of SupplementaryRequestProvider 👍

I see. But I'm wondering if you really need a processor for this implementation. What if you implemented the processor as a custom transformation (grafana.com/developers/scenes/transformations#add-custom-transformations) and attach a behaviour to it, that would react to the controls changes and call reprocessTransformations ? It's a bit more complex, but it would keep the interface of the SupplementaryRequestProvider simpler and actually focused on query execution.

This may be possible, I've not used behaviors much so would need to investigate. In particular I'm not sure how I'd link together the behaviors, controls, transformations and supplementary queries 🤔

FWIW, the nice thing about using the processor is being able to package up the controls, the extra queries, and the processing all in one reusable component so that downstream devs can just do this:

controls: [
  new SceneBaseliner({}),
  new SceneChangepointDetector({}),
],

and everything just works, similar to the time range comparisons.

…un separately Followup from [this comment](#587 (comment)) - this adds the option for SupplementalRequestProviders to state that they only wish to have their processor rerun (probably with different state), rather than the QueryRunner rerunning both the query _and_ the processor.

sd2k · 2024-05-24T12:41:59Z

I added a draft of how an alternate API for shouldRerun could look with the reprocessing handled by the query runner, in case it's interesting 🙂

…un separately Followup from [this comment](#587 (comment)) - this adds the option for SupplementalRequestProviders to state that they only wish to have their processor rerun (probably with different state), rather than the QueryRunner rerunning both the query _and_ the processor.

torkelo

@sd2k @dprokop yea, took a look at this in more detail now, I think it's direction looks pretty good and does not add that much new complexity, let's try to get this into mergable state

Added some rename suggestions, not 100% sure about them so happy for alternatives / thoughts

packages/scenes/src/querying/SceneQueryRunner.ts

packages/scenes/src/querying/SupplementaryRequestProvider.ts

…repareRequests

…name

…un separately Followup from [this comment](#587 (comment)) - this adds the option for SupplementalRequestProviders to state that they only wish to have their processor rerun (probably with different state), rather than the QueryRunner rerunning both the query _and_ the processor.

sd2k · 2024-05-28T14:06:14Z

I've addressed those issues (went with ExtraQueryProcessor for the processor name but can update it if you like), let me know if you have further comments 👍

sd2k · 2024-05-29T15:07:57Z

packages/scenes/src/querying/ExtraQueryProvider.ts

+// some more advanced processing such as fitting a time series model on the secondary data.
+//
+// See the docs for `extraQueryProcessingOperator` for more information.
+export type ExtraQueryDataProcessor = (primary: PanelData, secondary: PanelData) => PanelData;


Don't think it's suitable for this PR but if I want to offload my processing to a web worker this is going to need to return a Promise instead...

Suggested change

export type ExtraQueryDataProcessor = (primary: PanelData, secondary: PanelData) => PanelData;

export type ExtraQueryDataProcessor = (primary: PanelData, secondary: PanelData) => Promise<PanelData>;

If we don't introduce this now, it's going to be a breaking change in the future. I think it may be worth doing this straight away, wdyt @torkelo ?

I was thinking we could change it to export type ExtraQueryDataProcessor = (primary: PanelData, secondary: PanelData) => PanelData | Promise<PanelData>; in future but it is clunky, I agree...

Can it be an observable (like the current post processing) that will be much easier to slot in to the rxjs pipeline

yeah, +1 for that!

I've changed the type to export type ExtraQueryDataProcessor = (primary: PanelData, secondary: PanelData) => Observable<PanelData>; in bdab9f4, is that what you meant? 🤔 Think I got the types right but I might have misunderstood.

i think that's exactly it @sd2k

dprokop

This is looking good now, the rename to Extra... is 👌

dprokop · 2024-06-03T09:44:53Z

packages/scenes/src/querying/ExtraQueryProvider.ts

+// some more advanced processing such as fitting a time series model on the secondary data.
+//
+// See the docs for `extraQueryProcessingOperator` for more information.
+export type ExtraQueryDataProcessor = (primary: PanelData, secondary: PanelData) => PanelData;


If we don't introduce this now, it's going to be a breaking change in the future. I think it may be worth doing this straight away, wdyt @torkelo ?

sd2k · 2024-06-04T12:59:00Z

@torkelo are there any changes you'd like to see to this before merging?

torkelo

Looks great, thanks for sticking with this slow PR review process, hopefully the next one will not take months to get approved!

❤️

…rately Followup from [this comment](#587 (comment)) - this adds the option for ExtraQueryProviders to state that they only wish to have their processor rerun (probably with different state), rather than the QueryRunner rerunning both the query _and_ the processor. This is really useful for some ML-based providers which need to run an extra query then transform the results, and also include interactivity such as a slider, but _don't_ need to rerun the query as part of the interactivity - just the processing. There are some downsides here, most notably the extra complexity: - the `ExtraQueryProvider` interface is more flexible but more complex - the `SceneQueryRunner` needs another subscription and `ReplaySubject` in order to be able to re-send the latest unprocessed data to the processors again - I think this will also increase memory usage? but also it feels a bit like this is already being done by transformations somehow... I'm probably missing something

…rately Followup from [this comment](#587 (comment)) - this adds the option for ExtraQueryProviders to state that they only wish to have their processor rerun (probably with different state), rather than the QueryRunner rerunning both the query _and_ the processor. This is really useful for some ML-based providers which need to run an extra query then transform the results, and also include interactivity such as a slider, but _don't_ need to rerun the query as part of the interactivity - just the processing. There are some downsides here, most notably the extra complexity: - the `ExtraQueryProvider` interface is more flexible but more complex - the `SceneQueryRunner` needs another subscription and `ReplaySubject` in order to be able to re-send the latest unprocessed data to the processors again - I think this will also increase memory usage? Perhaps there's a way to do this using transformations instead?

grafanabot · 2024-06-05T15:33:04Z

🚀 PR was released in v4.26.1 🚀

…rately Followup from [this comment](#587 (comment)) - this adds the option for ExtraQueryProviders to state that they only wish to have their processor rerun (probably with different state), rather than the QueryRunner rerunning both the query _and_ the processor. This is really useful for some ML-based providers which need to run an extra query then transform the results, and also include interactivity such as a slider, but _don't_ need to rerun the query as part of the interactivity - just the processing. There are some downsides here, most notably the extra complexity: - the `ExtraQueryProvider` interface is more flexible but more complex - the `SceneQueryRunner` needs another subscription and `ReplaySubject` in order to be able to re-send the latest unprocessed data to the processors again - I think this will also increase memory usage? Perhaps there's a way to do this using transformations instead?

sd2k commented Feb 9, 2024

View reviewed changes

packages/scenes/src/querying/SceneRequestAdder.ts Outdated Show resolved Hide resolved

sd2k commented Feb 9, 2024

View reviewed changes

packages/scenes/src/querying/extraRequestProcessingOperator.ts Outdated Show resolved Hide resolved

sd2k commented Feb 9, 2024

View reviewed changes

packages/scenes/src/querying/SceneRequestAdder.ts Outdated Show resolved Hide resolved

sd2k force-pushed the decouple-comparisons-from-queryrunner branch from d35c5f3 to 7f75cef Compare February 9, 2024 22:00

sd2k marked this pull request as ready for review February 11, 2024 17:19

sd2k requested a review from torkelo February 14, 2024 10:59

sd2k force-pushed the decouple-comparisons-from-queryrunner branch from 7f75cef to e12a606 Compare February 16, 2024 10:39

sd2k force-pushed the decouple-comparisons-from-queryrunner branch from e12a606 to 4cc87f3 Compare March 21, 2024 20:13

sd2k force-pushed the decouple-comparisons-from-queryrunner branch 4 times, most recently from 8b74dd9 to 8154573 Compare April 24, 2024 13:52

sd2k force-pushed the decouple-comparisons-from-queryrunner branch from 8154573 to 79d35df Compare May 2, 2024 08:38

sd2k and others added 2 commits May 16, 2024 11:41

Find first RequestAdder of each type, rather than stopping when first…

87eb1c6

… of any is found

sd2k force-pushed the decouple-comparisons-from-queryrunner branch from 79d35df to 4f58dee Compare May 16, 2024 10:41

Rename 'transform' to 'processor' and 'adder' to 'supplementer'

9aa6700

Not sure these names are much better but they're possibly less ambiguous.

sd2k force-pushed the decouple-comparisons-from-queryrunner branch from 4f58dee to 9aa6700 Compare May 16, 2024 10:44

sd2k mentioned this pull request May 24, 2024

SceneQueryRunner: make some fields and methods protected #745

Closed

dprokop reviewed May 24, 2024

View reviewed changes

packages/scenes/src/querying/SceneQueryRunner.ts Outdated Show resolved Hide resolved

packages/scenes/src/querying/SceneQueryRunner.ts Outdated Show resolved Hide resolved

packages/scenes/src/querying/SceneRequestAdder.ts Outdated Show resolved Hide resolved

Return array from getClosestRequestAdders

aea64d9

We need to use a map during the search to avoid including duplicates of any given type of `SceneRequestSupplementer`, but there's no need to return the map itself; we only ever care about the values.

s/supplemental/supplementary/g

374bda5

sd2k mentioned this pull request May 24, 2024

SceneQueryRunner: allow extra queries and processors to be rerun separately #748

Draft

torkelo reviewed May 27, 2024

View reviewed changes

packages/scenes/src/querying/SceneQueryRunner.ts Outdated Show resolved Hide resolved

packages/scenes/src/querying/SceneQueryRunner.ts Outdated Show resolved Hide resolved

packages/scenes/src/querying/SupplementaryRequestProvider.ts Outdated Show resolved Hide resolved

sd2k added 3 commits May 27, 2024 17:55

Extract PreparedRequests type; document; use non-arrow function for p…

8d19b5d

…repareRequests

Use map from provider constructor to object, rather than constructor …

5d867d5

…name

Rename SupplementalRequestFoo to ExtraQueryFoo throughout

2b13e2a

Include annotations from secondary processed frames

61de2a1

s/ExtraQueryProcessor/ExtraQueryDataProcessor/

c410bf2

sd2k commented May 29, 2024

View reviewed changes

dprokop approved these changes Jun 3, 2024

View reviewed changes

sd2k added 2 commits June 3, 2024 16:43

Change ExtraQueryDataProcessor to return an Observable

bdab9f4

Add trivial processor to TestExtraQueryProvider

87ead7f

torkelo approved these changes Jun 5, 2024

View reviewed changes

sd2k merged commit 207b3f5 into main Jun 5, 2024
3 checks passed

sd2k deleted the decouple-comparisons-from-queryrunner branch June 5, 2024 10:03

grafanabot added the released label Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SceneQueryRunner: decouple time range comparisons #587

SceneQueryRunner: decouple time range comparisons #587

sd2k commented Feb 9, 2024 •

edited

Loading

torkelo commented Feb 14, 2024

sd2k commented Feb 14, 2024

sd2k commented Feb 16, 2024 •

edited

Loading

sd2k commented Mar 12, 2024 •

edited

Loading

sd2k commented Mar 21, 2024

torkelo commented Mar 22, 2024

sd2k commented Apr 24, 2024

dprokop left a comment

sd2k commented May 24, 2024

sd2k commented May 24, 2024

sd2k commented May 24, 2024

torkelo left a comment

sd2k commented May 28, 2024

sd2k May 29, 2024

dprokop Jun 3, 2024

sd2k Jun 3, 2024 •

edited

Loading

torkelo Jun 3, 2024

dprokop Jun 3, 2024

sd2k Jun 3, 2024

dprokop Jun 3, 2024

dprokop left a comment

dprokop Jun 3, 2024

sd2k commented Jun 4, 2024

torkelo left a comment

grafanabot commented Jun 5, 2024

	export type ExtraQueryDataProcessor = (primary: PanelData, secondary: PanelData) => PanelData;
	export type ExtraQueryDataProcessor = (primary: PanelData, secondary: PanelData) => Promise<PanelData>;

SceneQueryRunner: decouple time range comparisons #587

SceneQueryRunner: decouple time range comparisons #587

Conversation

sd2k commented Feb 9, 2024 • edited Loading

torkelo commented Feb 14, 2024

sd2k commented Feb 14, 2024

sd2k commented Feb 16, 2024 • edited Loading

sd2k commented Mar 12, 2024 • edited Loading

sd2k commented Mar 21, 2024

torkelo commented Mar 22, 2024

sd2k commented Apr 24, 2024

dprokop left a comment

Choose a reason for hiding this comment

sd2k commented May 24, 2024

sd2k commented May 24, 2024

sd2k commented May 24, 2024

torkelo left a comment

Choose a reason for hiding this comment

sd2k commented May 28, 2024

sd2k May 29, 2024

Choose a reason for hiding this comment

dprokop Jun 3, 2024

Choose a reason for hiding this comment

sd2k Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

torkelo Jun 3, 2024

Choose a reason for hiding this comment

dprokop Jun 3, 2024

Choose a reason for hiding this comment

sd2k Jun 3, 2024

Choose a reason for hiding this comment

dprokop Jun 3, 2024

Choose a reason for hiding this comment

dprokop left a comment

Choose a reason for hiding this comment

dprokop Jun 3, 2024

Choose a reason for hiding this comment

sd2k commented Jun 4, 2024

torkelo left a comment

Choose a reason for hiding this comment

grafanabot commented Jun 5, 2024

sd2k commented Feb 9, 2024 •

edited

Loading

sd2k commented Feb 16, 2024 •

edited

Loading

sd2k commented Mar 12, 2024 •

edited

Loading

sd2k Jun 3, 2024 •

edited

Loading