Fixes #30895: Complete Metrics hierarchy and details - #30896
Conversation
github-actions
Bot
added
backend
safe to test
|
Too many files changed for review. ( Bypass the limit by tagging |
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
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 |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| repository.get(null, id, repository.getFields("id")); | ||
| Set<UUID> visibleAssets = new HashSet<>(); | ||
| for (MetricAssetDirection linked : repository.getAssetsWithDirection(id)) { | ||
| if (canViewAsset(securityContext, linked.getAsset())) { | ||
| visibleAssets.add(linked.getAsset().getId()); | ||
| } | ||
| } | ||
| return repository.getObservability(id, visibleAssets); |
There was a problem hiding this comment.
💡 Performance: Observability endpoint fetches linked assets twice
getObservability() calls repository.getAssetsWithDirection(id) in the resource to build the visibleAssets set (and runs a per-asset authorize() for each linked asset), then calls repository.getObservability(id, visibleAssets), whose compute() invokes metricRepository.getAssetsWithDirection(metricId) a second time. Each getAssetsWithDirection walks lineage for every linked asset, so it runs the full asset+lineage resolution twice per request. Consider passing the already-resolved linkedAssets into the builder (e.g. an overload of build/compute that accepts the pre-fetched List) so the lineage scan runs once.
Was this helpful? React with 👍 / 👎
| int visibleMetricCount(UUID groupId, Predicate<EntityReference> isVisible) { | ||
| CollectionDAO.MetricGroupDAO groupDAO = (CollectionDAO.MetricGroupDAO) dao; | ||
| return scanMemberIds(groupDAO, groupId, 0, 0, null, "%", false, isVisible).total(); | ||
| } | ||
|
|
||
| private MemberScan scanMemberIds( | ||
| CollectionDAO.MetricGroupDAO groupDAO, | ||
| UUID groupId, | ||
| int limit, | ||
| int offset, | ||
| String query, | ||
| String nameLike, | ||
| boolean rootOnly, | ||
| Predicate<EntityReference> isVisible) { | ||
| List<UUID> page = new ArrayList<>(); |
There was a problem hiding this comment.
💡 Performance: Metric group member listing scans all members per page/count
scanMemberIds() always walks every member of a group in MEMBER_SCAN_BATCH_SIZE (200) batches — issuing loadMetricReferences() (and, for rootOnly, subtreeMatchesQuery()->expandSubtree()) on each batch — because it must compute the visible total and skip to the offset with permission filtering applied in Java. visibleMetricCount() does the same full scan just to count. For groups with many members this is O(n) DB round trips on every /metricGroups/{id}/metrics request and every count, regardless of the requested page size. This is inherent to permission-aware pagination, but for large groups it can become a hot-path scalability issue; consider caching the visible count or bounding the scan.
Was this helpful? React with 👍 / 👎
✅ TypeScript Types Auto-UpdatedThe generated TypeScript types have been automatically updated based on JSON schema changes in this PR. |
Code Review 👍 Approved with suggestions 0 resolved / 2 findingsCompletes the metrics hierarchy and detail experience with new schema-first groups, search indexes, and UI views. Consider optimizing the observability endpoint asset fetching and metric group member pagination scans to avoid redundant queries. 💡 Performance: Observability endpoint fetches linked assets twice📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/metrics/MetricResource.java:1157-1164 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/MetricObservabilityBuilder.java:154-168 getObservability() calls repository.getAssetsWithDirection(id) in the resource to build the visibleAssets set (and runs a per-asset authorize() for each linked asset), then calls repository.getObservability(id, visibleAssets), whose compute() invokes metricRepository.getAssetsWithDirection(metricId) a second time. Each getAssetsWithDirection walks lineage for every linked asset, so it runs the full asset+lineage resolution twice per request. Consider passing the already-resolved linkedAssets into the builder (e.g. an overload of build/compute that accepts the pre-fetched List) so the lineage scan runs once. 💡 Performance: Metric group member listing scans all members per page/count📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/MetricGroupRepository.java:388-402 scanMemberIds() always walks every member of a group in MEMBER_SCAN_BATCH_SIZE (200) batches — issuing loadMetricReferences() (and, for rootOnly, subtreeMatchesQuery()->expandSubtree()) on each batch — because it must compute the visible total and skip to the offset with permission filtering applied in Java. visibleMetricCount() does the same full scan just to count. For groups with many members this is O(n) DB round trips on every /metricGroups/{id}/metrics request and every count, regardless of the requested page size. This is inherent to permission-aware pagination, but for large groups it can become a hot-path scalability issue; consider caching the visible count or bounding the scan. 🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source |
❌ UI Checkstyle Failed❌ Antd + Less Deprecation GuardA new Affected filesopenmetadata-ui/src/main/resources/ui/src/components/common/Table/TableV2.test.tsx: import type { ColumnsType } from 'antd/es/table/interface' 🔍 ESLint findings in this PR's files — ESLint report could not be read — see the job log.Errors block the build. Warnings do not yet — they are rules whose backlog is still No machine-readable report was produced. Fix locally (fast - only checks files changed in this branch): make ui-checkstyle-changed |
Describe your changes:
Fixes #30895
I completed the Metrics hierarchy and full detail experience because the existing flat view did not scale to grouped metrics or expose their governance, assets, observability, activity, and approval context.
Type of change:
High-level design:
Tests:
Use cases covered
Unit tests
Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
UI screen recording / screenshots:
Additional desktop, narrow, dark-mode, Assets, Observability, Activity, and Approval baselines are committed beside these screenshots.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.