Long Running Queries #21

kevinwcyu · 2022-08-30T19:42:10Z

No description provided.

CLAassistant · 2022-08-30T19:42:14Z

All committers have signed the CLA.

kevinwcyu · 2022-08-31T01:22:46Z

src/sql/types.ts

@@ -7,4 +7,5 @@ export interface SQLQuery extends DataQuery {
  rawSQL: string;
  format?: number;
  fillMode?: { mode: FillValueOptions; value?: number };
+  meta?: { queryFlow?: 'async' | 'sync' };


The frontend sends this to the backend to determine whether to use the sync (existing) query flow or the async query flow. The frontend sets this field based on a feature toggle. This is sent from the frontend because external plugins cannot access Grafana backend feature toggles

is it odd that this would be saved in the query? How would we update old queries? if we have to revert after releasing would that be ok if queries are saved with this field?

I wonder if we could somehow right our frontend code to make the query not knowing if it is sync or async and then make different choices based on the response from the backend instead?

This should not be saved in the query. The frontend adds this to the query right before it is executed. So old queries wouldn't need to know about this field. If I'm not mistaken, this only gets saved in the query if it's called with onChange, which isn't called between the time this field is added and when we execute the query. Here's where it's added in the redshift datasource https://github.com/grafana/redshift-datasource/pull/177/files#diff-804a5756e358074cb0ba27b1932cca77d39b90694fc1d4d122f46348c8653259R113.

The backend would need to decide when to run a query sync or async. Right now, we decide that based on a feature toggle. But since external plugins don't have access to the Grafana backend feature toggles, we're reading it from the frontend and making the decision there.

Ah interesting. I wonder if it would somehow make sense to move this out of the query object then? Like it's own separate state concept? Open to whatever you think is best though!

Yea that's a good idea. I've moved this to an interface in the DatasourceWithAsyncBackend.ts file.

sarahzinger

more thinking out loud questions, exciting stuff

src/sql/QueryEditor/FillValueSelect.tsx

sarahzinger · 2022-09-01T19:42:24Z

src/sql/types.ts

@@ -7,4 +7,5 @@ export interface SQLQuery extends DataQuery {
  rawSQL: string;
  format?: number;
  fillMode?: { mode: FillValueOptions; value?: number };
+  meta?: { queryFlow?: 'async' | 'sync' };


is it odd that this would be saved in the query? How would we update old queries? if we have to revert after releasing would that be ok if queries are saved with this field?

I wonder if we could somehow right our frontend code to make the query not knowing if it is sync or async and then make different choices based on the response from the backend instead?

andresmgot · 2022-09-26T08:49:28Z

src/sql/QueryEditor/FillValueSelect.tsx

@@ -64,7 +64,7 @@ export function FillValueSelect<TQuery extends DataQuery & Record<string, any>>(
                },
              })
            }
-            onBlur={() => props.onRunQuery()}
+            onBlur={() => props?.onRunQuery?.()}


nit but props should be defined (at least according to the function signature), so all the calls should be props.onRunQuery?

andresmgot · 2022-09-28T09:04:52Z

src/sql/QueryEditor/FillValueSelect.tsx

@@ -43,7 +43,7 @@ export function FillValueSelect<TQuery extends DataQuery & Record<string, any>>(
              // Keep the fillMode.value in case FillValueOptions.Value mode is selected back
              fillMode: { ...props.query.fillMode, mode: value },
            });
-            props.onRunQuery();
+            props?.onRunQuery?.();


@iwysiu you only changed one, but all the calls should be props.onRunQuery? 😅

oops, i did not realize there were multiple

andresmgot · 2022-09-28T09:05:39Z

src/ConnectionConfig.tsx

+    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [currentProvider, options, onOptionsChange]);


mmh, why don't you add awsAllowedAuthProviders to the array?

kevinwcyu · 2022-10-07T18:08:47Z

src/requestLooper.test.ts

+  expect(actual).toEqual(expected);
+});
+
+describe('requestLooper', () => {


Here's the docs for RxJS testing https://rxjs.dev/guide/testing/marble-testing

sarahzinger

Didn't do any manual testing here, but let me know if you'd like me to!

I think this looks cool from a code perspective to me :) Small suggestion to add a PR description so that if we're ever looking through the git history we can get some more context.

src/sql/QueryEditor/FillValueSelect.tsx

sunker

Thanks for moving the async data stuff to this package. It makes it much easier to understand how this feature will be used as a service. Just one concern in the comments.

sunker · 2022-10-11T12:58:05Z

src/AsyncDatasourceWithBackend.ts

+  cancel(target: TQuery) {
+    this.storeQuery(target, { shouldCancel: true });
+  }
+}


I wonder if this file should be called DataSourceWithAsyncBackend instead? Thoughts?

sunker · 2022-10-11T13:13:40Z

rollup.config.ts

@@ -26,7 +26,7 @@ const buildCjsPackage = ({ env }) => {
        },
      },
    ],
-    external: ['react', '@grafana/data', '@grafana/ui', 'lodash'],
+    external: ['react', '@grafana/data', '@grafana/ui', '@grafana/runtime', 'lodash', 'rxjs'],


The reason this grafana/aws-sdk did not use grafana/runtime previously was because before Grafana 9.0, grafana/ui depended on grafana/aws-sdk. This created an implicit dependency between grafana/ui and grafana/runtime and that is really bad.

This dependency was removed in 9.0, so this should be totally fine for any user running that version or newer versions of Grafana. However, I believe AMG are still running 8.4.x, so I think the long running queries feature must be compatible with that version. Just to make sure I'm correct on this, can you try and build all repos involded in the long running queries feature together with Grafana 8.4.x? If this doesn't work, we may have to break out the AsyncDataSourceWithBackend.ts (that's the only file who needs the runtime, no?) into a new "AsyncDataSourceBackend" repo/npm package. That wouldn't be so bad anyway because the long term intent is that this feature is made available for wider use beyond just AWS plugins.

I pulled down Grafana v8.4.7 to test and I didn't see any errors.

Okay good, thanks for testing!

package.json

.drone.yml

sunker

Awesome work!

sunker · 2022-11-03T14:18:06Z

rollup.config.ts

@@ -26,7 +26,7 @@ const buildCjsPackage = ({ env }) => {
        },
      },
    ],
-    external: ['react', '@grafana/data', '@grafana/ui', 'lodash'],
+    external: ['react', '@grafana/data', '@grafana/ui', '@grafana/runtime', 'lodash', 'rxjs'],


Okay good, thanks for testing!

andresmgot · 2022-11-03T15:27:56Z

🚀

This reverts commit ce0695d, reversing changes made to b67da1e.

Revert "Merge pull request #21 from grafana/long-running-queries"

kevinwcyu added 3 commits August 26, 2022 14:47

make onRunQuery optional

dd4c323

add type to change query flow

bba341d

make onRunQuery optional

e9abedf

kevinwcyu requested a review from sunker August 30, 2022 19:45

make query flow optional

9c8f3a3

kevinwcyu commented Aug 31, 2022

View reviewed changes

kevinwcyu requested review from a team and fridgepoet and removed request for a team August 31, 2022 12:51

sarahzinger reviewed Sep 1, 2022

View reviewed changes

iwysiu marked this pull request as ready for review September 23, 2022 19:30

iwysiu requested a review from andresmgot September 23, 2022 19:30

andresmgot reviewed Sep 26, 2022

View reviewed changes

andres nit

14775b1

andresmgot reviewed Sep 28, 2022

View reviewed changes

memoize awsAllowedAuthProviders

fde1e16

andresmgot approved these changes Sep 28, 2022

View reviewed changes

kevinwcyu added 4 commits September 29, 2022 12:35

Add RunQueryButtons

77437e2

add async datasource with backend

4bcece5

some cleanup

3257733

add tests and some cleanup

e1e754d

kevinwcyu commented Oct 7, 2022

View reviewed changes

sarahzinger approved these changes Oct 11, 2022

View reviewed changes

src/sql/QueryEditor/FillValueSelect.tsx Show resolved Hide resolved

sunker reviewed Oct 11, 2022

View reviewed changes

kevinwcyu added 2 commits October 11, 2022 14:24

rename

98e2ae7

update yarn.lock to fix ci error

f111948

kevinwcyu force-pushed the long-running-queries branch 3 times, most recently from 0a916bb to 8772b7f Compare October 12, 2022 21:55

kevinwcyu force-pushed the long-running-queries branch 4 times, most recently from 36f0917 to 44ed604 Compare October 12, 2022 22:05

build without linking

5aba4d9

kevinwcyu force-pushed the long-running-queries branch from 44ed604 to 5aba4d9 Compare October 12, 2022 22:26

kevinwcyu commented Oct 13, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

kevinwcyu commented Oct 13, 2022

View reviewed changes

.drone.yml Outdated Show resolved Hide resolved

stop button cancels actual running query instead of edited query

da5bcac

kevinwcyu mentioned this pull request Oct 28, 2022

Update dependencies #24

Merged

Merge branch 'main' into long-running-queries

9ec4821

kevinwcyu requested a review from sunker November 1, 2022 13:56

move interface with query meta to DatasourceWithAsyncBackend

618c560

kevinwcyu requested a review from sarahzinger November 1, 2022 14:51

sunker approved these changes Nov 3, 2022

View reviewed changes

kevinwcyu merged commit ce0695d into main Nov 3, 2022

kevinwcyu deleted the long-running-queries branch November 3, 2022 14:53

kevinwcyu added a commit that referenced this pull request Nov 3, 2022

Revert "Merge pull request #21 from grafana/long-running-queries"

8863ea4

This reverts commit ce0695d, reversing changes made to b67da1e.

kevinwcyu added a commit that referenced this pull request Nov 4, 2022

Merge pull request #28 from grafana/revert-21

63bcbbd

Revert "Merge pull request #21 from grafana/long-running-queries"

This was referenced Nov 4, 2022

Make onRunQuery optional #29

Merged

Add release instructions grafana/grafana-async-query-data-js#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Long Running Queries #21

Long Running Queries #21

kevinwcyu commented Aug 30, 2022

CLAassistant commented Aug 30, 2022 •

edited

kevinwcyu Aug 31, 2022

sarahzinger Sep 1, 2022

kevinwcyu Sep 5, 2022

sarahzinger Sep 9, 2022

kevinwcyu Nov 1, 2022

sarahzinger left a comment

sarahzinger Sep 1, 2022

andresmgot Sep 26, 2022

andresmgot Sep 28, 2022

iwysiu Sep 28, 2022

andresmgot Sep 28, 2022

iwysiu Sep 28, 2022

kevinwcyu Oct 7, 2022

sarahzinger left a comment

sunker left a comment

sunker Oct 11, 2022

sunker Oct 11, 2022

kevinwcyu Oct 28, 2022

sunker Nov 3, 2022

sunker left a comment

sunker Nov 3, 2022

andresmgot commented Nov 3, 2022

		// eslint-disable-next-line react-hooks/exhaustive-deps
		}, [currentProvider, options, onOptionsChange]);

Long Running Queries #21

Long Running Queries #21

Conversation

kevinwcyu commented Aug 30, 2022

CLAassistant commented Aug 30, 2022 • 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

Choose a reason for hiding this comment

sarahzinger left a 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

sarahzinger left a comment

Choose a reason for hiding this comment

sunker left a 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

sunker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot commented Nov 3, 2022

CLAassistant commented Aug 30, 2022 •

edited