Add variable query to be able to fetch Pixie variables. #46

TarasPriadka · 2022-06-08T17:53:04Z

Adding ability to query Pixie Grafana go backend to fetch and display Pixie clusters. This PR doesn't add the functionality to switch clusters for the whole dashboard, as in dashboard variables are not going to function yet. Dashboard cluster change will be introduced in the next PR.

Structural changes:

Restructured Go query schema. Now looks like:

{
  queryType: str
  queryBody? {
    pxlScript?: str
  }
}

New query structure allows switching on different types of requests that we may add in the future

Go backend:

Refactored code into query type processor and query processor. Query type processor switches on different types of queries. Query processor actually sends a proper request using Pixie API based on the query type.
Query processing module now supports queries to run the pxl script, or queries vizier clusters.
Added function to query vizier clusters to support Cluster dashboard variable.

Front-end:

Add functionality to Datasource which sends a query to plugin backend.
Add VariableQueryEditor which adds a dropdown menu to Variables page in the dashboard settings and allows to create a dashboard variable. For now there's only a single item to get clusters.
Update PixieDataQuery interface to reflect the schema changes.

Signed-off-by: Taras Priadka tpriadka@pixielabs.ai

Signed-off-by: Taras Priadka <tpriadka@pixielabs.ai>

nserrino · 2022-06-08T19:45:53Z

pkg/pixie_plugin.go

