Skip to content

MINOR - React and Java telemetry#27773

Merged
TeddyCr merged 20 commits into
mainfrom
MINOR-Sentry-UI-Backend
May 1, 2026
Merged

MINOR - React and Java telemetry#27773
TeddyCr merged 20 commits into
mainfrom
MINOR-Sentry-UI-Backend

Conversation

@TeddyCr
Copy link
Copy Markdown
Collaborator

@TeddyCr TeddyCr commented Apr 27, 2026

Describe your changes:

Added telemetry for UI and Java

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

Summary by Gitar

  • Configuration:
    • Introduced SentryConfiguration schema to manage error tracking and performance monitoring settings.
    • Updated OpenMetadataApplicationConfig to include the new Sentry configuration block.
  • Telemetry initialization:
    • Initialized IndexResource on application startup to inject Sentry configuration details into the UI index.html.
    • Added helper methods in IndexResource for safe JavaScript variable escaping.

This will update automatically on new commits.

Copilot AI review requested due to automatic review settings April 27, 2026 17:44
@github-actions github-actions Bot added Ingestion safe to test Add this label to run secure Github workflows on PRs labels Apr 27, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.html placeholders (sentryDsn, sentryEnvironment, sentryTraceSampleRate) from environment variables.
  • Populate clusterName from OPENMETADATA_CLUSTER_NAME.
  • Populate appVersion from VersionResource’s catalog version.

Copilot AI review requested due to automatic review settings April 27, 2026 18:40
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comment on lines +40 to +53
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()));
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
Comment on lines +40 to +53
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()));
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings April 30, 2026 15:09
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Apr 30, 2026

Code Review 👍 Approved with suggestions 4 resolved / 5 findings

Refactored telemetry to ensure proper Sentry placeholder replacement and secure HTML injection, though the lazy initialization in getSentryConfiguration() requires synchronization for thread safety.

💡 Bug: Lazy getSentryConfiguration() is not thread-safe

📄 openmetadata-service/src/main/java/org/openmetadata/service/OpenMetadataApplicationConfig.java:171-179

The getSentryConfiguration() method at line 174 performs lazy initialization (if (sentryConfiguration == null) { sentryConfiguration = new SentryConfiguration(); }) without synchronization, and the field is not volatile. In theory, two threads could each see null and create separate instances. In practice this is likely only called during single-threaded startup, but it's inconsistent with defensive patterns used elsewhere. Other similar getters in the same class (e.g., getAdminOpsConfiguration) appear to follow the same pattern, so this is a pre-existing style — but worth noting for the new field.

✅ 4 resolved
Bug: Sentry placeholders not replaced in getIndex() endpoint path

📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/system/IndexResource.java:46-48 📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/system/IndexResource.java:87-96
The new sentry/cluster/version replacements (lines 56-66) are only applied in the static getIndexFile(String basePath) method, which is used by OpenMetadataAssetServlet. However, the getIndex() JAX-RS endpoint (line 87-96) serves this.indexHtml, which is only processed by initialize() (line 46-48). Since initialize() only replaces ${basePath}, the getIndex() endpoint will serve HTML containing literal unreplaced placeholders like ${sentryDsn}, ${sentryEnvironment}, ${sentryTraceSampleRate}, ${clusterName}, and ${appVersion}.

The same replacements need to be applied in initialize() (or indexHtml should be computed using getIndexFile).

Performance: Env vars and VersionResource re-evaluated on every request

📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/system/IndexResource.java:53-66
getIndexFile() is called on every HTML-serving request from OpenMetadataAssetServlet. Each call reads 4 environment variables via System.getenv(), instantiates a new VersionResource, and performs 6 string replacements on the full HTML. These values (env vars, app version) are effectively static for the lifetime of the process. The result should be computed once and cached, not recomputed per request.

Security: Env var values injected into HTML without escaping

📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/system/IndexResource.java:56-65
Environment variable values (SENTRY_UI_DSN, SENTRY_ENVIRONMENT, OPENMETADATA_CLUSTER_NAME, etc.) are interpolated directly into the HTML template without HTML/JS escaping. If any env var contains characters like </script> or similar HTML-significant content, it could break the page or introduce an XSS vector. While these values are controlled by server administrators (reducing practical risk), defensive escaping is good practice for any value injected into HTML.

Bug: NPE if getIndexFile() called before initialize()

📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/system/IndexResource.java:27 📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/system/IndexResource.java:69
The field configProcessedHtml was changed from a static final (guaranteed initialized in the static block) to a static volatile field that is only set when initialize() is called. If any code path invokes getIndexFile() before initialize() completes (e.g., during startup race or a misconfigured test), configProcessedHtml.replace(...) at line 69 will throw a NullPointerException.

The previous code was safe because CONFIG_PROCESSED_HTML was assigned in the static initializer, guaranteeing it was non-null by the time the class was loaded.

🤖 Prompt for agents
Code Review: Refactored telemetry to ensure proper Sentry placeholder replacement and secure HTML injection, though the lazy initialization in getSentryConfiguration() requires synchronization for thread safety.

1. 💡 Bug: Lazy getSentryConfiguration() is not thread-safe
   Files: openmetadata-service/src/main/java/org/openmetadata/service/OpenMetadataApplicationConfig.java:171-179

   The `getSentryConfiguration()` method at line 174 performs lazy initialization (`if (sentryConfiguration == null) { sentryConfiguration = new SentryConfiguration(); }`) without synchronization, and the field is not `volatile`. In theory, two threads could each see `null` and create separate instances. In practice this is likely only called during single-threaded startup, but it's inconsistent with defensive patterns used elsewhere. Other similar getters in the same class (e.g., `getAdminOpsConfiguration`) appear to follow the same pattern, so this is a pre-existing style — but worth noting for the new field.

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud
Copy link
Copy Markdown

@sonarqubecloud
Copy link
Copy Markdown

@TeddyCr TeddyCr merged commit 457afab into main May 1, 2026
68 of 69 checks passed
@TeddyCr TeddyCr deleted the MINOR-Sentry-UI-Backend branch May 1, 2026 18:36
TeddyCr added a commit that referenced this pull request May 1, 2026
* chore: added sentry for UI

* chore: added sentry for UI

* chore: added sentry for UI

* chore: added sentry for UI and backend

* chore: move setup to config

* Update generated TypeScript types

* chore: addressed CI comments

* chore: handle no ui startup in IndexResource

---------

Co-authored-by: IceS2 <pablo.takara@getcollate.io>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
(cherry picked from commit 457afab)
jatinmasaram pushed a commit to jatinmasaram/OpenMetadata that referenced this pull request May 2, 2026
* chore: added sentry for UI

* chore: added sentry for UI

* chore: added sentry for UI

* chore: added sentry for UI and backend

* chore: move setup to config

* Update generated TypeScript types

* chore: addressed CI comments

* chore: handle no ui startup in IndexResource

---------

Co-authored-by: IceS2 <pablo.takara@getcollate.io>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
jaya6400 pushed a commit to jaya6400/OpenMetadata that referenced this pull request May 4, 2026
* chore: added sentry for UI

* chore: added sentry for UI

* chore: added sentry for UI

* chore: added sentry for UI and backend

* chore: move setup to config

* Update generated TypeScript types

* chore: addressed CI comments

* chore: handle no ui startup in IndexResource

---------

Co-authored-by: IceS2 <pablo.takara@getcollate.io>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ingestion safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants