chihsuan commented on code in PR #11243: URL: https://github.com/apache/ozone/pull/11243#discussion_r4145400331
########## ozone-ui/packages/om/src/api/metrics.ts: ########## @@ -0,0 +1,346 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** JMX query for the OM metrics bean (RPC operation counters + object counts). */ +export const OM_METRICS_QUERY = 'Hadoop:service=OzoneManager,name=OMMetrics'; + +/** + * Operation categories exposed by `OMMetrics` as `Num<Type><Op>` counters. Order + * seeds the metric-type dropdown; a type with no activity is disabled there. Any + * category found in the bean but missing here is still surfaced (appended), so a + * newly added OM metric type is never silently dropped. + */ +export const METRIC_TYPES = [ + 'Get', + 'Abort', + 'Add', + 'Block', + 'Bucket', + 'Cancel', + 'Commit', + 'Complete', + 'Create', + 'Delete', + 'Expired', + 'Follower', + 'Initiate', + 'Key', + 'Leader', + 'Linearizable', + 'List', + 'Lookup', + 'Open', + 'Put', + 'Recover', + 'Remove', + 'Set', + 'Snapshot', + 'Tenant', + 'Trash', + 'Volume', +] as const; + +export type MetricType = (typeof METRIC_TYPES)[number]; + +/** Raw OM metrics JMX bean — dynamic `Num*`/count keys plus string tags. */ +export type OMMetricsBean = Record<string, number | string>; + +export type OperationStatus = 'Active' | 'Warning' | 'Inactive'; + +export interface MetricOperation { + key: string; + /** Canonical operation label (e.g. `Allocate`, `Commit`, `Delete`). */ + name: string; + /** Successful request count for this operation. */ + requests: number; + /** Failure count for this operation. */ + failures: number; + status: OperationStatus; +} + +export interface MetricTypeData { + type: string; + /** Total requests: the sum of the requests of the operations shown for this type. */ + totalRequests: number; + /** Operations with any activity (requests or failures), busiest first. */ + operations: MetricOperation[]; + /** False when the type has no activity at all — its dropdown option is greyed out. */ + enabled: boolean; +} + +export interface MetricsSummary { + volumes: number; + buckets: number; + keys: number; + totalCommittedBytes: number; +} + +export interface ParsedOmMetrics { + summary: MetricsSummary; + byType: Record<string, MetricTypeData>; +} + +/** + * Request metric key: `Num<Type><Op>` — two CamelCase segments after `Num`, e.g. + * `NumKeyCommits` → type `Key`, op `Commits`. Single-word counters such as + * `NumKeys`/`NumVolumes` (no `<Op>` segment) intentionally do not match, so object + * counts are never treated as operations. + */ +const REQUEST_KEY_RE = /^Num([A-Z][a-z]+)([A-Z].+)$/; Review Comment: Thanks for trying the catalog, it made the parser much easier to follow! I noticed that **Other** still counts everything outside the catalog as requests, including aggregates like `NumKeyOps`. Could we list those as plain values, like v1 did? ########## ozone-ui/packages/om/src/App.tsx: ########## @@ -89,12 +90,19 @@ function AppShell() { <Routes> <Route path="/" element={<OverviewPage />} /> <Route path="/configuration" element={<Placeholder title="Configuration" />} /> - <Route path="/rpc" element={<Placeholder title="Remote Procedure Call" />} /> - <Route path="/ozone-manager" element={<Placeholder title="Ozone Manager" />} /> - <Route path="/jmx-info" element={<Placeholder title="JMX" />} /> + {/* Metrics group */} + <Route path="/metrics/rpc" element={<Placeholder title="Remote Procedure Call" />} /> + <Route + path="/metrics/ratis-event-timeline" + element={<Placeholder title="Ratis Event Timeline" />} + /> + <Route path="/metrics/ozone-manager" element={<MetricsPage />} /> + <Route path="/metrics/deletion" element={<Placeholder title="Deletion" />} /> + <Route path="/metrics/snapshots" element={<Placeholder title="Snapshots" />} /> + {/* Common tools group */} + <Route path="/jmx" element={<Placeholder title="JMX" />} /> Review Comment: Just curious, why did this change from `/jmx-info` to `/jmx`? If I reload this page in dev, I get OM's JMX JSON instead of the app. ########## ozone-ui/packages/om/src/api/metrics.ts: ########## @@ -0,0 +1,186 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { METRIC_TYPES, OM_OPERATIONS, OTHER_TYPE, SUMMARY_KEYS } from './metricsCatalog'; + +export { METRIC_TYPES, OTHER_TYPE } from './metricsCatalog'; +export type { MetricType, OmOperationDef } from './metricsCatalog'; + +/** JMX query for the OM metrics bean (RPC operation counters + object counts). */ +export const OM_METRICS_QUERY = 'Hadoop:service=OzoneManager,name=OMMetrics'; + +/** Raw OM metrics JMX bean — dynamic `Num*`/count keys plus string tags. */ +export type OMMetricsBean = Record<string, number | string>; + +export type OperationStatus = 'Active' | 'Warning' | 'Inactive'; + +export interface MetricOperation { + key: string; + /** Human-readable operation label (e.g. `Commit`, `Check Access`). */ + name: string; + /** Successful request count for this operation. */ + requests: number; + /** Failure count for this operation. */ + failures: number; + status: OperationStatus; +} + +export interface MetricTypeData { + type: string; + /** Total requests: the sum of the requests of the operations shown for this type. */ + totalRequests: number; + /** Operations with any activity (requests or failures), busiest first. */ + operations: MetricOperation[]; + /** False when the type has no activity at all — its dropdown option is greyed out. */ + enabled: boolean; +} + +export interface MetricsSummary { + volumes: number; + buckets: number; + keys: number; + totalCommittedBytes: number; +} + +export interface ParsedOmMetrics { + summary: MetricsSummary; + byType: Record<string, MetricTypeData>; +} + +function statusFor(requests: number, failures: number): OperationStatus { + if (failures > 0) { + return 'Warning'; + } + if (requests > 0) { + return 'Active'; + } + return 'Inactive'; +} + +/** `Num<Type><Op>` shape, used only to derive a readable label for "Other" rows. */ +const OTHER_NAME_RE = /^Num([A-Z][a-z]+)([A-Z].+)$/; + +/** Readable label for an unlisted metric: `Num<Type><Op>` → "Type Op", else the raw name. */ +function otherDisplayName(key: string): string { + const match = key.match(OTHER_NAME_RE); + return match ? `${match[1]} ${match[2]}` : key; +} + +interface OpRow { + type: string; + key: string; + name: string; + requests: number; + failures: number; +} + +/** + * Parse the `OMMetrics` bean into per-type operation data and the object-count + * summary. + * + * Operation identity comes from the explicit {@link OM_OPERATIONS} catalog (keyed by + * exact JMX name), so counts/gauges and cross-category aggregates are never mistaken + * for requests and failures are joined to their request by exact name. Any numeric + * metric the catalog does not describe (aggregate `Num<Type>Ops`, internal counters, + * or a future metric) is surfaced under the {@link OTHER_TYPE} category — labelled via + * a regex when it fits `Num<Type><Op>`, otherwise by its raw JMX name — so nothing is + * silently dropped. + */ +export function parseOmMetrics(bean: OMMetricsBean | undefined): ParsedOmMetrics { + const data = bean ?? {}; + + const numeric = (key: string): number => { + const value = data[key]; + return typeof value === 'number' ? value : 0; + }; + + const summary: MetricsSummary = { + volumes: numeric(SUMMARY_KEYS.volumes), + buckets: numeric(SUMMARY_KEYS.buckets), + keys: numeric(SUMMARY_KEYS.keys), + totalCommittedBytes: numeric(SUMMARY_KEYS.totalCommittedBytes), + }; + + // Keys the catalog/summary already account for — everything else numeric is "Other". + const referenced = new Set<string>(Object.values(SUMMARY_KEYS)); + const rows: OpRow[] = OM_OPERATIONS.map((op) => { + if (op.requestKey) { + referenced.add(op.requestKey); + } + if (op.failureKey) { + referenced.add(op.failureKey); + } + return { + type: op.type, + key: op.requestKey ?? op.failureKey ?? op.name, + name: op.name, + requests: op.requestKey ? numeric(op.requestKey) : 0, + failures: op.failureKey ? numeric(op.failureKey) : 0, + }; + }); + + for (const [key, value] of Object.entries(data)) { + if (typeof value !== 'number' || referenced.has(key)) { + continue; + } + const isFailure = key.endsWith('Fails'); Review Comment: Should `...FailsTotal` count as failures too? Right now `EcKeyCreateFailsTotal` and `EcBucketCreateFailsTotal` show up as **Active** requests. ```suggestion const isFailure = /Fails(Total)?$/.test(key); ``` ########## ozone-ui/packages/om/src/api/metricsCatalog.ts: ########## @@ -0,0 +1,436 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * Reference catalog for the `OMMetrics` JMX bean. + * + * The single source of truth mapping OM's exact JMX metric names to the operations + * the Metrics page shows. Keeping it as data (rather than inferring identity from a + * regex) means every row is cross-checked against the field declarations in + * `OMMetrics.java`, and metrics that are counts/gauges rather than RPC requests + * (e.g. `NumOpenKeysCleaned`, `NumSnapshotActive`, the aggregate `Num<Type>Ops`) are + * simply absent here and fall through to the "Other" category instead of being + * mis-reported as requests. + */ + +/** + * Ordered operation categories that seed the metric-type dropdown; a type with no + * activity is disabled there. These are the `<Type>` first-words OM uses in its + * `Num<Type><Op>` names. Any category discovered in the bean but missing here is + * still surfaced (appended after these); {@link OTHER_TYPE} is deliberately excluded + * so it sorts last. + */ +export const METRIC_TYPES = [ + 'Get', + 'Abort', + 'Add', + 'Block', + 'Bucket', + 'Cancel', + 'Commit', + 'Complete', + 'Create', + 'Delete', + 'Expired', + 'Follower', + 'Initiate', + 'Key', + 'Leader', + 'Linearizable', + 'List', + 'Lookup', + 'Open', + 'Put', + 'Recover', + 'Remove', + 'Set', + 'Snapshot', + 'Tenant', + 'Trash', + 'Volume', +] as const; + +export type MetricType = (typeof METRIC_TYPES)[number]; + +/** Catch-all category for numeric metrics not described by {@link OM_OPERATIONS}. */ +export const OTHER_TYPE = 'Other'; + +/** Object-count / data-size metrics rendered as the summary cards (not operations). */ +export const SUMMARY_KEYS = { + volumes: 'NumVolumes', + buckets: 'NumBuckets', + keys: 'NumKeys', + totalCommittedBytes: 'TotalDataCommitted', +} as const; + +/** + * One OM operation: its dropdown `type`, human-readable `name` (the "Operational + * Action" column), and the exact JMX counter names for its request and (optional) + * failure. `requestKey` is omitted for a failure-only aggregate (e.g. Trash). + */ +export interface OmOperationDef { + type: string; Review Comment: nit: Could this be `MetricType`? ```suggestion type: MetricType; ``` ########## ozone-ui/packages/om/src/__tests__/metrics.parsers.test.ts: ########## @@ -0,0 +1,263 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { describe, expect, it } from 'vitest'; +import { parseOmMetrics, OTHER_TYPE, type OMMetricsBean } from '../api/metrics'; + +// All keys use the exact JMX names OM exposes (see OMMetrics.java). +const bean: OMMetricsBean = { + name: 'Hadoop:service=OzoneManager,name=OMMetrics', + 'tag.Hostname': 'node1', + NumVolumes: 1, + NumBuckets: 2, + NumKeys: 485, + TotalDataCommitted: 62259, + // Key — cross-category aggregate (Other) plus per-op counters. + NumKeyOps: 2895, + NumKeyAllocate: 965, + NumKeyAllocateFails: 0, + NumKeyCommits: 965, + NumKeyCommitFails: 0, + NumKeyDeletes: 965, + NumKeyDeleteFails: 5, + NumKeyHSyncs: 0, + NumKeyLists: 0, + NumKeyListFails: 100, + NumGetServiceLists: 990, +}; + +describe('parseOmMetrics — summary', () => { + it('extracts the object-count summary', () => { + const { summary } = parseOmMetrics(bean); + expect(summary).toEqual({ + volumes: 1, + buckets: 2, + keys: 485, + totalCommittedBytes: 62259, + }); + }); + + it('is null-safe', () => { + expect(parseOmMetrics(undefined).summary.keys).toBe(0); + }); +}); + +describe('parseOmMetrics — Key operations', () => { + const key = parseOmMetrics(bean).byType.Key; + + it('joins a request counter to its failure counter into one row', () => { + const commit = key.operations.find((o) => o.name === 'Commit'); + const del = key.operations.find((o) => o.name === 'Delete'); + expect(commit).toMatchObject({ requests: 965, failures: 0, status: 'Active' }); + // NumKeyDeletes has 5 NumKeyDeleteFails → Warning. + expect(del).toMatchObject({ requests: 965, failures: 5, status: 'Warning' }); + }); + + it('surfaces a request with 0 count but nonzero failures as Warning', () => { + const list = key.operations.find((o) => o.name === 'List'); + expect(list).toMatchObject({ requests: 0, failures: 100, status: 'Warning' }); + }); + + it('omits operations with no activity (0 requests, 0 failures)', () => { + // NumKeyHSyncs = 0 with no failures → not shown. + expect(key.operations.find((o) => o.name === 'HSync')).toBeUndefined(); + }); + + it('sorts operations by requests descending', () => { + const requests = key.operations.map((o) => o.requests); + expect(requests).toEqual([...requests].sort((a, b) => b - a)); + }); +}); + +describe('parseOmMetrics — unlisted metrics go to the "Other" category', () => { + it('routes the cross-category aggregate NumKeyOps to Other, not Key', () => { + const { byType } = parseOmMetrics({ + NumKeyOps: 9999, // superset counter (Get/FSO/MPU); not a Key operation + NumKeyCommits: 10, + NumKeyDeletes: 5, + }); + expect(byType.Key.operations.find((o) => o.name === 'Key Ops')).toBeUndefined(); + expect(byType.Key.totalRequests).toBe(15); + const other = byType[OTHER_TYPE]; + expect(other?.operations.find((o) => o.name === 'Key Ops')).toMatchObject({ requests: 9999 }); + }); + + it('routes an internal count (NumOpenKeysCleaned) to Other, not Open (chihsuan)', () => { Review Comment: nit: Could we drop `(chihsuan)` from the test title? 🙂 ```suggestion it('routes an internal count (NumOpenKeysCleaned) to Other, not Open', () => { ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
