Skip to content

fix(ui): extract PROFILER_METRIC options behind ProfilerMetricsClassBase - #31934

Merged
shah-harshit merged 3 commits into
mainfrom
fix/profiler-metrics-picker-extension-seam
Aug 24, 2026
Merged

fix(ui): extract PROFILER_METRIC options behind ProfilerMetricsClassBase#31934
shah-harshit merged 3 commits into
mainfrom
fix/profiler-metrics-picker-extension-seam

Conversation

@shah-harshit

@shah-harshit shah-harshit commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Fixes open-metadata/openmetadata-collate#6076
Screenshot 2026-08-24 at 2 58 11 PM

Summary

  • The profiler settings modal's per-column metrics picker (ProfilerSettingsModal.tsx) read the PROFILER_METRIC constant directly. Downstream distributions that ship additional profiler metrics (e.g. Collate) have no way to add them to this picker without forking the modal, because a plain exported array has no override seam — unlike every other extensible piece of UI in this codebase, which goes through an XClassBase singleton.
  • Adds ProfilerMetricsClassBase (utils/ProfilerMetricsClassBase.ts), following the exact same shape as the existing TagClassBase/ProfilerConfigurationClassBase/etc. utilities: a class with a getProfilerMetricOptions() method, exported as a singleton default export.
  • ProfilerSettingsModal.tsx now calls profilerMetricsClassBase.getProfilerMetricOptions() instead of importing PROFILER_METRIC directly. No behavior change for OSS — it returns the same array.
  • Updated ProfilerSettingsModal.test.tsx's mocks accordingly.

Test plan

  • yarn test ProfilerSettingsModal — existing tests should pass unmodified in behavior
  • yarn tsc:check / yarn lint:fix on the 3 changed files
  • Manual: open a table's Profiler Settings → confirm the metrics picker still shows the same OSS metric list as before

Greptile Summary

The PR routes the profiler settings modal’s metric list through an extensible ProfilerMetricsClassBase singleton while preserving the existing OSS options.

  • Adds the class-based profiler metric provider.
  • Updates the modal to obtain metrics from the provider.
  • Updates the modal test mock for the new module boundary.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
openmetadata-ui/src/main/resources/ui/src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx Replaces the direct metric constant reference with the new provider while preserving the existing picker mapping.
openmetadata-ui/src/main/resources/ui/src/utils/ProfilerMetricsClassBase.ts Adds an extensible class and default singleton that return the existing profiler metric list.
openmetadata-ui/src/main/resources/ui/src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.test.tsx Moves the test fixture for profiler metrics to a default-export-compatible provider mock.

Reviews (3): Last reviewed commit: "fix(test): mark ProfilerMetricsClassBase..." | Re-trigger Greptile

The profiler settings modal's per-column metrics picker read PROFILER_METRIC
directly, giving downstream distributions that add their own profiler
metrics no seam to extend the picker's option list short of forking the
modal. Extract the lookup behind ProfilerMetricsClassBase (same pattern as
the existing *ClassBase utilities), so it can be overridden the same way.

