Adding support to filter columns for the Pod Metrics script

[kpattaswamy]

Added a drop down select option for a user to choose what columns to filter in the Pods-Metrics table. Did this by adding support to dynamically replace part of the px script dealing with displaying all columns with only columns chosen by user.

Signed-off-by: Kartik Pattaswamy kpattaswamy@pixielabs.ai

[nserrino]

we should put these pod column options as a property of the JSON for each script. that way, we don't hard code a particular script into the UI - it's all configurable via JSON

[kpattaswamy]

Ok I'll add them as a property of JSON for the script

[TarasPriadka]

A few stylistic and structural comments

[TarasPriadka]

Is it possible to use the user script if it has the filtering line in it? I think it may be slightly annoying for users if filtering disregards all of the changes they made.

Also, a thought for the future, we can send columns which we want to filter and then remove them in the go backend instead of altering the script.

[TarasPriadka]

You can store podColumnQueries in the value field of podColumnOptions instead of indicies. You'd need to change.

Also add type Array<SelectableValue<Type>> for this variable where Type is the type of the value. If you use store pod column options here, you'd use string.

[TarasPriadka]

Maybe add an optional field in the JSONs tabular = true so you don't have to hardcode the script names here in code.

[TarasPriadka]

obj is not a descriptive name for a variable. Also any[] is a very broad type, specify a tighter type instead. Specify a return type as well(ex.

function add(x: number, y: number): number {
  return x + y;
}

)

[TarasPriadka]

Don't use var since it is not scoped properly in JS. Either use let for mutable variables or const. Specify a proper type.

[TarasPriadka]

Default scripts should have a short docstring documenting what the script does with a link to grafana documentation. Look at other default scripts for reference.

[TarasPriadka]

I'd suggest adding a boolean field tabular to the query object instead of tracking tabular script names in a list.

[TarasPriadka]

Use variable name filteredScript, its already clear that it is constant from the const keyword

[aimichelle]

