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

TimeRangeCompare: Do not re-run queries if all have opted out of comparison #799

Closed domasx2 closed 3 months ago

domasx2 commented 3 months ago

:wave:

We found an issue in app o11y where changing time window comparison value causes query runner to re-run queries even if all queries have opted out of TWC. This causes unnecessary querying and side effects for us.

This PR fixes makes twc not trigger re-run if all queries are opted-out

Thanks!

domasx2 commented 3 months ago

I wonder if it's possible to do this without changing the ExtraQueryProvider interface, by setting a property like foundExtraQueries in the TimeRangeCompare's getExtraQueries method and checking it in shouldRerun 🤔 I'm not sure that's better, though...

There can be many query runners for a single TimeRangeCompare, so I think this kind of approach may get sticky with complex state :thinking:

sd2k commented 3 months ago

Ah yeah, good point.

This will work but I'm still a little annoyed by it (my fault, not yours :grinning:) - I feel like the query runner should be smart enough to realise that the provider didn't return any extra queries from getExtraQueries and skip calling provider.shouldRerun if the query hasn't changed; that way each provider wouldn't have to deal with properties like timeRangeCompare in more than one place. But getting that to work properly sounds complicated so perhaps it isn't worth it.

I'll wait for @dprokop to take a look before approving. Thanks @domasx2!

grafanabot commented 3 months ago

:rocket: PR was released in v5.3.1 :rocket: