Added script switch button to the query panel. #41

TarasPriadka · 2022-06-01T21:51:59Z

Added a dropdown menu with some predefined scripts to choose from.

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

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

htroisi · 2022-06-01T21:54:31Z

src/pxl_scripts.tsx

+};
+
+export const options = [
+  { label: 'scratch-pad' },


Would Custom Query be more descriptive than Scratch Pad?

I was going with the same name as on Pixie UI. Can change to Custom Query if that is more descriptive.

Let's make it Custom Query, I think that is more clear to the Grafana audience

TarasPriadka · 2022-06-01T21:58:15Z

What is the best way to keep track of predefined scripts? Keeping all of them in the tsx file doesn't seem right. Should I configure yarn so that it copies example scripts into dist on build? @vihangm @nserrino

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

nserrino · 2022-06-02T20:40:14Z

What is the best way to keep track of predefined scripts? Keeping all of them in the tsx file doesn't seem right. Should I configure yarn so that it copies example scripts into dist on build? @vihangm @nserrino

I think we should have a directory, containing a JSON file for each predefined script

nserrino · 2022-06-02T20:31:16Z

src/pxl_scripts.tsx

+px.display(per_ns_df['time_', 'service', 'request_throughput'])
+`,
+
+  'http-data-filtered': String.raw`# Copyright 2018- The Pixie Authors.


We can remove the copyright in these

nserrino · 2022-06-02T20:32:12Z

src/pxl_scripts.tsx

+};
+
+export const options = [
+  { label: 'scratch-pad' },


Let's make it Custom Query, I think that is more clear to the Grafana audience

nserrino · 2022-06-02T20:33:00Z

src/pxl_scripts.tsx

+  'scratch-pad': '',
+  example: String.raw`
+# Import Pixie's module for querying data.
+import px


Let's put some links to docs.px.dev in the comments at the top of this script, perhaps links to the Grafana tutorial and also how to write your own PxL script

nserrino · 2022-06-02T20:37:33Z

src/pxl_scripts.tsx

+px.display(per_ns_df['time_', 'service', 'request_throughput'])
+`,
+
+  'http-data-filtered': String.raw`# Copyright 2018- The Pixie Authors.


Let's have the title of this one be

Raw HTTP Events (Long Format)

So it's clear that it shouldn't be used with a timeseries and more clear about what the datasource is

nserrino · 2022-06-02T20:38:10Z

src/pxl_scripts.tsx

+#df = df[px.contains(df.pod, 'catalogue')]
+#df = df[df.req_path == '/healthz']
+
+# Avoid conversion to long format


I know this isn't your comment, but I believe this is actually avoiding the conversion to wide format, not the other way around: https://grafana.com/docs/grafana/latest/developers/plugins/data-frames/

nserrino · 2022-06-02T20:39:25Z

src/pxl_scripts.tsx

+# Output the DataFrame
+px.display(df)
+`,
+  'http-errors-per-service': String.raw`


Let's rename this to

HTTP Error Rate by Service (Wide Format)

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

aimichelle · 2022-06-03T18:32:54Z

src/query_editor.tsx

@@ -33,12 +34,26 @@ export class QueryEditor extends PureComponent<Props> {
    onRunQuery();
  }

+  onSelect(option: SelectableValue<number>) {


Kartik is working on adding some more dropdowns (for example, for column selection, groupbys). Let's name this onSelect something more specific to avoid confusion. Something like: onScriptSelect.

aimichelle · 2022-06-03T18:35:19Z

src/pxl_scripts.tsx

+export const options = scriptsRaw.map((script: Script) => {
+  return {
+    label: script.name,
+    description: script.description,


Maybe a nice, small follow-up PR is to figure out how to incorporate/show this description when you're looking through scripts in the dropdown. This is just a UI improvement thing though, so just a nice-to-have (and shouldn't be done in this PR).

This is already done through Grafana UI component. Additionally it can display images near the selections, but I don't think we need that.

aimichelle · 2022-06-03T18:38:10Z

src/pxl_scripts.tsx

+);
+
+// Construct options list which is injested by Select component in
+export const options = scriptsRaw.map((script: Script) => {


options is pretty vague here, so when it gets imported into query_editor.tsx, it can be unclear what options means, especially if there are potentially other dropdowns. Let's just rename this to scriptOptions.

NickLanam · 2022-06-03T18:48:23Z

src/pxl_scripts.tsx

+const scriptsRaw: Script[] = [customQuery, exampleQuery, httpDataFiltered, httpErrorsPerService];
+
+// Make a map for easier access of scripts
+export const scripts = new Map(


As a general rule (only writing it once since it's the same thing elsewhere):

Even though TypeScript can often figure out the type of something for you, it's still a good idea to specify it explicitly when:

The symbol is exported and isn't a primitive or otherwise trivial type

The type may not be instantly obvious to the reader even if TypeScript inferred it correctly (this is clearly a Map, but I have to read surrounding code to realize it's actually a Map<string, Script> and not a Map<string, string>)

The contents of the object might be complex, and specifying the type will let the compiler tell you if you didn't fill it out properly.

In this specific case, export const scripts = new Map<string, Script>(...) would make it nice and clear.

NickLanam · 2022-06-03T18:49:27Z

src/pxl_scripts.tsx

+export const scripts = new Map(
+  scriptsRaw.map((script) => {
+    return [script.name, script];
+  })


Nit: this is a good candidate for shorthand syntax.

scriptsRaw.map((script) => [script.name, script]);

NickLanam · 2022-06-03T18:52:09Z

src/pxl_scripts.tsx

+
+// Construct options list which is injested by Select component in
+export const options = scriptsRaw.map((script: Script) => {
+  return {


Nit: shorthand syntax also works for objects if you wrap them in parentheses, like this:

export const scriptOptions: Array<{ label: string, description: string }> = scriptsRaw.map((script) => ({ label: script.name, description: script.description, }));

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

Added script switch button to the query panel.

94949e0

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

htroisi reviewed Jun 1, 2022

View reviewed changes

Added licensing to the pxl scripts file.

43672a8

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

nserrino reviewed Jun 2, 2022

View reviewed changes

Put scripts into separate files and load them

d6c902c

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

TarasPriadka requested review from nserrino and htroisi June 3, 2022 18:05

Add whitespace to json, remove unnecessary arguments

faaa071

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

TarasPriadka mentioned this pull request Jun 3, 2022

Syntax highlighting #42

Merged

aimichelle reviewed Jun 3, 2022

View reviewed changes

NickLanam reviewed Jun 3, 2022

View reviewed changes

Addressing comments.

3dd1c40

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

aimichelle approved these changes Jun 3, 2022

View reviewed changes

NickLanam approved these changes Jun 3, 2022

View reviewed changes

aimichelle merged commit 736a760 into pixie-io:main Jun 3, 2022

TarasPriadka deleted the select-script branch June 3, 2022 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added script switch button to the query panel. #41

Added script switch button to the query panel. #41

TarasPriadka commented Jun 1, 2022

htroisi Jun 1, 2022

TarasPriadka Jun 1, 2022

nserrino Jun 2, 2022

TarasPriadka commented Jun 1, 2022 •

edited

Loading

nserrino commented Jun 2, 2022

nserrino Jun 2, 2022

nserrino Jun 2, 2022

nserrino Jun 2, 2022

nserrino Jun 2, 2022

nserrino Jun 2, 2022

nserrino Jun 2, 2022

aimichelle Jun 3, 2022

aimichelle Jun 3, 2022

TarasPriadka Jun 3, 2022

aimichelle Jun 3, 2022

NickLanam Jun 3, 2022

NickLanam Jun 3, 2022

NickLanam Jun 3, 2022

Added script switch button to the query panel. #41

Added script switch button to the query panel. #41

Conversation

TarasPriadka commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TarasPriadka commented Jun 1, 2022 • edited Loading

nserrino commented Jun 2, 2022

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

TarasPriadka commented Jun 1, 2022 •

edited

Loading