feat(ui): fetch storage capabilities from backend during Vite dev mode#3544
feat(ui): fetch storage capabilities from backend during Vite dev mode#3544aryunewaskar77-art wants to merge 18 commits intojaegertracing:mainfrom
Conversation
Signed-off-by: aryunewaskar77-art <aryu.newaskar77@gmail.com>
|
Hi @aryunewaskar77-art, thanks for your contribution! To ensure quality reviews, we limit how many concurrent open PRs new contributors can open. This PR is currently on hold (Status: 3/1 open). We will automatically move this into the review queue once your existing PRs are merged or closed. Please see our Contributing Guidelines for details on our tiered quota policy. |
Signed-off-by: aryunewaskar77-art <aryu.newaskar77@gmail.com>
| } catch (err) { | ||
| // Silent fallback for production behavior, but log for dev visibility | ||
| if (err instanceof Error && err.name === 'AbortError') { | ||
| console.log('[jaeger-ui-config] Backend fetch timed out (1s)'); | ||
| } else { | ||
| console.log('[jaeger-ui-config] Backend not running or error fetching config'); | ||
| } |
There was a problem hiding this comment.
The AbortError check will never execute because the fetch is already wrapped in .catch(() => null) on line 63. When the timeout triggers controller.abort(), the fetch's catch handler converts it to null, preventing the outer catch block from seeing the AbortError.
The code will incorrectly log "Backend not running or error fetching config" instead of "Backend fetch timed out (1s)" when a timeout occurs.
Fix:
try {
const fetchOptions = { signal: controller.signal };
const response = await fetch('http://127.0.0.1:16686/api/ui/config', fetchOptions);
if (response?.ok) {
const data = await response.json();
// ... rest of the logic
}
} catch (err) {
if (err instanceof Error && err.name === 'AbortError') {
console.log('[jaeger-ui-config] Backend fetch timed out (1s)');
} else {
console.log('[jaeger-ui-config] Backend not running or error fetching config');
}
}Remove the .catch(() => null) to allow proper error propagation.
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Pull request overview
This PR enhances the Jaeger UI development experience by automatically fetching backend configuration during Vite dev mode. Previously, developers had to manually configure local files to test features like the Monitor tab or Archive storage. The changes enable the UI to automatically sync with the running backend's capabilities, making development more seamless.
Changes:
- Added automatic fetching of storageCapabilities, version, and uiConfig from the backend's
/api/ui/configendpoint during dev mode - Implemented smart config merging with priority: JS config (full override) > JSON config (shallow merge) > backend defaults
- Added resilient error handling with 1-second fetch timeout and graceful fallback when backend is unavailable
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| verify_otel.cjs | New OpenTelemetry verification script for testing trace submission to Jaeger |
| packages/jaeger-ui/vite.config.mts | Enhanced Vite plugin to fetch and merge backend config with local overrides, added TypeScript annotations |
| package.json | Added @types/node dependency for Node.js type definitions |
| package-lock.json | Updated lockfile with dependency changes and removed peer dependency flags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Minimal OpenTelemetry Verification Script | ||
| * This script sends a single trace to Jaeger at http://localhost:4318/v1/traces | ||
| */ | ||
|
|
||
| const http = require('http'); | ||
|
|
||
| const traceData = { | ||
| resourceSpans: [ | ||
| { | ||
| resource: { | ||
| attributes: [ | ||
| { | ||
| key: 'service.name', | ||
| value: { stringValue: 'verification-service' } | ||
| } | ||
| ] | ||
| }, | ||
| scopeSpans: [ | ||
| { | ||
| spans: [ | ||
| { | ||
| traceId: '1234567890abcdef1234567890abcdef', | ||
| spanId: '1234567890abcdef', | ||
| name: 'verification-span', | ||
| startTimeUnixNano: (Date.now() * 1000000).toString(), | ||
| endTimeUnixNano: ((Date.now() + 100) * 1000000).toString(), | ||
| kind: 1, | ||
| status: { code: 1 } | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| }; | ||
|
|
||
| const options = { | ||
| hostname: '127.0.0.1', | ||
| port: 4318, | ||
| path: '/v1/traces', | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json' | ||
| } | ||
| }; | ||
|
|
||
| const req = http.request(options, (res) => { | ||
| console.log(`Status Code: ${res.statusCode}`); | ||
| res.on('data', (d) => { | ||
| process.stdout.write(d); | ||
| }); | ||
| if (res.statusCode === 200) { | ||
| console.log('\nSuccess! Trace sent to Jaeger.'); | ||
| console.log('Now check your Jaeger UI (http://localhost:16686) for "verification-service".'); | ||
| } | ||
| }); | ||
|
|
||
| req.on('error', (error) => { | ||
| console.error('Error connecting to Jaeger:', error.message); | ||
| console.log('Make sure Jaeger is running (docker ps) and port 4318 is exposed.'); | ||
| }); | ||
|
|
||
| req.write(JSON.stringify(traceData)); | ||
| req.end(); |
There was a problem hiding this comment.
The verify_otel.cjs file appears unrelated to the PR description, which focuses on fetching storage capabilities from the backend during Vite dev mode. This file is a standalone OpenTelemetry verification script. Please clarify whether this file is intended to be part of this PR or if it was accidentally included.
| const response = await fetch('http://127.0.0.1:16686/api/ui/config', fetchOptions).catch( | ||
| () => null | ||
| ); |
There was a problem hiding this comment.
The fetch call has a .catch(() => null) that swallows errors, but then the code checks response?.ok which could mask certain failure scenarios. If the fetch throws an error (not AbortError), it will be caught by the outer try-catch, but if it returns null from the inner catch, the outer catch won't execute. This creates inconsistent error handling. Consider removing the inner .catch() and letting all errors bubble to the outer try-catch block for consistent handling.
| const response = await fetch('http://127.0.0.1:16686/api/ui/config', fetchOptions).catch( | |
| () => null | |
| ); | |
| const response = await fetch('http://127.0.0.1:16686/api/ui/config', fetchOptions); |
| if (fs.existsSync(jsConfigPath)) { | ||
| try { | ||
| const jsContent = fs.readFileSync(jsConfigPath, 'utf-8'); | ||
| // Replace the JAEGER_CONFIG_JS comment with UIConfig function | ||
| // This mimics the Go server behavior for .js config files | ||
| const uiConfigFn = `function UIConfig() { ${jsContent} }`; | ||
| html = html.replace('// JAEGER_CONFIG_JS', uiConfigFn); | ||
| console.log('[jaeger-ui-config] Loaded config from jaeger-ui.config.js'); | ||
| return html; | ||
| console.log( | ||
| '[jaeger-ui-config] Loaded config from jaeger-ui.config.js (full override, no merge)' | ||
| ); | ||
| // Note: Don't return early here, we still need to inject storageCapabilities and version below | ||
| } catch (err) { | ||
| console.error('[jaeger-ui-config] Error loading jaeger-ui.config.js:', err); | ||
| } |
There was a problem hiding this comment.
When a jaeger-ui.config.js file exists, the code doesn't inject the finalUiConfig into the HTML because it's inside the else block (lines 101-135). This means that even if the backend provides uiConfig, it won't be injected when using the JS config file. The storageCapabilities and version will be injected (lines 137-151), but JAEGER_CONFIG won't be set, potentially breaking the UI. The logic should inject JAEGER_CONFIG regardless of which config path is taken, or document why this is intentional.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Aaryaa Newaskar <aryu.newaskar77@gmail.com>
Signed-off-by: aryunewaskar77-art <aryu.newaskar77@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Extract storageCapabilities from JSON if present | ||
| if (parsedJsonConfig.storageCapabilities) { | ||
| storageCapabilities = { ...storageCapabilities, ...parsedJsonConfig.storageCapabilities }; | ||
| delete parsedJsonConfig.storageCapabilities; | ||
| } | ||
|
|
||
| // Shallow merge: JSON on top of backend base |
There was a problem hiding this comment.
The PR description says storageCapabilities and version come only from the backend with no local overrides, but this code allows jaeger-ui.config.json to override storageCapabilities. This is a behavior/contract mismatch; either remove this override path or update the PR description and any related docs to reflect it.
| transformIndexHtml: { | ||
| order: 'pre' as const, | ||
| async handler(html: string) { | ||
| // Check for JS config first (higher priority, like in Go server) | ||
| let backendUiConfig: any = null; | ||
| let storageCapabilities: any = null; | ||
| let version: any = null; | ||
|
|
||
| const controller = new AbortController(); | ||
| const timeout = setTimeout(() => controller.abort(), 1000); | ||
|
|
||
| try { | ||
| const fetchOptions = { signal: controller.signal }; | ||
| const response = await fetch('http://127.0.0.1:16686/api/ui/config', fetchOptions); | ||
|
|
There was a problem hiding this comment.
This plugin is included unconditionally in the Vite config, and transformIndexHtml also runs during vite build. With the new backend fetch, production builds can become non-deterministic (if a local backend is running) and will always incur the 1s timeout on failures. Consider gating the backend fetch logic to command === 'serve' / dev mode so builds remain reproducible and fast.
| let backendUiConfig: any = null; | ||
| let storageCapabilities: any = null; | ||
| let version: any = null; | ||
|
|
||
| const controller = new AbortController(); | ||
| const timeout = setTimeout(() => controller.abort(), 1000); | ||
|
|
||
| try { | ||
| const fetchOptions = { signal: controller.signal }; | ||
| const response = await fetch('http://127.0.0.1:16686/api/ui/config', fetchOptions); | ||
|
|
||
| if (response?.ok) { | ||
| const data = await response.json(); | ||
|
|
||
| // Extract data from unified response | ||
| storageCapabilities = data.storageCapabilities; | ||
| version = data.version; | ||
| backendUiConfig = data.uiConfig; | ||
|
|
||
| console.log('[jaeger-ui-config] Fetched config from backend /api/ui/config'); | ||
| } | ||
| } catch (err) { | ||
| // Silent fallback for production behavior, but log for dev visibility |
There was a problem hiding this comment.
transformIndexHtml will run on every page load in dev, and this implementation fetches /api/ui/config each time (plus logs on success). This can add noticeable overhead and noisy logs during development. Consider caching the backend response for a short TTL (e.g., 5–30s) and/or only logging when the source changes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Aaryaa Newaskar <aryu.newaskar77@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Aaryaa Newaskar <aryu.newaskar77@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Aaryaa Newaskar <aryu.newaskar77@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
verify_otel.cjs:65
- This file appears to be unrelated to the PR's stated purpose of fetching storage capabilities from the backend during Vite dev mode. The PR description and linked issue make no mention of OpenTelemetry verification functionality. This script seems to have been accidentally included in the commit. Consider removing this file from the PR unless it's intentionally part of a broader set of changes not mentioned in the description.
/**
* This file previously contained a standalone OpenTelemetry verification script.
* It has been intentionally left empty to avoid keeping unreferenced tooling
* at the repository root.
*/
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let backendUiConfig: any = null; | ||
| let storageCapabilities: any = null; | ||
| let version: any = null; |
There was a problem hiding this comment.
The variables 'backendUiConfig', 'storageCapabilities', and 'version' are all typed as 'any', which bypasses TypeScript's type safety. Since '@types/node' was added as a dependency in this PR, consider defining proper TypeScript interfaces for the expected backend response structure. This would make the code more maintainable and catch potential type errors at compile time.
| const fetchOptions = { signal: controller.signal }; | ||
| const response = await fetch('http://127.0.0.1:16686/api/ui/config', fetchOptions); | ||
|
|
||
| if (response?.ok) { |
There was a problem hiding this comment.
The optional chaining operator 'response?.ok' is unnecessary here. If the fetch call succeeds without throwing, the response object is guaranteed to be defined. The check can be simplified to 'response.ok'. The optional chaining doesn't add safety since any network or timeout errors are already caught in the catch block.
| if (response?.ok) { | |
| if (response.ok) { |
| if (finalUiConfig) { | ||
| html = html.replace( | ||
| 'const JAEGER_CONFIG = DEFAULT_CONFIG;', | ||
| `const JAEGER_CONFIG = ${JSON.stringify(parsedConfig)};` | ||
| `const JAEGER_CONFIG = ${JSON.stringify(finalUiConfig)};` | ||
| ); | ||
| console.log('[jaeger-ui-config] Loaded config from jaeger-ui.config.json'); | ||
| return html; | ||
| } catch (err) { | ||
| console.error('[jaeger-ui-config] Error loading jaeger-ui.config.json:', err); | ||
| } | ||
| } | ||
|
|
||
| // Inject storageCapabilities into index.html | ||
| if (storageCapabilities) { | ||
| html = html.replace( | ||
| 'const JAEGER_STORAGE_CAPABILITIES = DEFAULT_STORAGE_CAPABILITIES;', | ||
| `const JAEGER_STORAGE_CAPABILITIES = ${JSON.stringify(storageCapabilities)};` | ||
| ); | ||
| } | ||
|
|
||
| // Inject version into index.html | ||
| if (version) { | ||
| html = html.replace( | ||
| 'const JAEGER_VERSION = DEFAULT_VERSION;', | ||
| `const JAEGER_VERSION = ${JSON.stringify(version)};` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Multiple JSON.stringify calls (lines 131, 140, 148) are used on data fetched from the backend without error handling. If the backend returns objects with circular references or other non-serializable content, these calls will throw and crash the dev server during HTML transformation. Consider wrapping these operations in try-catch blocks or at minimum documenting the assumption that backend data is always JSON-serializable.
|
|
||
| const proxyConfig = { | ||
| target: 'http://localhost:16686', | ||
| target: 'http://127.0.0.1:16686', |
There was a problem hiding this comment.
The change from 'localhost' to '127.0.0.1' improves consistency with the fetch URL used later in the code (line 63), but breaks consistency with existing documentation. The CONTRIBUTING.md and AGENTS.md files both reference 'http://localhost:16686' throughout. While both localhost and 127.0.0.1 generally work the same, using localhost is more standard and user-friendly in documentation. Consider either reverting this change or updating the documentation files to match.
| target: 'http://127.0.0.1:16686', | |
| target: 'http://localhost:16686', |
| try { | ||
| const fetchOptions = { signal: controller.signal }; | ||
| const response = await fetch('http://127.0.0.1:16686/api/ui/config', fetchOptions); | ||
|
|
||
| if (response?.ok) { | ||
| const data = await response.json(); | ||
|
|
||
| // Extract data from unified response | ||
| storageCapabilities = data.storageCapabilities; | ||
| version = data.version; | ||
| backendUiConfig = data.uiConfig; | ||
|
|
||
| console.log('[jaeger-ui-config] Fetched config from backend /api/ui/config'); |
There was a problem hiding this comment.
This code assumes the backend endpoint '/api/ui/config' exists and returns a JSON response with 'storageCapabilities', 'version', and 'uiConfig' fields. However, this is a frontend-only repository and the backend implementation is not visible in this PR. The PR description mentions this endpoint as a "prerequisite", but there's no verification that it exists or that it returns data in the expected format. If the endpoint doesn't exist yet or returns a different structure, the fetch will fail silently and the UI will fall back to defaults. Consider adding a comment documenting the expected response structure, or better yet, creating a TypeScript interface for the expected response.
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com> Signed-off-by: Aaryaa Newaskar <aryu.newaskar77@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (err) { | ||
| console.error('[jaeger-ui-config] Error loading jaeger-ui.config.json:', err); |
There was a problem hiding this comment.
The error object 'err' is declared but never used in this catch block. Similar to the issue above, the error should either be logged for debugging or the parameter should be removed. This is especially important here since JSON parsing errors would be helpful to see during development.
| if (fs.existsSync(jsConfigPath)) { | ||
| try { | ||
| const jsContent = fs.readFileSync(jsConfigPath, 'utf-8'); | ||
| // Replace the JAEGER_CONFIG_JS comment with UIConfig function | ||
| // This mimics the Go server behavior for .js config files | ||
| const uiConfigFn = `function UIConfig() { ${jsContent} }`; | ||
| html = html.replace('// JAEGER_CONFIG_JS', uiConfigFn); | ||
| console.log('[jaeger-ui-config] Loaded config from jaeger-ui.config.js'); | ||
| return html; | ||
| console.log( | ||
| '[jaeger-ui-config] Loaded config from jaeger-ui.config.js (full override, no merge)' | ||
| ); | ||
| // Note: Don't return early here, we still need to inject storageCapabilities and version below |
There was a problem hiding this comment.
When a JS config file exists, the code still attempts to inject storageCapabilities and version from the backend, but it doesn't inject the backend's uiConfig. This creates an inconsistency: with JS config, developers lose access to the backend's uiConfig as a baseline, which contradicts one of the key benefits mentioned in the PR description ("automatic backend sync").
Consider whether JS config should also have access to backend uiConfig values, perhaps by making them available as a parameter or global variable that the JS config function can reference if needed.
| if (response?.ok) { | ||
| const data = await response.json(); | ||
|
|
||
| // Extract data from unified response | ||
| storageCapabilities = data.storageCapabilities; | ||
| version = data.version; | ||
| backendUiConfig = data.uiConfig; | ||
|
|
||
| console.log('[jaeger-ui-config] Fetched config from backend /api/ui/config'); | ||
| } |
There was a problem hiding this comment.
The optional chaining response?.ok is unnecessary here because if fetch completes successfully (doesn't throw), it always returns a Response object, never null or undefined. The optional chaining may give a false sense of safety - if the fetch succeeds but returns a non-ok status (e.g., 404, 500), the code silently skips config injection without logging any information about the error status.
Consider changing this to check only response.ok and optionally log the status code when it's not ok: if (!response.ok) { console.log('[jaeger-ui-config] Backend returned status: ' + response.status); }
| } catch (err) { | ||
| console.error('[jaeger-ui-config] Error loading jaeger-ui.config.js:', err); |
There was a problem hiding this comment.
The error object 'err' is declared but never used in this catch block. The error should either be logged for debugging purposes (especially helpful during development) or the parameter should be removed if intentionally unused. Consider changing to 'console.error' or adding a comment explaining why the error is deliberately ignored.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Aaryaa Newaskar <aryu.newaskar77@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (response?.ok) { | ||
| const data = await response.json(); | ||
|
|
||
| // Extract data from unified response | ||
| storageCapabilities = data.storageCapabilities; | ||
| version = data.version; | ||
| backendUiConfig = data.uiConfig; |
There was a problem hiding this comment.
Consider adding validation for the backend response to ensure it has the expected structure before attempting to access properties. For example, if data is null or not an object, accessing data.storageCapabilities could lead to unexpected behavior. Adding a basic check like if (data && typeof data === 'object') would make the code more robust.
| * This file previously contained a standalone OpenTelemetry verification script. | ||
| * It has been intentionally left empty to avoid keeping unreferenced tooling | ||
| * at the repository root. | ||
| */ |
There was a problem hiding this comment.
This file has been intentionally emptied but not removed. If the file is no longer needed and there are no references to it, consider deleting it entirely rather than leaving an empty file with a comment. An empty file in the repository root can be confusing for future contributors who might wonder if it serves a purpose.
| * This file previously contained a standalone OpenTelemetry verification script. | |
| * It has been intentionally left empty to avoid keeping unreferenced tooling | |
| * at the repository root. | |
| */ | |
| * Deprecated OpenTelemetry verification entrypoint. | |
| * | |
| * This script is kept only for backward compatibility with existing tooling | |
| * or documentation that may still reference `verify_otel.cjs`. | |
| * It no longer performs any verification. | |
| */ | |
| if (require.main === module) { | |
| console.warn( | |
| 'verify_otel.cjs is deprecated and no longer performs any checks. ' + | |
| 'Please remove any references to this script.' | |
| ); | |
| } |
| let backendUiConfig: any = null; | ||
| let storageCapabilities: any = null; | ||
| let version: any = null; |
There was a problem hiding this comment.
The variables backendUiConfig, storageCapabilities, and version are typed as any. Consider using more specific types or at least defining a type interface for the expected backend response structure. This would provide better type safety and make the code more maintainable. For example:
interface BackendConfig {
uiConfig?: Record<string, any>;
storageCapabilities?: { archiveStorage?: boolean; metricsStorage?: boolean };
version?: { gitCommit: string; gitVersion: string; buildDate: string };
}
let backendConfig: BackendConfig | null = null;Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Aaryaa Newaskar <aryu.newaskar77@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| console.log( | ||
| `[jaeger-ui-config] Error fetching backend config: ${err.message}`, | ||
| err | ||
| ); |
There was a problem hiding this comment.
The error message in the console.log at line 80-82 is misleading. It logs the error object twice - once in the string interpolation and once as a separate parameter. The second parameter err will be logged by the console, so the error object appears duplicated in the output. Consider removing the second err parameter or logging just the error without the interpolated message.
| console.log( | |
| `[jaeger-ui-config] Error fetching backend config: ${err.message}`, | |
| err | |
| ); | |
| console.log('[jaeger-ui-config] Error fetching backend config:', err); |
| let version: any = null; | ||
|
|
||
| const controller = new AbortController(); | ||
| const timeout = setTimeout(() => controller.abort(), 1000); |
There was a problem hiding this comment.
The timeout value (1000ms) is hardcoded in the implementation. This should be extracted as a named constant at the top of the function or module level to make it more maintainable and easier to adjust if needed. Consider defining something like const BACKEND_FETCH_TIMEOUT_MS = 1000;.
| `const JAEGER_CONFIG = ${JSON.stringify(finalUiConfig)};` | ||
| ); | ||
| console.log('[jaeger-ui-config] Loaded config from jaeger-ui.config.json'); | ||
| return html; | ||
| } catch (err) { | ||
| console.error('[jaeger-ui-config] Error loading jaeger-ui.config.json:', err); | ||
| } | ||
| } | ||
|
|
||
| // Inject storageCapabilities into index.html | ||
| if (storageCapabilities) { | ||
| html = html.replace( | ||
| 'const JAEGER_STORAGE_CAPABILITIES = DEFAULT_STORAGE_CAPABILITIES;', | ||
| `const JAEGER_STORAGE_CAPABILITIES = ${JSON.stringify(storageCapabilities)};` | ||
| ); | ||
| } | ||
|
|
||
| // Inject version into index.html | ||
| if (version) { | ||
| html = html.replace( | ||
| 'const JAEGER_VERSION = DEFAULT_VERSION;', | ||
| `const JAEGER_VERSION = ${JSON.stringify(version)};` |
There was a problem hiding this comment.
The JSON.stringify calls on lines 129, 138, and 146 inject configuration data into a script tag but don't escape the string </script>. If the backend returns configuration containing </script> in any string value, it could prematurely close the script tag. While the risk is low (requires malicious localhost backend), consider using .replace(/<\/script>/gi, '<\\/script>') on the JSON.stringify output as a defense-in-depth measure. This follows best practices for injecting JSON into HTML script contexts.
|
I’ve gone through the changes in this PR and the updated files. One thing I wanted to know that in packages/jaeger-ui/vite.config.mts, the plugin is always enabled, and transformIndexHtml also runs during vite build. |
|
It does not run during the production build. While the plugin logic itself is present in this configuration file, it is designed to be triggered only when a developer run the development server (npm start / vite). |
Signed-off-by: aryunewaskar77-art <aryu.newaskar77@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {config?.mainTitle && <h2 className="main-title-empty-state">{config.mainTitle}</h2>} | ||
| {config?.subTitle && <h3 className="sub-title-empty-state">{config.subTitle}</h3>} | ||
| {config?.description && <h4 className="description-empty-state">{config.description}</h4>} | ||
| {config?.button?.text && ( | ||
| <Button className="button-empty-state" onClick={() => config.button?.onClick?.()}> | ||
| {config.button.text} | ||
| </Button> | ||
| )} | ||
| {config.alert && ( | ||
| {config?.alert && ( |
There was a problem hiding this comment.
The optional chaining operators (config?.mainTitle, config?.subTitle, etc.) are redundant because there's already an early return on line 14-16 that checks if config is null/undefined. These lines will only execute if config is truthy, so the optional chaining is unnecessary and can be removed for cleaner code.
| if (!config) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The new null check on lines 14-16 adds a code path that returns early when config is null/undefined. However, the existing test file (index.test.js) only tests the case where config is defined and doesn't cover the scenario where getConfigValue returns null/undefined. Consider adding a test case to verify that the component renders null when config is not available, ensuring this defensive code path is covered.
| const parsedJsonConfig = JSON.parse(jsonContent); | ||
|
|
||
| // Shallow merge: JSON on top of backend base | ||
| finalUiConfig = { ...backendUiConfig, ...parsedJsonConfig }; |
There was a problem hiding this comment.
The shallow merge approach here differs from how the get-config.ts handles merging. In get-config.ts (lines 42-47), only fields listed in mergeFields are shallow merged, while other fields are completely overwritten. However, this implementation always does a shallow merge of the entire uiConfig object regardless of field semantics.
This could lead to unexpected behavior where nested properties from backendUiConfig inadvertently bleed through when they should be completely replaced. Consider applying the same mergeFields logic here as used in get-config.ts to maintain consistency.
Signed-off-by: aryunewaskar77-art <aryu.newaskar77@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const mergeFields = ['dependencies', 'search', 'tracking', 'monitor']; | ||
| mergeFields.forEach(key => { | ||
| if ( | ||
| parsedJsonConfig && | ||
| typeof parsedJsonConfig[key] === 'object' && | ||
| parsedJsonConfig[key] !== null | ||
| ) { | ||
| const backendValue = backendUiConfig ? backendUiConfig[key] : {}; | ||
| finalUiConfig[key] = { ...backendValue, ...parsedJsonConfig[key] }; | ||
| } | ||
| }); |
There was a problem hiding this comment.
The merge-field list is hardcoded here (['dependencies', 'search', 'tracking', 'monitor']) and also maintained in src/constants/default-config.ts as mergeFields. Duplicating this list risks drift (future config fields could be merged differently in dev vs prod). Consider centralizing the list in a small shared module that can be imported by both the runtime config merger and this Vite plugin (without pulling in browser-only code).
| return { | ||
| name: 'jaeger-ui-config', | ||
| configureServer(server) { | ||
| configureServer(server: import('vite').ViteDevServer) { | ||
| server.watcher.add([jsConfigPath, jsonConfigPath]); | ||
| server.watcher.on('change', path => { | ||
| if (path === jsConfigPath || path === jsonConfigPath) { | ||
| console.log(`[jaeger-ui-config] Config changed: ${path}. Triggering full reload...`); | ||
| server.watcher.on('change', (changedPath: string) => { | ||
| if (changedPath === jsConfigPath || changedPath === jsonConfigPath) { | ||
| console.log(`[jaeger-ui-config] Config changed: ${changedPath}. Triggering full reload...`); | ||
| server.ws.send({ type: 'full-reload', path: '*' }); | ||
| } | ||
| }); | ||
| }, | ||
| transformIndexHtml: { | ||
| order: 'pre' as const, | ||
| async handler(html: string) { |
There was a problem hiding this comment.
The transformIndexHtml hook runs during vite build as well as dev server. With the new backend fetch + HTML replacements, a local backend running during build would bake UI config/capabilities/version into the built index.html and remove the exact placeholder patterns the query-service expects to search/replace at runtime. Consider restricting this plugin to dev-only (e.g., apply: 'serve' and/or guard on Vite command === 'serve') so production builds keep the injection markers intact.
Signed-off-by: aryunewaskar77-art <aryu.newaskar77@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
packages/jaeger-ui/vite.config.mts:167
- The vite.config.mts plugin now has significant new logic for fetching backend config, merging configurations, and handling errors. However, there are no tests for this plugin's behavior. Consider adding unit tests for the jaegerUiConfigPlugin to verify:
- Correct handling when backend is available vs unavailable
- Proper config merging between backend, JSON, and JS configs
- Timeout behavior
- Error handling for malformed responses
- Correct HTML replacement for all three data types (uiConfig, storageCapabilities, version)
This is especially important since this logic now runs on every page load during development and impacts the developer experience.
function jaegerUiConfigPlugin() {
const jsConfigPath = path.resolve(__dirname, 'jaeger-ui.config.js');
const jsonConfigPath = path.resolve(__dirname, 'jaeger-ui.config.json');
return {
name: 'jaeger-ui-config',
apply: 'serve' as const,
configureServer(server: import('vite').ViteDevServer) {
server.watcher.add([jsConfigPath, jsonConfigPath]);
server.watcher.on('change', (changedPath: string) => {
if (changedPath === jsConfigPath || changedPath === jsonConfigPath) {
console.log(`[jaeger-ui-config] Config changed: ${changedPath}. Triggering full reload...`);
server.ws.send({ type: 'full-reload', path: '*' });
}
});
},
transformIndexHtml: {
order: 'pre' as const,
async handler(html: string) {
let backendUiConfig: any = null;
let storageCapabilities: any = null;
let version: any = null;
const controller = new AbortController();
const timeout = setTimeout(() => controller.abort(), 1000);
try {
const fetchOptions = { signal: controller.signal };
const response = await fetch('http://127.0.0.1:16686/api/ui/config', fetchOptions).catch(
() => null
);
if (response?.ok) {
const data = await response.json();
// Extract data from unified response
storageCapabilities = data.storageCapabilities;
version = data.version;
backendUiConfig = data.uiConfig;
console.log('[jaeger-ui-config] Fetched config from backend /api/ui/config');
}
} catch (err) {
// Silent fallback for production behavior, but log for dev visibility
if (err instanceof Error && err.name === 'AbortError') {
console.log('[jaeger-ui-config] Backend fetch timed out (1s)');
} else if (err instanceof Error) {
console.log(`[jaeger-ui-config] Error fetching backend config: ${err.message}`, err);
} else {
console.log('[jaeger-ui-config] Unknown error fetching backend config:', err);
}
} finally {
clearTimeout(timeout);
}
// Check for JS config first (highest priority - no merge)
if (fs.existsSync(jsConfigPath)) {
try {
const jsContent = fs.readFileSync(jsConfigPath, 'utf-8');
const uiConfigFn = `function UIConfig() { ${jsContent} }`;
html = html.replace('// JAEGER_CONFIG_JS', uiConfigFn);
console.log(
'[jaeger-ui-config] Loaded config from jaeger-ui.config.js (full override, no merge)'
);
// Note: Don't return early here, we still need to inject storageCapabilities and version below
} catch (err) {
console.error('[jaeger-ui-config] Error loading jaeger-ui.config.js:', err);
}
} else {
// Handle JSON config merged on top of backend UI config
let finalUiConfig = backendUiConfig;
if (fs.existsSync(jsonConfigPath)) {
try {
const jsonContent = fs.readFileSync(jsonConfigPath, 'utf-8');
const parsedJsonConfig = JSON.parse(jsonContent);
// Shallow merge: JSON on top of backend base
finalUiConfig = { ...backendUiConfig, ...parsedJsonConfig };
mergeFields.forEach(key => {
if (
parsedJsonConfig &&
typeof parsedJsonConfig[key] === 'object' &&
parsedJsonConfig[key] !== null
) {
const backendValue = backendUiConfig ? backendUiConfig[key] : {};
finalUiConfig[key] = { ...backendValue, ...parsedJsonConfig[key] };
}
});
console.log(
'[jaeger-ui-config] Merged config from jaeger-ui.config.json on top of backend uiConfig'
);
} catch (err) {
console.error('[jaeger-ui-config] Error loading jaeger-ui.config.json:', err);
}
} else if (backendUiConfig) {
console.log('[jaeger-ui-config] Using backend uiConfig as base');
}
// Inject final UI config into index.html
if (finalUiConfig) {
html = html.replace(
'const JAEGER_CONFIG = DEFAULT_CONFIG;',
`const JAEGER_CONFIG = ${JSON.stringify(finalUiConfig)};`
);
}
}
// Inject storageCapabilities into index.html
if (storageCapabilities) {
html = html.replace(
'const JAEGER_STORAGE_CAPABILITIES = DEFAULT_STORAGE_CAPABILITIES;',
`const JAEGER_STORAGE_CAPABILITIES = ${JSON.stringify(storageCapabilities)};`
);
}
// Inject version into index.html
if (version) {
html = html.replace(
'const JAEGER_VERSION = DEFAULT_VERSION;',
`const JAEGER_VERSION = ${JSON.stringify(version)};`
);
}
return html;
},
},
};
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Aaryaa Newaskar <aryu.newaskar77@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "@babel/cli": "7.28.0", | ||
| "@babel/core": "7.28.5", | ||
| "@testing-library/dom": "^10.4.1", | ||
| "@types/node": "^25.2.3", |
There was a problem hiding this comment.
The repo declares engines.node as >=24, but this adds @types/node ^25.2.3. If the project is targeting Node 24 in CI/dev, consider aligning the major version of @types/node with the supported runtime (or pinning a known-good version) to avoid type drift and incompatibilities.
| "@types/node": "^25.2.3", | |
| "@types/node": "^24.0.0", |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Aaryaa Newaskar <aryu.newaskar77@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (err) { | ||
| // Silent fallback for production behavior, but log for dev visibility | ||
| if (err instanceof Error && err.name === 'AbortError') { | ||
| console.log('[jaeger-ui-config] Backend fetch timed out (1s)'); | ||
| } else if (err instanceof Error) { | ||
| console.log(`[jaeger-ui-config] Error fetching backend config: ${err.message}`, err); | ||
| } else { | ||
| console.log('[jaeger-ui-config] Unknown error fetching backend config:', err); | ||
| } |
There was a problem hiding this comment.
Backend fetch failures/timeouts are logged inside transformIndexHtml, which runs frequently during dev. If the backend isn’t running, this will repeatedly print the same message on every request/reload. Consider logging once per TTL (or behind a debug flag) to avoid noisy console output.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Fields that should be merged with user-supplied config values rather than overwritten. | ||
| type TMergeField = 'dependencies' | 'search' | 'tracking'; | ||
| export const mergeFields: readonly TMergeField[] = ['dependencies', 'search', 'tracking']; | ||
| import { mergeFields } from './config-keys'; |
There was a problem hiding this comment.
The import statement at line 122 is placed after the const declaration, which is unconventional. While JavaScript/TypeScript hoists imports automatically, it's more idiomatic and consistent with typical code style to place all imports at the top of the file (lines 3-10). Consider moving this import to line 11 to group it with other imports.
Fixes #3541
This PR improves the Jaeger UI development experience by making the Vite dev server automatically fetch configuration from the running Jaeger backend. Previously, developers had to manually tweak local config files to test features like the Monitor tab or Archive storage. With this update, the UI now reflects the backend’s actual capabilities by default.
Key Changes
Automatic Backend Sync: Fetches storageCapabilities (archive/metrics), version, and uiConfig from http://localhost:16686/api/ui/config.
Smart Config Merging:
Highest priority: jaeger-ui.config.js — full manual override for advanced dev scenarios.
Next: jaeger-ui.config.json — shallow merge on top of backend defaults for easy tweaks.
Fallback: backend defaults automatically used if no local config exists.
Resilient Fallback: Added a 1-second fetch timeout and graceful failover. If the backend isn’t running, the UI still works with default settings.
Better Developer Feedback: Clear console logs show which config source is currently active, making debugging easier.
Why This Is Needed
Before this change, dev mode could be misleading: feature flags and tabs like Monitor wouldn’t appear unless a developer manually created a JSON config. Now, the UI behaves almost exactly like production out-of-the-box, reducing friction and guesswork during development.
Testing Done
✅ Backend running: window.getJaegerStorageCapabilities() correctly reflects backend settings (e.g., archiveStorage: true).
✅ Backend down: Dev server starts without errors and defaults are used.
✅ Local JSON overrides: Confirmed jaeger-ui.config.json merges correctly with backend values.
✅ UI behavior: Monitor tab and Archive buttons appear/disappear according to backend state.