@@ -99,7 +99,8 @@ func (td *PixieDatasource) QueryData(ctx context.Context, req *backend.QueryData

 type queryModel struct {
 	// The PxL script passed in by the user.
-	PxlScript string `json:"pxlScript"`
+	PxlScript   string `json:"pxlScript"`
+	ClusterFlag bool   `json:bool`


probably want json:"clusterFlag"

I think we should think about the design of this API a bit more - we can support autocomplete in the future for things like pod name, service name, node name, etc. This could be pretty powerful when adding filters to dashboard variables. Today, we wouldn't be able to do that without running a query across the entire cluster, but in theory we could do so. The Pixie API doesn't currently expose autocomplete, but maybe it could eventually (cc @aimichelle on that thought).

We do already expose the autocomplete API via gRPC, since the CLI used it for the cmd-K.

oops, never mind! then, i think we should consider adding in support for autocompleting service name, pod name, node name, etc to the grafana plugin as well. along with cluster ID

I think we should think about the design of this API a bit more - we can support autocomplete in the future for things like pod name, service name, node name, etc. This could be pretty powerful when adding filters to dashboard variables. Today, we wouldn't be able to do that without running a query across the entire cluster, but in theory we could do so. The Pixie API doesn't currently expose autocomplete, but maybe it could eventually (cc @aimichelle on that thought).

I am still working on the PR. Basically trying to get things to work bruteforce style since it is quite hard to do things which are slightly more different from their tutorials. I will refactor code a bit after everything works end-to-end to be more robust.

Not sure how to implement autocomplete yet, so I think it would be best to get some variable fetching to work first before doing anything with autocompletion.

Yeah, we don't have to support autocomplete or pod/etc suggestions in this PR. Let's just make sure the API changes we are adding are scalable to those being added in the future, so that the API doesn't churn.

Signed-off-by: Taras Priadka <tpriadka@pixielabs.ai>

aimichelle · 2022-06-09T22:06:55Z

pkg/pixie_plugin.go

+}
+
+type queryModel struct {
+	// The PxL script passed in by the user.


This comment is now incorrect right? This should probably be a comment above PxlScript in the queryBody struct.

aimichelle · 2022-06-09T22:08:46Z

pkg/pixie_plugin.go

+
+type queryModel struct {
+	// The PxL script passed in by the user.
+	QueryType string    `json:"queryType"`


We can make QueryType an enum just for some extra space efficiency (also prevents bugs from typos). See https://yourbasic.org/golang/iota/ Complete enum type with strings [best practice] section.

aimichelle · 2022-06-09T22:13:24Z

pkg/pixie_queries.go

+
+	response.Frames = append(response.Frames, vizierFrame)
+
+	backend.Logger.Debug(fmt.Sprintf("number of viziers: $d", len(viziers)))


rm these debug logs.

aimichelle · 2022-06-09T22:15:57Z

src/datasource.ts

 export class DataSource extends DataSourceWithBackend<PixieDataQuery, PixieDataSourceOptions> {
+  backendServ?: BackendSrv;


Nit: slightly different spellings of BackendSrv and backendServ we should just go with backendSrv for the variable name as well.

NickLanam

Looks sensible overall from the frontend side, just a few nits

NickLanam · 2022-06-09T22:50:02Z

src/types.ts

@@ -22,11 +22,17 @@ import { scriptOptions } from 'pxl_scripts';
 // PixieDataQuery is the interface representing a query in Pixie.
 // Pixie queries use PxL, Pixie's query language.
 export interface PixieDataQuery extends DataQuery {
-  pxlScript: string;
+  queryType: string;


Nit:

Likely, this should be a literal string type instead of a general one. Right now, the only value it can take is 'run-script'.

So if we do this instead, consumers can get type-checking to tell them if they make a typo or try to use a type of query that doesn't exist:

export type PixieDataQueryType = 'run-script' | 'foo' | 'bar'; export interface PixieDataQuery extends DataQuery { queryType: PixieDataQueryType; // ... } export const someQuery: Partial<PixieDataQuery> = { // The `as const` part lets TypeScript know to treat it as both a type and a value // rather than as an arbitrary string that could violate the constraint. queryType: 'run-script' as const, // ... };

Is it a better practice to do this through a literal string as you have described, or with a string enum?

NickLanam · 2022-06-09T22:52:24Z

src/variable_query_editor.tsx

+import React from 'react';
+
+interface VariableQueryProps {
+  query: string;


This should probably have a comment describing what it actually means (for instance, is it a PxL script or something else?). It also seems to be currently unused below, so there's no context to make it obvious yet.

NickLanam · 2022-06-09T22:53:46Z

src/query_editor.tsx

@@ -46,21 +46,21 @@ const editorStyle = {
 export class QueryEditor extends PureComponent<Props> {
  onPxlScriptChange(event: string) {
    const { onChange, query, onRunQuery } = this.props;
-    onChange({ ...query, pxlScript: event });
+    onChange({ ...query, queryType: 'run-script', queryBody: { pxlScript: event } });


Nit (same in onScriptSelect): even though this still fits on one line, it has a mix of things going on that could stand to live on multiple lines.

NickLanam · 2022-06-09T22:55:20Z

src/query_editor.tsx

@@ -71,7 +71,7 @@ export class QueryEditor extends PureComponent<Props> {
          defaultValue={scriptOptions[0]}
        />
        <Editor
-          value={pxlScript}
+          value={pxlScript === undefined ? '' : pxlScript}


Nit: this can be simplified to pxlScript ?? '' (null coalesce)

NickLanam · 2022-06-09T23:00:01Z

src/datasource.ts

+  }
+
+  async fetchMetricNames(options: any): Promise<FetchResponse | void> {
+    let refId = 'tempvar';


Suggested change

let refId = 'tempvar';

const refId = options?.variable?.name ?? 'tempvar';

NickLanam · 2022-06-09T23:00:40Z

src/datasource.ts

+
+    const range = options?.range as TimeRange;
+    const interpolatedQuery = {
+      refId: refId,


Suggested change

refId: refId,

refId,

Object shorthand syntax is nice for stuff like this

NickLanam · 2022-06-09T23:12:48Z

src/datasource.ts

+    const zippedData = new Array(df.length);
+
+    for (let i = 0; i < df.length; i++) {
+      zippedData[i] = {};
+    }
+
+    df.fields.forEach((field) => {
+      field.values.toArray().forEach((elem, i) => {
+        zippedData[i][field.name] = elem;
+      });
+    });
+
+    console.log(zippedData);
+    return zippedData;


Leftover log, and you don't need to call so many functions:

Suggested change

const zippedData = new Array(df.length);

for (let i = 0; i < df.length; i++) {

zippedData[i] = {};

}

df.fields.forEach((field) => {

field.values.toArray().forEach((elem, i) => {

zippedData[i][field.name] = elem;

});

});

console.log(zippedData);

return zippedData;

const zipped = Array(df.length).fill(null);

for (const field of df.fields) {

for (let i = 0; i < field.values.length; i++) {

zipped[i] ??= {};

zipped[i][field.name] = field.values.get(i);

}

}

return zipped;

Thanks, this is definitely a more readable function implementation.

Signed-off-by: Taras Priadka <tpriadka@pixielabs.ai>

aimichelle · 2022-06-10T20:46:17Z

pkg/pixie_plugin.go

+	return client, nil
+}
+
+//Specifies available query types


My remaining major nit is that we usually like to add a space after //. So: // Specifies available query types.

There are a few other places in the PR where we can clean this up.

Oh oops. Not sure why linter didn't take care of this

Signed-off-by: Taras Priadka <tpriadka@pixielabs.ai>

TarasPriadka added 3 commits June 8, 2022 10:50

Add variable query to be able to fetch Pixie variables.

d9ab7a3

Signed-off-by: Taras Priadka <tpriadka@pixielabs.ai>

Add vizier cluster request handler in go backend

a91a66b

Signed-off-by: Taras Priadka <tpriadka@pixielabs.ai>

Add frontend handling for cluster variable queries

f3d42ca

Signed-off-by: Taras Priadka <tpriadka@pixielabs.ai>

nserrino reviewed Jun 8, 2022

View reviewed changes

TarasPriadka added 6 commits June 8, 2022 15:25

Only fetch healthy clusters.

b201b06

Signed-off-by: Taras Priadka <tpriadka@pixielabs.ai>

Parse and display backend response

a05e1c5

Signed-off-by: Taras Priadka <tpriadka@pixielabs.ai>

Refactor Go Pixie API query code from Grafana query handling

40968d6

Signed-off-by: Taras Priadka <tpriadka@pixielabs.ai>

Use a switch to process different types of queries.

8841224

Signed-off-by: Taras Priadka <tpriadka@pixielabs.ai>

Make go JSON struct structured.

d0654a9

Signed-off-by: Taras Priadka <tpriadka@pixielabs.ai>

Refactor frontend to use new query schema

5871576

Signed-off-by: Taras Priadka <tpriadka@pixielabs.ai>

TarasPriadka marked this pull request as ready for review June 9, 2022 21:32

TarasPriadka requested review from aimichelle and nserrino June 9, 2022 21:32

Remove debug statements.

ada7ca1

Signed-off-by: Taras Priadka <tpriadka@pixielabs.ai>

aimichelle reviewed Jun 9, 2022

View reviewed changes

NickLanam reviewed Jun 9, 2022

View reviewed changes

Address comments.

91126d8

Signed-off-by: Taras Priadka <tpriadka@pixielabs.ai>

NickLanam approved these changes Jun 10, 2022

View reviewed changes

TarasPriadka requested a review from aimichelle June 10, 2022 20:46

aimichelle reviewed Jun 10, 2022

View reviewed changes

Add spaces.

2e65f65

Signed-off-by: Taras Priadka <tpriadka@pixielabs.ai>

TarasPriadka requested a review from aimichelle June 10, 2022 20:54

aimichelle approved these changes Jun 10, 2022

View reviewed changes

aimichelle merged commit 305bc00 into pixie-io:main Jun 10, 2022

TarasPriadka deleted the variable-queries branch June 10, 2022 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add variable query to be able to fetch Pixie variables. #46

Add variable query to be able to fetch Pixie variables. #46

TarasPriadka commented Jun 8, 2022 •

edited

Loading

nserrino Jun 8, 2022

nserrino Jun 8, 2022

aimichelle Jun 8, 2022

nserrino Jun 8, 2022

TarasPriadka Jun 8, 2022 •

edited

Loading

nserrino Jun 8, 2022

aimichelle Jun 9, 2022

aimichelle Jun 9, 2022

aimichelle Jun 9, 2022

aimichelle Jun 9, 2022

NickLanam left a comment

NickLanam Jun 9, 2022

TarasPriadka Jun 10, 2022

NickLanam Jun 9, 2022

NickLanam Jun 9, 2022

NickLanam Jun 9, 2022

NickLanam Jun 9, 2022

NickLanam Jun 9, 2022

NickLanam Jun 9, 2022

TarasPriadka Jun 10, 2022

aimichelle Jun 10, 2022

TarasPriadka Jun 10, 2022


		response.Frames = append(response.Frames, vizierFrame)

		backend.Logger.Debug(fmt.Sprintf("number of viziers: $d", len(viziers)))

		export class DataSource extends DataSourceWithBackend<PixieDataQuery, PixieDataSourceOptions> {
		backendServ?: BackendSrv;

	let refId = 'tempvar';
	const refId = options?.variable?.name ?? 'tempvar';

Add variable query to be able to fetch Pixie variables. #46

Add variable query to be able to fetch Pixie variables. #46

Conversation

TarasPriadka commented Jun 8, 2022 • edited Loading

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

TarasPriadka Jun 8, 2022 • edited Loading

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

NickLanam 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

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

TarasPriadka commented Jun 8, 2022 •

edited

Loading

TarasPriadka Jun 8, 2022 •

edited

Loading