No behavior change for OSS: getProfilerMetricOptions() returns the same
PROFILER_METRIC array as before.
@github-actions

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

  • No GitHub issue is linked. Link an issue in the Development section of the PR (or add Fixes #12345 to the description). For a same-org cross-repo issue, add Fixes open-metadata/<repo>#123 to the description.

The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically.

Maintainers can bypass this check by adding the skip-pr-checks label.

@github-actions

Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

✅ Playwright Results — workflow succeeded

Validated commit 9d37f3f410548e82cc9f6aead8a3051c5c497e7b in Playwright run 32715753270, attempt 1.

✅ 790 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 51m 41s

⏱️ Max setup 4m 10s · max shard execution 18m 18s · max shard-job elapsed before upload 21m 21s · reporting 4s

🌐 214.79 requests/attempt · 2.60 app boots/UI scenario · 12.66% common-shard skew

Optimization targets still in progress:

  • Browser traffic was 214.79 requests per attempt (convergence target: fewer than 200).
  • Application boot ratio was 2.6 per UI scenario (2179 boots / 838 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard chromium-01 145 0 0 0 0 0
✅ Shard chromium-02 136 0 0 0 0 0
✅ Shard chromium-03 150 0 0 0 0 0
✅ Shard chromium-04 145 0 0 0 0 0
✅ Shard data-asset-rules-01 61 0 0 0 0 0
✅ Shard domain-isolation-01 16 0 0 0 0 0
✅ Shard global-state-01 34 0 0 0 0 0
✅ Shard import-export-01 26 0 0 0 0 0
✅ Shard ingestion-01 32 0 0 0 0 0
✅ Shard reindex-01 5 0 0 0 0 0
✅ Shard search-01 11 0 0 0 0 0
✅ Shard search-rbac-01 29 0 0 0 0 0

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ UI Checkstyle passed — lint findings in changed files

🔍 ESLint findings in this PR's files — 0 error(s), 21 warning(s)

Errors block the build. Warnings do not yet — they are rules whose backlog is still
being worked down, listed so this PR does not add to it. See docs/ui-code-quality-gate.md.

0 error(s), 21 warning(s) across 1 changed file(s).

Count Rule
9 react-hooks/exhaustive-deps
8 sonarjs/no-duplicate-string
2 sonarjs/cyclomatic-complexity
1 sonarjs/no-nested-conditional
1 jsx-a11y/no-autofocus
All findings
Location Rule Message
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:137:6 react-hooks/exhaustive-deps React Hook useMemo has a missing dependency: 't'. Either include it or remove the dependency array.
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:154:6 react-hooks/exhaustive-deps React Hook useMemo has a missing dependency: 't'. Either include it or remove the dependency array.
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:181:5 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 12 which is greater than 10 authorized.","cost":2,"secondaryLocations":[{"line":181,"column":4,"endLine":181,"endColumn
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:288:32 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 11 which is greater than 10 authorized.","cost":1,"secondaryLocations":[{"line":288,"column":31,"endLine":288,"endColum
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:306:11 sonarjs/no-nested-conditional Extract this nested ternary operation into an independent statement.
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:346:25 sonarjs/no-duplicate-string Define a constant instead of duplicating this literal 3 times.
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:366:5 react-hooks/exhaustive-deps React Hook useCallback has missing dependencies: 'onVisibilityChange', 'partitionIntervalType', 't', and 'tableId'. Either include them or remove the dependency
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:373:6 react-hooks/exhaustive-deps React Hook useCallback has missing dependencies: 'onVisibilityChange' and 'updateInitialConfig'. Either include them or remove the dependency array. If 'onVisib
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:380:5 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'handleStateChange'. Either include it or remove the dependency array.
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:388:5 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'handleStateChange'. Either include it or remove the dependency array.
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:395:6 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'handleStateChange'. Either include it or remove the dependency array.
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:430:5 react-hooks/exhaustive-deps React Hook useCallback has missing dependencies: 'form' and 'handleStateChange'. Either include them or remove the dependency array.
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:450:6 react-hooks/exhaustive-deps React Hook useEffect has a missing dependency: 'fetchProfileConfig'. Either include it or remove the dependency array.
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:488:24 sonarjs/no-duplicate-string Define a constant instead of duplicating this literal 5 times.
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:494:17 jsx-a11y/no-autofocus The autoFocus prop should not be used, as it can reduce usability and accessibility for users.
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:511:27 sonarjs/no-duplicate-string Define a constant instead of duplicating this literal 5 times.
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:700:34 sonarjs/no-duplicate-string Define a constant instead of duplicating this literal 7 times.
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:816:30 sonarjs/no-duplicate-string Define a constant instead of duplicating this literal 3 times.
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:817:39 sonarjs/no-duplicate-string Define a constant instead of duplicating this literal 6 times.
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:837:40 sonarjs/no-duplicate-string Define a constant instead of duplicating this literal 3 times.
🟡 src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:851:30 sonarjs/no-duplicate-string Define a constant instead of duplicating this literal 3 times.

Fix locally (fast - only checks files changed in this branch):

make ui-checkstyle-changed

Without __esModule: true, Babel's CJS/ESM interop double-wraps the mock's
default export, so profilerMetricsClassBase.getProfilerMetricOptions()
resolves to undefined at runtime. Passed locally (different jest/babel
config) but failed in CI: "TypeError:
ProfilerMetricsClassBase_1.default.getProfilerMetricOptions is not a
function".
@gitar-bot

gitar-bot Bot commented Aug 24, 2026

Copy link
Copy Markdown
Code Review ✅ Approved

Extracts PROFILER_METRIC options behind the new ProfilerMetricsClassBase singleton to allow downstream metric extensions in the profiler settings modal. No issues found.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

@github-actions

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 67%
67.06% (80448/119959) 51.47% (49251/95678) 52.46% (14707/28033)

@sonarqubecloud

Copy link
Copy Markdown

@shah-harshit
shah-harshit added this pull request to the merge queue Aug 24, 2026
Merged via the queue into main with commit 2e33632 Aug 24, 2026
92 of 94 checks passed
@shah-harshit
shah-harshit deleted the fix/profiler-metrics-picker-extension-seam branch August 24, 2026 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants