Skip to content

Commit

Permalink
ui: tests for custom chart logic to determine metric sources
Browse files Browse the repository at this point in the history
#122151 fixed the custom
chart tool to properly display store-level metrics. However, it did not
include any tests. Despite metric charts being difficult to unit test,
we can still test the logic used to determine which metric sources
should be included in the timeseries query.

This commit adds tests for this logic.

Release note: none
  • Loading branch information
abarganier committed Apr 25, 2024
1 parent 001375a commit d03a367
Show file tree
Hide file tree
Showing 2 changed files with 202 additions and 32 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
import { NodesSummary } from "src/redux/nodes";
import { INodeStatus } from "src/util/proto";
import { GetSources } from "src/views/reports/containers/customChart/index";
import * as protos from "src/js/protos";
import { CustomMetricState } from "src/views/reports/containers/customChart/customMetric";

import TimeSeriesQueryAggregator = protos.cockroach.ts.tspb.TimeSeriesQueryAggregator;
import TimeSeriesQueryDerivative = protos.cockroach.ts.tspb.TimeSeriesQueryDerivative;

describe("Custom charts page", function () {
describe("Getting metric sources", function () {
it("returns empty when nodesSummary is undefined", function () {
const metricState = new testCustomMetricStateBuilder().build();
expect(GetSources(undefined, metricState)).toStrictEqual([]);
});

it("returns empty when the nodeStatuses collection is empty", function () {
const nodesSummary = new testNodesSummaryBuilder().build();
nodesSummary.nodeStatuses = [];
const metricState = new testCustomMetricStateBuilder().build();
expect(GetSources(nodesSummary, metricState)).toStrictEqual([]);
});

it("returns empty when no specific node source is requested, nor per-source metrics", function () {
const nodesSummary = new testNodesSummaryBuilder().build();
const metricState = new testCustomMetricStateBuilder()
.setNodeSource("")
.setIsPerSource(false)
.build();
expect(GetSources(nodesSummary, metricState)).toStrictEqual([]);
});

describe("The metric is at the store-level", function () {
const storeMetricName = "cr.store.metric";

it("returns the store IDs associated with a specific node when a node source is set", function () {
const expectedSources = ["1", "2", "3"];
const metricState = new testCustomMetricStateBuilder()
.setName(storeMetricName)
.setNodeSource("1")
.build();
const nodesSummary = new testNodesSummaryBuilder()
.setStoreIDsByNodeID({
"1": expectedSources,
})
.build();
expect(GetSources(nodesSummary, metricState)).toStrictEqual(
expectedSources,
);
});

it("returns all known store IDs for the cluster when no node source is set", function () {
const expectedSources = ["1", "2", "3", "4", "5", "6", "7", "8", "9"];
const metricState = new testCustomMetricStateBuilder()
.setName(storeMetricName)
.build();
const nodesSummary = new testNodesSummaryBuilder()
.setStoreIDsByNodeID({
"1": ["1", "2", "3"],
"2": ["4", "5", "6"],
"3": ["7", "8", "9"],
})
.build();
const actualSources = GetSources(nodesSummary, metricState).sort();
expect(actualSources).toStrictEqual(expectedSources);
});
});

describe("The metric is at the node-level", function () {
const nodeMetricName = "cr.node.metric";

it("returns the specified node source when a node source is set", function () {
const expectedSources = ["1"];
const metricState = new testCustomMetricStateBuilder()
.setName(nodeMetricName)
.setNodeSource("1")
.build();
const nodesSummary = new testNodesSummaryBuilder().build();
expect(GetSources(nodesSummary, metricState)).toStrictEqual(
expectedSources,
);
});

it("returns all known node IDs when no node source is set", function () {
const expectedSources = ["1", "2", "3"];
const metricState = new testCustomMetricStateBuilder()
.setName(nodeMetricName)
.build();
const nodesSummary = new testNodesSummaryBuilder()
.setNodeIDs(["1", "2", "3"])
.build();
expect(GetSources(nodesSummary, metricState)).toStrictEqual(
expectedSources,
);
});
});
});
});

class testCustomMetricStateBuilder {
name: string;
nodeSource: string;
perSource: boolean;

setName(name: string): testCustomMetricStateBuilder {
this.name = name;
return this;
}

setNodeSource(nodeSource: string): testCustomMetricStateBuilder {
this.nodeSource = nodeSource;
return this;
}

setIsPerSource(perSource: boolean): testCustomMetricStateBuilder {
this.perSource = perSource;
return this;
}

build(): CustomMetricState {
return {
metric: this.name,
downsampler: TimeSeriesQueryAggregator.AVG,
aggregator: TimeSeriesQueryAggregator.SUM,
derivative: TimeSeriesQueryDerivative.NONE,
perSource: this.perSource,
perTenant: false,
nodeSource: this.nodeSource,
tenantSource: "",
};
}
}

