From 1a444b2b3615a7eefaba8c3c875d96ca57a1f7ac Mon Sep 17 00:00:00 2001 From: Alex Barganier Date: Wed, 10 Apr 2024 20:11:24 +0000 Subject: [PATCH] ui: make custom chart tool work at store level Fixes: https://github.com/cockroachdb/cockroach/issues/121364 This patch fixes a bug in the DB Console custom chart tool, where selecting the "Per Node" checkbox on a metric would not properly display store-level metrics. The previous expected behavior was that the check box would cause the metric to aggregate across stores at the node level (e.g. if the node had 3 stores, it'd SUM the store-level timeseries together and return a single timeseries for the node). Instead, the feature was only showing the 1st store associated with the node. This was due to a bug in the code used to determine if a metric was store-level. A function was used that improperly assumed that the `cr.node.*` or `cr.store.*` prefix had been stripped from the metric name, which was not always the case. This led to us improperly interpret store-level metrics as node-level. The fix is to fix the logic used to determine if a metric is store-level. Additionally, this patch updates the code to no longer aggregate store-level metrics across each node. Instead, we will now show a single timeseries per-store to provide finer-grained observability into store-level metrics within the custom chart tool. Release note (bug fix): A bug has been fixed in the DB Console's custom chart tool, where store-level metrics were not being displayed properly. Previously, if a store-level metric was selected to be displayed at the store-level on a multi-store node, only data for the 1st store ID associated with that node would be displayed. This patch ensures that data is displayed for all stores present on a node. Additionally, it updates the behavior to show a single timeseries for each store, as opposed to aggregating (e.g. SUM) all stores across the node. This allows finer-grained observability into store-level metrics when using the custom chart tool in DB Console. --- .../containers/customChart/customMetric.tsx | 26 +++---- .../reports/containers/customChart/index.tsx | 74 +++++++++++++------ 2 files changed, 63 insertions(+), 37 deletions(-) diff --git a/pkg/ui/workspaces/db-console/src/views/reports/containers/customChart/customMetric.tsx b/pkg/ui/workspaces/db-console/src/views/reports/containers/customChart/customMetric.tsx index 0f7549dd241a..61e813b500e1 100644 --- a/pkg/ui/workspaces/db-console/src/views/reports/containers/customChart/customMetric.tsx +++ b/pkg/ui/workspaces/db-console/src/views/reports/containers/customChart/customMetric.tsx @@ -54,9 +54,9 @@ export class CustomMetricState { downsampler = TimeSeriesQueryAggregator.AVG; aggregator = TimeSeriesQueryAggregator.SUM; derivative = TimeSeriesQueryDerivative.NONE; - perNode = false; + perSource = false; perTenant = false; - source = ""; + nodeSource = ""; tenantSource = ""; } @@ -112,9 +112,9 @@ export class CustomMetricRow extends React.Component { }); }; - changeSource = (selectedOption: DropdownOption) => { + changeNodeSource = (selectedOption: DropdownOption) => { this.changeState({ - source: selectedOption.value, + nodeSource: selectedOption.value, }); }; @@ -124,9 +124,9 @@ export class CustomMetricRow extends React.Component { }); }; - changePerNode = (selection: React.FormEvent) => { + changePerSource = (selection: React.FormEvent) => { this.changeState({ - perNode: selection.currentTarget.checked, + perSource: selection.currentTarget.checked, }); }; @@ -151,8 +151,8 @@ export class CustomMetricRow extends React.Component { downsampler, aggregator, derivative, - source, - perNode, + nodeSource, + perSource, tenantSource, perTenant, }, @@ -217,17 +217,17 @@ export class CustomMetricRow extends React.Component { className="metric-table-dropdown__select" clearable={false} searchable={false} - value={source} + value={nodeSource} options={nodeOptions} - onChange={this.changeSource} + onChange={this.changeNodeSource} /> {canViewTenantOptions && ( @@ -340,7 +340,7 @@ export class CustomChartTable extends React.Component { Aggregator Rate Source - Per Node + Per Node/Store {canViewTenantOptions && ( Virtual Cluster )} diff --git a/pkg/ui/workspaces/db-console/src/views/reports/containers/customChart/index.tsx b/pkg/ui/workspaces/db-console/src/views/reports/containers/customChart/index.tsx index fcdccb77b5a3..c01662b6d16a 100644 --- a/pkg/ui/workspaces/db-console/src/views/reports/containers/customChart/index.tsx +++ b/pkg/ui/workspaces/db-console/src/views/reports/containers/customChart/index.tsx @@ -208,56 +208,79 @@ 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) => { const { nodesSummary, tenantOptions } = this.props; + // We require nodes information to determine sources (storeIDs/nodeIDs) down below. + if (!(nodesSummary?.nodeStatuses?.length > 0)) { + return; + } const tenants = tenantOptions.length > 1 ? tenantOptions.slice(1) : []; return metrics.map((m, i) => { if (m.metric === "") { return ""; } - if (m.perNode && m.perTenant) { - return _.flatMap(nodesSummary.nodeIDs, nodeID => { + if (m.perSource && m.perTenant) { + const sources = this.getSources(nodesSummary, m); + return _.flatMap(sources, source => { return tenants.map(tenant => ( - n.toString(), - ) - : [nodeID] - } + sources={[source]} tenantSource={tenant.value} /> )); }); - } else if (m.perNode) { - return _.map(nodesSummary.nodeIDs, nodeID => ( + } else if (m.perSource) { + const sources = this.getSources(nodesSummary, m); + return _.map(sources, source => ( - n.toString(), - ) - : [nodeID] - } + sources={[source]} tenantSource={m.tenantSource} /> )); } else if (m.perTenant) { + const sources = this.getSources(nodesSummary, m); return tenants.map(tenant => ( )); @@ -279,7 +302,7 @@ export class CustomChart extends React.Component< aggregator={m.aggregator} downsampler={m.downsampler} derivative={m.derivative} - sources={m.source === "" ? [] : [m.source]} + sources={this.getSources(nodesSummary, m)} tenantSource={m.tenantSource} /> ); @@ -402,5 +425,8 @@ export default withRouter( ); function isStoreMetric(nodeStatus: INodeStatus, metricName: string) { + if (metricName?.startsWith("cr.store")) { + return true; + } return _.has(nodeStatus.store_statuses[0].metrics, metricName); }