Nit: You can shorten this to query.isTabular ? ( ...

[NickLanam]

Since the other path is just an empty fragment, you can even just do query.isTabular && (...) and not have a ternary at all. React renders nothing at all if it's told to render a literal false, so a && b resolves to either rendering nothing or rendering b.

[aimichelle]

Let's rename the input ID to something more specific, like column-select.

[aimichelle]

For the columnNames field in the json files, it seems redundant to specify the value since arrays are inherently ordered anyway.

I think restructuring the JSON file to just be:
"columnNames": ["timestamp", "remote_addr", ...]

Then here, we convert it to the values/label obj:
columnOptions: scriptObject.columnNames.map((name, index) => { 'label': name, 'value': index })

[aimichelle]

My worry here is if there's a script that uses [[ and ]] multiple times, and not just in the final output. For example, if you take a look at the px/cluster script's conn_stats_by_service function.

Luckily these scripts are written by ourselves, so we can make sure that doesn't happen. But we may to be a bit more robust and search for the last indexOf [[ and ]]. You can use lastIndexOf for this.

[NickLanam]

I'm in agreement that scanning for this substring seems uncomfortably fragile. Template variables like you're already doing in datasource.tsx (or some other format that is definitely not valid PxL) are a more robust choice here since you're controlling the preset scripts in this same repository. px/cluster, as mentioned, has at least three [[something]]s in it, how do you know for sure which one should be used?

[TarasPriadka]

We could try creating some macro which is easy to find without collisions with user code. #45 (comment)

[NickLanam]

Largely LGTM, just a couple of questions (see below)

[NickLanam]

Since the other path is just an empty fragment, you can even just do query.isTabular && (...) and not have a ternary at all. React renders nothing at all if it's told to render a literal false, so a && b resolves to either rendering nothing or rendering b.

[NickLanam]

I'm in agreement that scanning for this substring seems uncomfortably fragile. Template variables like you're already doing in datasource.tsx (or some other format that is definitely not valid PxL) are a more robust choice here since you're controlling the preset scripts in this same repository. px/cluster, as mentioned, has at least three [[something]]s in it, how do you know for sure which one should be used?

[TarasPriadka]

Looks good overall. Had some small nits

[TarasPriadka]

Make these fields optional since not all scripts may have these fields.

Suggested change

	isTabular: boolean;
	columnOptions: Array<{ label: string; value: number }>;
	isTabular?: boolean;
	columnOptions?: Array<{ label: string; value: number }>;

[aimichelle]

This is a good suggestion.

[TarasPriadka]

If isTabular is optional in the query object, you don't have to add this field to every script. Same goes for all changed script files.

[TarasPriadka]

Maybe we can introduce some sort of macro which users can use to specify the column names? We can find that macro then and process it in a predictable way.
Ex.

...
COLUMNS{all} // or COLUMNS{'pod', 'namespace', 'latency', ...}
...

Then later find this macro in code, process the columns that users want to display, and replace this macro with df.display in query.pxlScript before sending the code query.

[TarasPriadka]

You already have a method which does this.

Suggested change

	//Dynamically builds the script by concatenating each option that was chosen
	filteredColumnScript = `[[`;
	for (let i = 0; i < chosenOptions.length; i++) {
	filteredColumnScript += formatColumnName(chosenOptions[i].label);
	//Dynamically builds the script by concatenating each option that was chosen
	filteredColumnScript = getAllColumnScript(chosenOptions);

[TarasPriadka]

Can join an array of values, and also should probably name this something general like getScriptFromColumns:

Suggested change

	//Returns a script which can be used to display all columns
	function getAllColumnScript(columnNames: Array<{ label: string; value: number }>): string {
	let script = '[[';
	for (let i = 0; i < columnNames.length; i++) {
	script += formatColumnName(columnNames[i].label);
	}
	return script;
	}
	function getScriptFromColumns(columnNames: Array<{ label: string; value: number }>): string {
	return [
	'[[',
	...columnNames.map(colName=>`'${colName.label}'`),
	']]'
	].join()
	}

This basically constructs your string where the mid part just formats colNames the way you want.

[TarasPriadka]

For readability, can do something like

Suggested change

	timeVars.forEach((replace) => {
	query.pxlScript = query.pxlScript.replaceAll(replace[0], replace[1]);
	for (const [changeFrom, changeTo] of timeVars) {
	query.pxlScript = query.pxlScript.replaceAll(replaceFrom, replaceTo);
	}

[TarasPriadka]

Since these may be not present in some scripts, you can make these fields optional.

Suggested change

	isTabular: boolean;
	columnNames: string[];
	isTabular?: boolean;
	columnNames?: string[];

[TarasPriadka]

These are already a part of scriptObject, so you don't need to add these as fields. Also SelectableValue doesn't have these fields, so its better to avoid adding them directly to the object.

[TarasPriadka]

Suggested change

	const script = option.value;
	const script:Script = option.value;

[TarasPriadka]

In React you want to call functions which get called on change or on click as on[name of action which is happening]. So in this case you could call this method onFilterSelect.

[aimichelle]

Javascript has variables can be defined as let or const.
let is usually used when you know the variable is going to be reassigned:

let a = 1; 
a = 2;

However, const should be used when you essentially have a constant.

const a = 1;
a = 2; // You shouldn't do this!

In the case of timeVars, this can be a const. And columnVars below as well.
You can actually pull them out of this function and move them to the top of the file (defined on the same level as functions). In general, we like to pull out constants to the top so that they're easy to find and change if needbe.

[aimichelle]

Nit: We like to make sure theres a space after the // in our comments. So this should be: // Replace Grafana's...

This is usually enforced by a linter, so let me look into why that's not set up yet so that it's easier to keep in mind for your next PRs.

[aimichelle]

This is a good suggestion.