Use storageCapabilities config as authoritative signal for SPM availability#3587
Use storageCapabilities config as authoritative signal for SPM availability#3587yurishkuro merged 8 commits intojaegertracing:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates Jaeger UI Monitor configuration merging and gating so the Monitor menu is controlled by config while the Monitor page behavior depends on metrics-storage capability.
Changes:
- Add
monitorto the list of config fields that should be deep-merged with user config. - Gate metrics fetching and the Monitor empty state on
storageCapabilities.metricsStorage. - Change TopNav visibility logic for the Monitor link to use
monitor.menuEnabled(not storage capability).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/jaeger-ui/src/constants/default-config.ts | Ensures monitor config merges with user config instead of being overwritten. |
| packages/jaeger-ui/src/components/Monitor/ServicesView/index.tsx | Prevents metrics fetch/render when metrics storage capability is disabled. |
| packages/jaeger-ui/src/components/App/TopNav.tsx | Keeps Monitor nav item controlled by monitor.menuEnabled instead of storage capability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
packages/jaeger-ui/src/components/Monitor/ServicesView/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
packages/jaeger-ui/src/utils/config/get-config.ts:28
getJaegerStorageCapabilities()may return a partial object (or in tests it’s stubbed to{}), butgetCapabilities()currently returns it verbatim as long as it’s non-nullish. That can drop default keys likemetricsStorage/archiveStorageand causegetConfigValue('storageCapabilities.*')to beundefined. Consider normalizing by merging into defaults (and guarding for non-object returns), e.g. return{ ...defaultConfig.storageCapabilities, ...(capabilities ?? {}) }so missing flags default tofalse.
function getCapabilities() {
const getter = window.getJaegerStorageCapabilities;
const capabilities = typeof getter === 'function' ? getter() : null;
return capabilities ?? defaultConfig.storageCapabilities;
}
packages/jaeger-ui/src/components/App/TopNav.tsx:76
- The Monitor nav link gating changed from
storageCapabilities.metricsStoragetomonitor.menuEnabled, but the existingTopNav.test.jsmock still returnstrueforstorageCapabilities.metricsStorage(and not formonitor.menuEnabled). This will cause the test that asserts the Monitor link renders to fail unless the mock is updated to includemonitor.menuEnabled(or the test is adjusted to match the new condition).
// We keep the menu item for Monitor in the top nav visible if config says so.
// If the storage capability indicates that metrics storage is not supported,
// the page will show a landing page with instructions on how to set up SPM.
if (getConfigValue('monitor.menuEnabled')) {
NAV_LINKS.push({
to: monitorATMUrl.getUrl(),
matches: monitorATMUrl.matches,
text: 'Monitor',
});
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
packages/jaeger-ui/src/components/Monitor/ServicesView/index.test.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
packages/jaeger-ui/src/components/Monitor/ServicesView/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
packages/jaeger-ui/src/utils/config/get-config.ts:28
getCapabilities()returns the injected object as-is, so a partial injection (e.g. devjaeger-ui.config.jsonwith{ "metricsStorage": true }) will drop default keys likearchiveStorageand can yieldundefinedvalues. To keep defaults stable, consider merging withdefaultConfig.storageCapabilitieswhen the getter returns an object (e.g. default + injected), rather than replacing the whole object.
// getCapabilities reads storage capabilities injected by the query-service backend
// via window.getJaegerStorageCapabilities (search-replaced into index.html).
// In development, the Vite plugin replicates this injection from jaeger-ui.config.json.
// Falls back to the default config when the function is not present.
function getCapabilities() {
const getter = window.getJaegerStorageCapabilities;
const capabilities = typeof getter === 'function' ? getter() : null;
return capabilities ?? defaultConfig.storageCapabilities;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
packages/jaeger-ui/src/components/Monitor/ServicesView/index.test.js
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/Monitor/ServicesView/index.test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3587 +/- ##
==========================================
- Coverage 89.15% 89.14% -0.02%
==========================================
Files 304 304
Lines 9719 9716 -3
Branches 2586 2588 +2
==========================================
- Hits 8665 8661 -4
- Misses 1051 1052 +1
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Problem
The Service Performance Monitoring (SPM) Monitor page had several interconnected issues:
Page crash on load —
MonitorATMEmptyStatecrashed withTypeError: Cannot read properties of undefined (reading 'mainTitle')because the embedded config provided amonitorkey that completely overwrotedefaultConfig.monitor(includingemptyState), due to a shallow merge.Unnecessary API calls — When metrics storage is not configured, the Monitor page fired API requests on load and only showed the empty state after receiving a
501 Not Implementedresponse. The intent was always to usestorageCapabilities.metricsStoragefrom config as the authoritative signal, not a runtime API probe.storageCapabilitiesinjaeger-ui.config.jsonwas silently ignored — The Vite dev-server plugin injected the JSON config intogetJaegerUiConfig()but never intogetJaegerStorageCapabilities(). Sinceindex.htmlalways definesgetJaegerStorageCapabilities(hardcoded to returnmetricsStorage: false), the config file value was always overwritten.Changes
default-config.ts— addmonitortomergeFieldsmergeFieldscontrols which top-level config keys are shallow-merged (rather than wholesale replaced) when the embedded config is combined with defaults.monitorwas missing, so a backend-providedmonitor: { menuEnabled: true }silently droppedemptyState, causing the crash.vite.config.mts— injectstorageCapabilitiesintogetJaegerStorageCapabilities()The Go query-service injects
storageCapabilitiesintoindex.htmlvia a separate search-replace onJAEGER_STORAGE_CAPABILITIES. The Vite plugin was only replicating theJAEGER_CONFIGinjection. It now also replacesJAEGER_STORAGE_CAPABILITIESwhenstorageCapabilitiesis present injaeger-ui.config.json, making local dev fully equivalent to production.get-config.ts— add comments clarifying the two-source designstorageCapabilitiesis intentionally loaded from a separate source (getJaegerStorageCapabilities) and always overrides anything in the main UI config, keeping the backend authoritative. This was not documented; comments now explain the three-priority merge model.Monitor/ServicesView/index.tsx— gate on config, not runtime API responsestorageCapabilities.metricsStoragefrom config at render time.<MonitorATMEmptyState />immediately when it isfalse, with no API calls made.isATMActivated !== falseguard fromfetchMetrics(it was checking a Redux flag that was only set after a 501 response).reducers/metrics.ts/types/metrics.ts— removeisATMActivatedisATMActivatedwas a runtime approximation ofstorageCapabilities.metricsStorage: it started asnull, then flipped tofalseupon a501response. Now that the config is the authoritative source (known at page load), this redundant state and the 501-detection logic have been removed entirely.Tests
get-config.test.js— newstorageCapabilitiesdescribe block covering: backend injection, fallback to defaults, and precedence ofgetJaegerStorageCapabilitiesover the UI config.reducers/metrics.test.js— removedisATMActivatedassertions and the dedicated "501 Not Implemented" test case.Monitor/ServicesView/index.test.js— updatedgetConfigValuemock to returntrueforstorageCapabilities.metricsStorage; empty-state test now toggles the config mock instead of settingisATMActivated: false.Monitor/index.test.js— addedgetConfigValuemock; updated empty-state test accordingly.How the config loading works (for reference)
AI Usage in this PR (choose one)
See AI Usage Policy.