Explore: Use PanelQueryState to handle querying #18694

hugohaggmark · 2019-08-23T11:38:25Z

What this PR does / why we need it:
Completely replaces existing epics with PanelQueryState in Explore. Reasoning behind this is that we're no longer using the RxJs part of epics because they have been replaced by PanelQueryState. So to minimize complexity I transformed existing epics to Thunks.

Which issue(s) this PR fixes:

Special notes for your reviewer:
Tested with both Loki and Prometheus and it seems to work. ~~Had to remove the feature where Prom instant is returned first and then the TimeSeries. Reverted back to returning all.~~

public/app/features/explore/state/epics/runRequestEpic.ts

hugohaggmark · 2019-08-23T11:45:23Z

public/app/plugins/datasource/prometheus/datasource.ts

              unsubscribe: () => undefined,
            };

-            return state;
+            if (query.instant) {


Hack until we implement can use sub requests.

does not need sub requests -- just a different key

note the changes in #16676 are not required for sub requests, that really just uses them for the mixed query datasource and also adds work to better report it in the editor UI. Multi-request streaming is already supported, but no built-in datasources use it (yet)... partly because keeping in sync with explore

public/app/features/explore/state/epics/runRequestEpic.ts

ryantxu · 2019-08-26T04:08:32Z

public/app/features/explore/state/epics/runRequestEpic.ts

+              let newRange = request.range;
+              let absoluteRange: AbsoluteTimeRange = {
+                from: newRange.from.valueOf(),
+                to: newRange.to.valueOf(),


You don't need to do any of this part... it already happened in validateStreamsAndGetPanelData()

you can just use the request.range

ryantxu

I don't have enough knowledge about redux or epics to say anything helpful in that domain.

calling queryState seems good though :)

ryantxu · 2019-08-26T21:31:30Z

@hugohaggmark -- Check if the changes in #18674 are useful for you... if yes, you can merge them into this, otherwise I will just remove that PR. And obviously if any other change to panelQueryState would make your interaction smoother we should do that.

…or multiple targets

hugohaggmark · 2019-08-27T14:21:42Z

public/app/features/explore/state/actions.ts

+      }
+
+      if (state === LoadingState.Streaming) {
+        dispatch(limitMessageRate(exploreId, isLokiDataSource ? series : legacy, datasourceId));


We have to replace the call to limitMessageRate with processQueryResults and implement throttling in datasources instead because Prometheus datasource uses this as a way to run Instant and TimeSeries queries in parallel.

throttling is interesting -- there are two ways we may want to throttle:

listener reading messages from the server (dropped values are ok)

notification updates in the panel/explore (the refresh rate)

In #18728, I looked at how we could maybe make these part of the standard request object. Maybe something like:

interface ObserverConfig { pause?: boolean; throttle?: number; } export interface StreamingQueryOptions { ... listener?: ObserverConfig; notification?: ObserverConfig; }

@hugohaggmark limitMessageRate does not actually limit anything right now does it?

Nope because of the reason in my comment, it would break Prometheus queries.

More details: to make Prometheus parallel querying work with PanelQueryState I use the streaming callback. But after posting each query's result I immediately send a "stop this stream" kind of result as well. If we then throttle/debounce that it will miss the first result with the data. So I'm just passing data along, I'm currently simplifying this even further in next PR so limitMessageRate will go away and also @ryantxu is adding buffering directly to the Loki datasource.

So is there anything to do on my side? I mean seems like my work with the buffering was kinda hijacked :D. Should I just wait for @ryantxu and reuse that work afterwards?

Sorry didn't mean to hijack your work but I think you'll have to move your part to the loki datasource for now. Let me know if you need help with that. And yes it's hard to sync 3 ongoing efforts

aocenas · 2019-08-28T09:30:39Z

public/app/features/explore/state/actions.ts

+    // but we're using the datasource interval limit for now
+    const interval = datasourceInstance.interval;
+
+    if (!queryState.isStarted()) {


I think there should not be the negation, should it? Also seems like this could use the stopQueryState utility function.

Good catch! Will fix

aocenas · 2019-08-28T10:06:20Z

public/app/features/explore/state/reducers.ts

@@ -114,6 +116,7 @@ export const makeExploreItemState = (): ExploreItemState => ({
  mode: null,
  isLive: false,
  urlReplaced: false,
+  queryState: new PanelQueryState(),


Not sure about this. I feel like this should not be accessible by the react and also does not drive rendering in any. We do not change it and it is also mutable. So I wonder if we shouldn't just move it outside of the redux store and handle it as external dependency kind of thing.

Think this is fine for now, it is mutable so not ideal to have in redux but not forbidden either. Think before we do more complex work figure out the proper redux solution around this we can move ahead with this. I think the coming rewrite to how queries are executed to use observable patterns, that will fundamentally change things so good to wait to do more work around the current API & responsibility of PanelQueryState as I think that will change

Good points! But I'll let it be for now.

aocenas · 2019-08-28T10:10:41Z

public/app/features/explore/state/actions.ts

+      }
+
+      if (state === LoadingState.Streaming) {
+        dispatch(limitMessageRate(exploreId, isLokiDataSource ? series : legacy, datasourceId));


@hugohaggmark limitMessageRate does not actually limit anything right now does it?

aocenas · 2019-08-28T10:14:19Z

public/app/features/explore/state/actions.ts

@@ -401,11 +431,304 @@ export function modifyQueries(
 */
 export function runQueries(exploreId: ExploreId): ThunkResult<void> {
  return (dispatch, getState) => {
-    dispatch(updateTimeRangeAction({ exploreId }));
-    dispatch(runQueriesAction({ exploreId }));
+    dispatch(updateTime({ exploreId }));


As a note, not sure if this is because I already know what this should be doing, but I find this a bit more readable than before.

…a/grafana into hugoh/use-panelQueryDataInExplore

torkelo

Have tested this quite a bit, with Prometheus, Loki, InfluxDB, Elasticsearch, Postgres, Graphite.

Looks to work really well. Seems to even fix some current issues I have seen with being stuck in a loading state when switching data sources or logs mode.

I have found some issues with Elasticsearch that I am yet not sure is related to this PR or existing issues in master. Will investigate more. But mergeable soon!

torkelo · 2019-08-28T16:24:45Z

Merging this now to aid further development in other PRs. But need to investigate more the issue with Elasticsearch (query editor shows error box but otherwise works great), Elasticsearch Logs is also not working (think due to time field losing its time type somewhere).

But can be fixed in a PR tomorrow

* grafana/master: Explore: Use PanelQueryState to handle querying (grafana#18694) Chore: Improve err message for notifications (grafana#18757) @grafana/toolkit: add package versions to the ci report (grafana#18751) @grafana/data: Matchers and Transforms (grafana#16756) Docs: Document LDAP config reload in admin http api (grafana#18739) center NoDataSourceCallToActionCard in Explore (grafana#18752) DataLinks: enable data links in Gauge, BarGauge and SingleStat2 panel (grafana#18605) DashboardDatasource: reuse query results within a dashboard (grafana#16660) Plugins: show a clear error on the plugin page when it failed to load (grafana#18733) Chore: Use ruleId instead of alertId as log keyword (grafana#18738) @grafana/data: improve the CircularVector api (grafana#18716) QueryEditor: check if optional func toggleEditorMode is provided (grafana#18705) Emails: remove the yarn.lock (grafana#18724) OAuth: Support JMES path lookup when retrieving user email (grafana#14683) Emails: resurrect template notification (grafana#18686) Email: add reply-to and direct attachment (grafana#18715) Dashboard: Adds Logs Panel (alpha) as visualization option for Dashboards (grafana#18641)

WIP: inital POC

349c42b

hugohaggmark added the area/explore label Aug 23, 2019

hugohaggmark requested review from torkelo, ryantxu and aocenas August 23, 2019 11:38

hugohaggmark self-assigned this Aug 23, 2019

hugohaggmark commented Aug 23, 2019

View reviewed changes

public/app/features/explore/state/epics/runRequestEpic.ts Outdated Show resolved Hide resolved

hugohaggmark commented Aug 23, 2019

View reviewed changes

aocenas reviewed Aug 23, 2019

View reviewed changes

public/app/features/explore/state/epics/runRequestEpic.ts Outdated Show resolved Hide resolved

public/app/features/explore/state/epics/runRequestEpic.ts Outdated Show resolved Hide resolved

ryantxu reviewed Aug 26, 2019

View reviewed changes

Wip: Moving forward

7f7de19

hugohaggmark added 6 commits August 27, 2019 06:44

Chore: Merge with master

2b59fd5

Wip

c6ceaf6

Refactor: Makes loading indicator work for Prometheus

dfb810b

Refactor: Reverts prom observable queries because they did not work f…

9176537

…or multiple targets

Refactor: Transforms all epics into thunks

6f837d9

Fix: Fixes scanning

8b7aa49

hugohaggmark marked this pull request as ready for review August 27, 2019 13:21

hugohaggmark added area/streaming type/refactor labels Aug 27, 2019

Fix: Fixes so that Instant and TimeSeries Prom query loads in parallel

eb72bd9

hugohaggmark changed the title ~~WIP: inital POC~~ Explore: Use PanelQueryState to handle querying Aug 27, 2019

hugohaggmark requested a review from davkal August 27, 2019 13:44

hugohaggmark added the pr/needs-manual-testing Before merge some help with manual testing & verification is requested label Aug 27, 2019

hugohaggmark added this to Under review in Observability (deprecated, use Observability Squad) via automation Aug 27, 2019

hugohaggmark added this to the 6.4 milestone Aug 27, 2019

hugohaggmark commented Aug 27, 2019

View reviewed changes

aocenas reviewed Aug 28, 2019

View reviewed changes

hugohaggmark and others added 3 commits August 28, 2019 13:30

Fix: Fixes negation logic error

0317d0e

Propagate errors in stream events, and close streams

28168d7

Merge branch 'hugoh/use-panelQueryDataInExplore' of github.com:grafan…

fef2e66

…a/grafana into hugoh/use-panelQueryDataInExplore

torkelo approved these changes Aug 28, 2019

View reviewed changes

torkelo merged commit 5ca643f into master Aug 28, 2019

Observability (deprecated, use Observability Squad) automation moved this from Under review to Done Aug 28, 2019

torkelo deleted the hugoh/use-panelQueryDataInExplore branch August 28, 2019 16:24

torkelo mentioned this pull request Sep 15, 2019

Explore: When zooming in the logs graph, the table has entries but graph doesn't #19086

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explore: Use PanelQueryState to handle querying #18694

Explore: Use PanelQueryState to handle querying #18694

hugohaggmark commented Aug 23, 2019 •

edited

hugohaggmark Aug 23, 2019

ryantxu Aug 23, 2019

ryantxu Aug 26, 2019

ryantxu Aug 26, 2019

ryantxu left a comment

ryantxu commented Aug 26, 2019

hugohaggmark Aug 27, 2019

ryantxu Aug 27, 2019

aocenas Aug 28, 2019

hugohaggmark Aug 28, 2019

hugohaggmark Aug 28, 2019

aocenas Aug 28, 2019

hugohaggmark Aug 28, 2019

aocenas Aug 28, 2019

hugohaggmark Aug 28, 2019

aocenas Aug 28, 2019

torkelo Aug 28, 2019

hugohaggmark Aug 28, 2019

aocenas Aug 28, 2019

aocenas Aug 28, 2019

torkelo left a comment

torkelo commented Aug 28, 2019

Explore: Use PanelQueryState to handle querying #18694

Explore: Use PanelQueryState to handle querying #18694

Conversation

hugohaggmark commented Aug 23, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryantxu left a comment

Choose a reason for hiding this comment

ryantxu commented Aug 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torkelo left a comment

Choose a reason for hiding this comment

torkelo commented Aug 28, 2019

hugohaggmark commented Aug 23, 2019 •

edited