Conversation
github-actions
Bot
added
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adds server-side placeholder substitution to the UI index.html payload so telemetry-related configuration (e.g., Sentry settings), cluster name, and application version can be injected at render time.
Changes:
- Populate additional
index.htmlplaceholders (sentryDsn,sentryEnvironment,sentryTraceSampleRate) from environment variables. - Populate
clusterNamefromOPENMETADATA_CLUSTER_NAME. - Populate
appVersionfromVersionResource’s catalog version.
Code Review ✅ Approved 3 resolved / 3 findingsIntegrates React and Java telemetry while resolving Sentry placeholder injection, redundant resource re-evaluation, and unescaped environment variable exposure. No issues found. ✅ 3 resolved✅ Bug: Sentry placeholders not replaced in getIndex() endpoint path
✅ Performance: Env vars and VersionResource re-evaluated on every request
✅ Security: Env var values injected into HTML without escaping
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
| CONFIG_PROCESSED_HTML = | ||
| RAW_INDEX_HTML | ||
| .replace("${sentryDsn}", escapeJs(System.getenv().getOrDefault("SENTRY_UI_DSN", ""))) | ||
| .replace( | ||
| "${sentryEnvironment}", | ||
| escapeJs(System.getenv().getOrDefault("SENTRY_ENVIRONMENT", "development"))) | ||
| .replace( | ||
| "${sentryTraceSampleRate}", | ||
| escapeJs(System.getenv().getOrDefault("SENTRY_TRACE_SAMPLE_RATE", "0.5"))) | ||
| .replace( | ||
| "${clusterName}", | ||
| escapeJs(System.getenv().getOrDefault("OPENMETADATA_CLUSTER_NAME", "openmetadata"))) | ||
| .replace( | ||
| "${appVersion}", escapeJs(new VersionResource().getCatalogVersion().getVersion())); |
There was a problem hiding this comment.
New logic injects multiple telemetry-related placeholders into the index HTML, but there are no tests asserting that these new placeholders are correctly replaced and safely escaped. Adding a test fixture /assets/index.html that includes the new tokens (or updating the existing test resource) and asserting the output of getIndexFile(...) contains the resolved values would prevent regressions (e.g., placeholders leaking to the client).
| public Response getIndex(@Context HttpServletRequest request) { | ||
| final String cspNonce = (String) request.getAttribute(CspNonceHandler.CSP_NONCE_ATTRIBUTE); | ||
| String html = indexHtml; | ||
| if (cspNonce != null && !cspNonce.isEmpty()) { | ||
| html = html.replace("${cspNonce}", cspNonce); | ||
| } | ||
| return Response.ok(html).build(); | ||
| return Response.ok(getIndexFile("/", cspNonce)).build(); | ||
| } |
There was a problem hiding this comment.
getIndex() always calls getIndexFile("/", cspNonce) which hardcodes the basePath to /. This bypasses the configured base path used elsewhere (e.g., OpenMetadataAssetServlet passes config.getBasePath()), and would break deployments served under a non-root base path if this JAX-RS resource is used. Consider deriving the basePath from configuration (as before) or from the request context, instead of hardcoding /.
| CONFIG_PROCESSED_HTML = | ||
| RAW_INDEX_HTML | ||
| .replace("${sentryDsn}", escapeJs(System.getenv().getOrDefault("SENTRY_UI_DSN", ""))) | ||
| .replace( | ||
| "${sentryEnvironment}", | ||
| escapeJs(System.getenv().getOrDefault("SENTRY_ENVIRONMENT", "development"))) | ||
| .replace( | ||
| "${sentryTraceSampleRate}", | ||
| escapeJs(System.getenv().getOrDefault("SENTRY_TRACE_SAMPLE_RATE", "0.5"))) | ||
| .replace( | ||
| "${clusterName}", | ||
| escapeJs(System.getenv().getOrDefault("OPENMETADATA_CLUSTER_NAME", "openmetadata"))) | ||
| .replace( | ||
| "${appVersion}", escapeJs(new VersionResource().getCatalogVersion().getVersion())); |
There was a problem hiding this comment.
The static HTML preprocessing replaces ${sentryDsn}, ${sentryEnvironment}, ${sentryTraceSampleRate}, ${clusterName}, and ${appVersion}. In the current repo, neither openmetadata-ui/src/main/resources/ui/index.html nor the test /assets/index.html contains these placeholders, so these replacements will be no-ops unless the built /assets/index.html template is also updated during the UI build/packaging. Please ensure the served /assets/index.html actually includes these tokens (or adjust the replacement keys accordingly), otherwise the telemetry config won’t be injected.
|
🟡 Playwright Results — all passed (13 flaky)✅ 3961 passed · ❌ 0 failed · 🟡 13 flaky · ⏭️ 86 skipped
🟡 13 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
Describe your changes:
Added telemetry for UI and Java
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
IndexResourceto pre-process Sentry environment variables intoCONFIG_PROCESSED_HTMLduring static initialization.escapeJsutility usingStringEscapeUtilsto sanitize environment variables before injection into the UI template.indexHtmlstate and legacyinitializemethod.getIndexresponse generation by centralizing configuration replacement logic.This will update automatically on new commits.