class testNodesSummaryBuilder {
nodeStatuses: INodeStatus[];
storeIDsByNodeID: { [key: string]: string[] };
nodeIDs: string[];

setStoreIDsByNodeID(storeIDsByNodeID: {
[key: string]: string[];
}): testNodesSummaryBuilder {
this.storeIDsByNodeID = storeIDsByNodeID;
return this;
}

setNodeIDs(nodeIDs: string[]): testNodesSummaryBuilder {
this.nodeIDs = nodeIDs;
return this;
}

build(): NodesSummary {
return {
// We normally don't care about the nodeStatuses elements, so long as it's not an empty list.
// Populate with an empty object.
nodeStatuses: [
{
// We also need a non-empty list of store_statuses, for the isStoreMetric() call made.
store_statuses: [{}],
},
],
nodeIDs: this.nodeIDs,
nodeStatusByID: { "": {} },
nodeDisplayNameByID: { "": "" },
livenessStatusByNodeID: {},
livenessByNodeID: {},
storeIDsByNodeID: this.storeIDsByNodeID,
nodeLastError: undefined,
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,34 @@ interface UrlState {
charts: string;
}

export const GetSources = (
nodesSummary: NodesSummary,
metricState: CustomMetricState,
): string[] => {
if (!(nodesSummary?.nodeStatuses?.length > 0)) {
return [];
}
// If we have no nodeSource, and we're not asking for perSource metrics,
// then the user is asking for cluster-wide metrics. We can return an empty
// source list.
if (metricState.nodeSource === "" && !metricState.perSource) {
return [];
}
if (isStoreMetric(nodesSummary.nodeStatuses[0], metricState.metric)) {
// If a specific node is selected, return the storeIDs associated with that node.
// Otherwise, we're at the cluster level, so we grab each store ID.
return metricState.nodeSource
? nodesSummary.storeIDsByNodeID[metricState.nodeSource]
: Object.values(nodesSummary.storeIDsByNodeID).flatMap(s => s);
} else {
// If it's not a store metric, and a specific nodeSource is chosen, just return that.
// Otherwise, return all known node IDs.
return metricState.nodeSource
? [metricState.nodeSource]
: nodesSummary.nodeIDs;
}
};

export class CustomChart extends React.Component<
CustomChartProps & RouteComponentProps
> {
Expand Down Expand Up @@ -208,34 +236,6 @@ export class CustomChart extends React.Component<
);
};

getSources = (
nodesSummary: NodesSummary,
metricState: CustomMetricState,
): string[] => {
if (!(nodesSummary?.nodeStatuses?.length > 0)) {
return [];
}
// If we have no nodeSource, and we're not asking for perSource metrics,
// then the user is asking for cluster-wide metrics. We can return an empty
// source list.
if (metricState.nodeSource === "" && !metricState.perSource) {
return [];
}
if (isStoreMetric(nodesSummary.nodeStatuses[0], metricState.metric)) {
// If a specific node is selected, return the storeIDs associated with that node.
// Otherwise, we're at the cluster level, so we grab each store ID.
return metricState.nodeSource
? nodesSummary.storeIDsByNodeID[metricState.nodeSource]
: Object.values(nodesSummary.storeIDsByNodeID).flatMap(s => s);
} else {
// If it's not a store metric, and a specific nodeSource is chosen, just return that.
// Otherwise, return all known node IDs.
return metricState.nodeSource
? [metricState.nodeSource]
: nodesSummary.nodeIDs;
}
};

// This function handles the logic related to creating Metric components
// based on perNode and perTenant flags.
renderMetricComponents = (metrics: CustomMetricState[], index: number) => {
Expand All @@ -250,7 +250,7 @@ export class CustomChart extends React.Component<
return "";
}
if (m.perSource && m.perTenant) {
const sources = this.getSources(nodesSummary, m);
const sources = GetSources(nodesSummary, m);
return _.flatMap(sources, source => {
return tenants.map(tenant => (
<Metric
Expand All @@ -266,7 +266,7 @@ export class CustomChart extends React.Component<
));
});
} else if (m.perSource) {
const sources = this.getSources(nodesSummary, m);
const sources = GetSources(nodesSummary, m);
return _.map(sources, source => (
<Metric
key={`${index}${i}${source}`}
Expand All @@ -280,7 +280,7 @@ export class CustomChart extends React.Component<
/>
));
} else if (m.perTenant) {
const sources = this.getSources(nodesSummary, m);
const sources = GetSources(nodesSummary, m);
return tenants.map(tenant => (
<Metric
key={`${index}${i}${tenant.value}`}
Expand All @@ -302,7 +302,7 @@ export class CustomChart extends React.Component<
aggregator={m.aggregator}
downsampler={m.downsampler}
derivative={m.derivative}
sources={this.getSources(nodesSummary, m)}
sources={GetSources(nodesSummary, m)}
tenantSource={m.tenantSource}
/>
);
Expand Down

0 comments on commit d03a367

Please sign in to comment.