search

grafana / scenes

Build Grafana dashboards directly in your Grafana app plugins.
https://grafana.com/developers/scenes
Apache License 2.0
133 stars 20 forks source link

SceneQueryRunner: make runWithTimeRange protected #866

Closed gtk-grafana closed 1 month ago

gtk-grafana commented 1 month ago

In explore logs we want more control over when queries are run, so we want to extend the SceneQueryRunner class and call private method runWithTimeRange.

This PR simply makes runWithTimeRange protected instead of private so we can override the default behavior of runQueries for specific datasource api calls.

torkelo commented 1 month ago

runQueries is already public, is it not good enough?

dprokop commented 1 month ago

@gtk-grafana - +1 to what @torkelo says. Not sure why you wouldn't simply override the runQueries method to get rid of the time range subscription.

svennergr commented 1 month ago

@torkelo @dprokop I'll let @gtk-grafana chime in here, but AFAIK this is exactly what we want to do: https://github.com/grafana/explore-logs/blob/c3a6182bb9b0b8dafe89acad7b83355a2caf18b3/src/services/LogsSceneQueryRunner.ts#L8-L17

We just want the same runQueries behavior, but without subscribing to timerange changes.

Is your suggestion that we "rebuild" the logic runWithTimeRange does into the runQueries method of our own class?

torkelo commented 1 month ago

@svennergr @gtk-grafana ah, so you want something like https://github.com/grafana/scenes/pull/334 then? I did that PR and asked if there was any interest in this feature in #project-grafana-scenes-apps but go no response so closed it. But I think it's what your after?

I do not think overriding runWithTime to skip execute query on time range change is the right approach as a time range subscription is already setup when the query runner activates.

gtk-grafana commented 1 month ago

@torkelo #334 is similar to what we are looking for, except we want to skip running subscribeToTimeRangeChanges, as we don't want to activate the query runner as this necessitates that we call the query far more often then we really need. If there was a way to control the time range subscription #334 would work great.

torkelo commented 1 month ago

is similar to what we are looking for, except we want to skip running subscribeToTimeRangeChanges

@gtk-grafana is that not what that PR does?

gtk-grafana commented 1 month ago

Looks like #334 doesn't update runQueries, so when manually calling runQueries in our application we'll still get the subscription we don't want, but I'll play around with it a bit and see if this gets us closer.

gtk-grafana commented 1 month ago

Yeah, #334 doesn't look like it will currently work as the query runner is still activated when calling runQueries, which will still make the call we're trying to avoid on every route change within the application.

Is making runWithTimeRange protected so developers can overload runQueries without calling subscribeToTimeRangeChanges as proposed in this PR an acceptable solution for now?

torkelo commented 1 month ago

Looks like https://github.com/grafana/scenes/pull/334 doesn't update runQueries, so when manually calling runQueries in our application we'll still get the subscription we don't want, but I'll play around with it a bit and see if this gets us closer.

@gtk-grafana that is because the PR is from a time before the time range subscription was updated (in case of changed time range object) in runQueries, if we where to re-open it and update that PR to account for the current state of the code we would of course make sure we do not add a subscription there as well as in onActivate

torkelo commented 1 month ago

Is making runWithTimeRange protected so developers can overload runQueries without calling subscribeToTimeRangeChanges as proposed in this PR an acceptable solution for now?

You would still have a time range subscription create in onActivate with current PR

gtk-grafana commented 1 month ago

You would still have a time range subscription create in onActivate with current PR

We're working around this for now by not manually activating additional query runners on a scene, it looks like data providers on a scene that aren't in the $data prop don't get activated automatically anyway.

@gtk-grafana that is because the PR is from a time before the time range subscription was updated (in case of changed time range object) in runQueries, if we where to re-open it and update that PR to account for the current state of the code we would of course make sure we do not add a subscription there as well as in onActivate

Sounds good, yeah then that sounds exactly like it would do exactly what we're looking for! What's the best way to help move this forward?

torkelo commented 1 month ago

Sounds good, yeah then that sounds exactly like it would do exactly what we're looking for! What's the best way to help move this forward?

@gtk-grafana can you take that PR an open a new version of it? (one that disables time range subscription in activate as well in runQueries)