Skip to content

Commit

Permalink
Code review changes from Grafana and cleanup (#5)
Browse files Browse the repository at this point in the history 
* Code review changes from Grafana and cleanup

* Remove dynamic javascript from the node value field (missed last time)
* Remove Avg aggregation, which didn't work properly with the new
  expression logic.
* Fix rendering differences when the top layer border is toggled.
* Allow the value field (default field) to be a string or a number.
* Cosmetic fixes: Aggregate category is now Aggregation. hidden-node
  placeholder text now matches the expected format.
  • Loading branch information
@byronholldorf
byronholldorf authored Apr 29, 2024
1 parent 204ee24 commit f4b3bdf
Show file tree
Hide file tree
Showing 7 changed files with 27 additions and 72 deletions.
1 change: 0 additions & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,6 @@ In the case of multiple duplicate entries, use this method to merge them togethe
* None - Don't do any merging. If any duplicates exist, the most recent value from the query is used.
* Max - Maximum value of all duplicates.
* Min - Minimum value of all duplicates.
* Avg - The average of all duplicates.
* Last - The most recent value is used. This requires a Timestamp Field to be selected.

#### Timestamp Field
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "hpehpc-grafanaclusterview-panel",
"version": "1.3.0",
"version": "1.3.1",
"description": "A high density view of large amounts of data focused on high performance computing",
"scripts": {
"build": "webpack -c ./.config/webpack/webpack.config.ts --env production",
Expand Down
2 changes: 1 addition & 1 deletion src/ClusterviewPanel.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ export const ClusterviewPanel: React.FC<Props> = ({ options, data, width, height
}

// draw the panel
return <div style={{ overflow: 'hidden', height: '100%' }}>{draw(nodes)}</div>;
return <div style={{ overflow: 'hidden', height: '100%' , width: 'fit-content'}}>{draw(nodes)}</div>;
};

/**
Expand Down
20 changes: 8 additions & 12 deletions src/datastructure.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,6 @@ describe('Build data structure test', () => {
expect(_testPoints.lookupValue(fields, '1', 0)).toEqual(4);
expect(_testPoints.lookupValue(fields, 'value', 1)).toEqual(5);
expect(_testPoints.lookupValue(fields, '${Test}', 0)).toEqual(4);
expect(_testPoints.lookupValue(fields, "return fields['value']", 0)).toEqual(4);
expect(_testPoints.lookupValue(fields, 'return fields[1]', 0)).toEqual(4);
expect(_testPoints.lookupValue(fields, "return fields['${Test}']", 1)).toEqual(5);
expect(_testPoints.lookupValue(fields, 'return ${1}', 0)).toEqual(4);
});

test('Sort', () => {
Expand All @@ -146,14 +142,14 @@ describe('Build data structure test', () => {
options.hiddennodes = '[["A1", "\\w2"], ["A2", "B2", "C2"]]';

let data = new DataLevel('cluster');
data.addDataNode(['A1', 'B1', 'C1'], 'n1', 0, 12345, '', 0, -1);
data.addDataNode(['A1', 'B1', 'C2'], 'n2', 0, 12345, '', 0, -1);
data.addDataNode(['A1', 'B2', 'C1'], 'n3', 0, 12345, '', 0, -1);
data.addDataNode(['A1', 'B2', 'C2'], 'n4', 0, 12345, '', 0, -1);
data.addDataNode(['A2', 'B1', 'C1'], 'n5', 0, 12345, '', 0, -1);
data.addDataNode(['A2', 'B1', 'C2'], 'n6', 0, 12345, '', 0, -1);
data.addDataNode(['A2', 'B2', 'C1'], 'n7', 0, 12345, '', 0, -1);
data.addDataNode(['A2', 'B2', 'C2'], 'n8', 0, 12345, '', 0, -1);
data.addDataNode(['A1', 'B1', 'C1'], 'n1', "0", 12345, '', 0, -1);
data.addDataNode(['A1', 'B1', 'C2'], 'n2', "0", 12345, '', 0, -1);
data.addDataNode(['A1', 'B2', 'C1'], 'n3', "0", 12345, '', 0, -1);
data.addDataNode(['A1', 'B2', 'C2'], 'n4', "0", 12345, '', 0, -1);
data.addDataNode(['A2', 'B1', 'C1'], 'n5', "0", 12345, '', 0, -1);
data.addDataNode(['A2', 'B1', 'C2'], 'n6', "0", 12345, '', 0, -1);
data.addDataNode(['A2', 'B2', 'C1'], 'n7', "0", 12345, '', 0, -1);
data.addDataNode(['A2', 'B2', 'C2'], 'n8', "0", 12345, '', 0, -1);

// _testPoints.sortdata(data, options);

Expand Down
56 changes: 10 additions & 46 deletions src/datastructure.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ export function register(vars: InterpolateFunction) {
*/
export class Node {
text: string;
value: number | null;
value: string | null;
timestamp: number | null;
id: number;
colorIndex: number;
url: string | null;
static _id_count = 1;
constructor(text: string, value: number | null, ts: number | null, url: string | null, colorIndex=-1) {
constructor(text: string, value: string | null, ts: number | null, url: string | null, colorIndex=-1) {
this.text = text;
this.value = value;
this.timestamp = ts;
Expand Down Expand Up @@ -80,7 +80,7 @@ export class DataLevel {
* @param url a url fragment for clickability
* @param level what level are we currently at
*/
addDataNode(names: string[], text: string, value: number | null, ts: number, url: string, level: number, colorIndex: number) {
addDataNode(names: string[], text: string, value: string | null, ts: number, url: string, level: number, colorIndex: number) {
if (names.length) {
const nextLevel = this.fetchLevel(names[0]);
nextLevel.addDataNode(names.splice(1), text, value, ts, url, level + 1, colorIndex);
Expand Down Expand Up @@ -163,19 +163,6 @@ function buildParserForGroup(
}
}

function buildFields(fields: Field[], i: number) {
let result: { [key: string]: string } = {};
fields.forEach((v, j) => {
let value = v.values.get(i);
result[j] = value;
result[v.name] = value;
if (v.config?.displayName) {
result[v.config.displayName] = value;
}
});
return result;
}

/**
* The value needs to be more complex than just a simple lookup. Try the field
* as a raw number, as a generic field, and then as a chunk of javascript.
Expand All @@ -185,21 +172,18 @@ function buildFields(fields: Field[], i: number) {
* @param i Which index of the data we're on
* @returns the resulting value we should use
*/
function lookupValue(fields: Field[], txt: string, i: number) {
function lookupValue(fields: Field[], txt: string, i: number): string | null {
let rplTxt = replaceVariables(txt);
let valueindex = lookupFieldIndex(fields, rplTxt, false);
let value = null;
if (!isNaN(+valueindex!)) {
value = fields[valueindex!].values.get(i)
} else {
try {
value = Function('fields', rplTxt)(buildFields(fields, i));
} catch (e) {
console.error(e);
}
// unknown type. We'll just use it verbatim
value = txt
}
if (value !== null) {
return Number(value);
return value;
}
return null;
}
Expand Down Expand Up @@ -274,7 +258,7 @@ export const buildData = function (options: ClusterviewOptions, fields: Field[],
if (!options.ignoreNull || value != null) {
data.addDataNode(keys, nodetext, value, timestamp, nodeURL, 0, colorPicker.pickColor((x) => {
return transformText(x, fields, i, { value: String(value) });
}, String(value)));
}, value));
}
}

Expand Down Expand Up @@ -343,26 +327,6 @@ function aggregateData(data: DataLevel, options: ClusterviewOptions) {
}
else {
switch (options.aggregate) {
case 'avg':
let avgVal: number | null = null;
let avgTs: number | null = null;
let count = 0;
let firstNode = data.firstNotEmptyNode();
data.values.forEach((n) => {
if (n.value != null) {
avgVal! += n.value;
avgTs! += n.timestamp!;
count++;
}
});
if (count >0) {
avgVal! /= count;
avgTs! /= count;
}
data.values.length = 0;
data.values.push(new Node(firstNode.text, avgVal, avgTs, firstNode.url));

break;
case 'last':
aggregate(data.values, (a, b) => {
if (a.timestamp! == null) {
Expand All @@ -382,7 +346,7 @@ function aggregateData(data: DataLevel, options: ClusterviewOptions) {
if (b.value! == null) {
return true;
}
return a.value! < b.value!;
return Number(a.value) < Number(b.value);
});
break;
case 'max':
Expand All @@ -393,7 +357,7 @@ function aggregateData(data: DataLevel, options: ClusterviewOptions) {
if (b.value! == null) {
return true;
}
return a.value! > b.value!;
return Number(a.value) > Number(b.value);
});
break;
}
Expand Down
16 changes: 6 additions & 10 deletions src/module.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,10 +148,10 @@ export const plugin = new PanelPlugin<ClusterviewOptions>(ClusterviewPanel).setP
path: 'nodevalue',
name: 'Value Field',
category: ['Node'],
description: 'Field to use for value. index or name or javascript',
description: 'Field to use for value. index or name',
defaultValue: '1',
settings: {
placeholder: '1 fieldname `return field[3] || field["value"];`',
placeholder: '1 fieldname`',
},
})
.addTextInput({
Expand All @@ -161,7 +161,7 @@ export const plugin = new PanelPlugin<ClusterviewOptions>(ClusterviewPanel).setP
description: 'List(s) of regexs to filter out displayed nodes',
defaultValue: '',
settings: {
placeholder: '[[/x11/,/02/],[/x21/,//]]',
placeholder: '[["x11","02"],["x21",""]]',
},
})
.addNumberInput({
Expand Down Expand Up @@ -190,7 +190,7 @@ export const plugin = new PanelPlugin<ClusterviewOptions>(ClusterviewPanel).setP
.addSelect({
path: 'aggregate',
name: 'Aggregate data',
category: ['Aggregate'],
category: ['Aggregation'],
defaultValue: 'None',
settings: {
options: [
Expand All @@ -206,10 +206,6 @@ export const plugin = new PanelPlugin<ClusterviewOptions>(ClusterviewPanel).setP
label: 'Min',
value: 'min',
},
{
label: 'Avg',
value: 'avg',
},
{
label: 'Last',
value: 'last',
Expand All @@ -220,7 +216,7 @@ export const plugin = new PanelPlugin<ClusterviewOptions>(ClusterviewPanel).setP
.addTextInput({
path: 'timestampField',
name: 'Timestamp Field',
category: ['Aggregate'],
category: ['Aggregation'],
showIf: (config) => config.aggregate === 'last',
settings: {
placeholder: '1 fieldname',
Expand All @@ -229,7 +225,7 @@ export const plugin = new PanelPlugin<ClusterviewOptions>(ClusterviewPanel).setP
.addBooleanSwitch({
path: `ignoreNull`,
name: 'Ignore Null Values',
category: ['Aggregate'],
category: ['Aggregation'],
defaultValue: true,
})
.addColorPicker({
Expand Down
2 changes: 1 addition & 1 deletion src/types.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@

type LevelDisplayDirection = 'hz' | 'vt' | 'fl' | 'gr';
type Aggregate = 'none' | 'max' | 'min' | 'avg' | 'last';
type Aggregate = 'none' | 'max' | 'min' | 'last';

export interface Condition {
expression: string;
Expand Down

0 comments on commit f4b3bdf

Please sign in to comment.