Add Datadog Browser RUM client instrumentation for Next.js App Router - #610
Add Datadog Browser RUM client instrumentation for Next.js App Router#610jbeard4 wants to merge 9 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d302a22d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| service: DATADOG_SERVICE, | ||
| context: { | ||
| app_name: DATADOG_APP_NAME, | ||
| raw_pathname: pathname, |
There was a problem hiding this comment.
View names are normalized via normalizePathnameForViewName, but raw_pathname is still attached verbatim to each startView event here. On routes with dynamic segments (IDs, tokens, or other unique slugs), this sends unsanitized high-cardinality data to Datadog and can expose sensitive path values, which defeats the redaction/cardinality controls implemented elsewhere in this change. Remove this field or sanitize it before adding it to the event context.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR introduces browser RUM instrumentation, but it is not safe to approve yet due to a privacy risk and untested risky logic.
Blocking issues
- Privacy/PII risk:
datadogRum.startViewsendsraw_pathname: pathnamein event context without sanitization. Path segments can contain identifiers/emails, which can leak user data to telemetry. - Risky logic has no tests: route normalization/sanitization (
normalizePathnameForViewName,sanitizeStringValue,beforeSend) and single-init behavior are critical for privacy and telemetry correctness but are untested, leaving regression risk.
There was a problem hiding this comment.
The PR adds browser RUM instrumentation, but it introduces privacy-risky telemetry behavior without sufficient safeguards/tests, so it is not safe to approve yet.
Blocking issues
- Privacy/compliance risk:
DatadogRumInitinitializes RUM unconditionally for all users when tokens are present, withtrackUserInteractions,trackResources, andtrackLongTasksenabled, and no consent gate (src/components/datadog-rum-init.tsx). The README even notes consent is only a future TODO. This can capture user activity before consent. - PII leakage risk:
beforeSendsanitization is incomplete (src/components/datadog-rum-init.tsx,src/components/datadog-rum-utils.ts). It only redacts query/hash + email patterns in a small subset of fields (view.url,resource.url,error.message), leaving other event fields/contexts potentially containing sensitive identifiers. - Risky logic lacks integration coverage: only utility-unit tests were added (
src/components/datadog-rum-utils.test.ts), but there are no tests validating initialization behavior in app runtime (single init across rerenders/navigation, manualstartViewcorrectness, and privacy filtering application to emitted events). Per conservative policy, this is a blocking regression risk.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fd03eca06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const [pathWithoutQuery] = value.split("?"); | ||
| return pathWithoutQuery.split("#")[0] ?? pathWithoutQuery; |
There was a problem hiding this comment.
stripQueryAndHash removes everything after the first ? and then everything after #, and sanitizeStringValue is used on error.message in beforeSend. In practice this drops critical debugging context for normal error strings containing those characters (for example, messages with private-field names like #x, or fetch errors that include a URL and then status/details after a query string), so Datadog receives an incomplete message. Consider limiting query/hash stripping to URL fields or sanitizing URL substrings inside messages instead of truncating the entire message.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Datadog RUM wiring is close, but there are privacy-consent handling gaps that make this unsafe to approve as-is.
Blocking issues
- Privacy/consent regression: consent is only checked during
useEffectruns keyed topathname, and after initialization there is no call to update Datadog tracking consent (for example when a user revokes consent). RUM can continue collecting/sending telemetry after opt-out. (src/components/datadog-rum-init.tsx) - Consent state changes are not handled reactively unless navigation occurs. If consent is granted or revoked without a route change, behavior is stale (no init when newly granted, no shutdown/update when revoked), which is an unclear and risky analytics behavior needing explicit handling/tests. (
src/components/datadog-rum-init.tsx)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60753e52cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!hasDatadogConfiguration || !hasConsent || isAlreadyInitialized) { | ||
| return false; |
There was a problem hiding this comment.
When consent changes from granted to denied after RUM has already been initialized, this early return path exits without updating the Datadog SDK consent state, so the already-active instance keeps collecting automatic telemetry (trackResources, trackUserInteractions, trackLongTasks) for that session. This only happens when users can revoke consent mid-session, but in that flow it violates the intended consent gate and should explicitly transition the SDK to non-granted tracking.
Useful? React with 👍 / 👎.
| return decodeURIComponent( | ||
| cookie.slice(cookieName.length + 1), | ||
| ).toLowerCase(); |
There was a problem hiding this comment.
The consent parser decodes the cookie value without error handling, so a malformed value (for example one containing an invalid % escape) throws URIError during the client consent check and interrupts RUM initialization/view tracking on that render cycle. This is input-dependent, but a single bad cookie should not break telemetry logic; wrap decoding in a safe fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The PR introduces useful Datadog RUM gating and tests, but it is not safe to approve yet due to privacy and runtime robustness gaps in the new sanitization/consent utilities.
Blocking issues
- Potential PII leakage:
sanitizeRumEventstops recursion after depth 6 (MAX_SANITIZE_DEPTHinsrc/components/datadog-rum-utils.ts), so deeply nested event fields are left unsanitized and may still contain emails/query tokens. Since this code is intended to prevent sensitive data from being sent to Datadog, this is a blocking privacy risk. - Client runtime crash risk:
parseCookieValuecallsdecodeURIComponent(...)without a try/catch (src/components/datadog-rum-utils.ts). A malformed cookie value (invalid percent-encoding) will throwURIError, which can break theDatadogRumIniteffect insrc/components/datadog-rum-init.tsxand cause app-level instability.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0248193a57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const NUMERIC_SEGMENT_PATTERN = /^\d+$/; | ||
| const UUID_SEGMENT_PATTERN = | ||
| /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; | ||
| const LONG_TOKEN_SEGMENT_PATTERN = /^[a-z0-9_-]{24,}$/i; |
There was a problem hiding this comment.
normalizePathnameForViewName treats any segment matching ^[a-z0-9_-]{24,}$ as :id, which also matches legitimate long static slugs (for example internal-comment-guidelines, a real route constant in this repo). In those cases, startRumView will collapse distinct static pages into the same view name (route:/:id), reducing route-level accuracy in RUM dashboards and breaking page-specific analysis even when no dynamic identifier is present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I can’t safely approve this PR yet. The Datadog RUM integration introduces privacy-risky behavior and has gaps in test coverage for critical runtime flows.
Blocking issues
- Potential PII leakage in RUM view names:
normalizePathnameForViewNameonly masks numeric/UUID/long-token segments, so human-readable slugs (including names or other identifiers) are sent viastartRumViewunchanged. This can leak user-identifying path data to Datadog. - Missing tests for the highest-risk logic in
datadog-rum-init.tsx: there is no component-level test validating consent transitions and initialization/view-start ordering. Given this controls third-party telemetry enablement, the current unit tests are insufficient to rule out regressions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 442d6c952b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,140 @@ | |||
| "use client"; | |||
|
|
|||
| import { datadogRum } from "@datadog/browser-rum"; | |||
There was a problem hiding this comment.
Because @datadog/browser-rum is imported at module load and DatadogRumInit is mounted in the root layout, every client still downloads and parses the RUM SDK even when NEXT_PUBLIC_DATADOG_ENABLED is false or consent is not granted. In those common no-op paths, this adds avoidable JavaScript cost to all page loads and undercuts the runtime gating in this change; loading the SDK dynamically only after the gate passes avoids that startup regression.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cannot safely approve due to a functional regression risk in the RUM init component that can generate duplicate view events and repeated side effects in production.
Blocking issues
src/components/datadog-rum-init.tsx:runtimeDepsis recreated on every render (const runtimeDeps = deps ?? createDeps()), but all threeuseEffecthooks depend onruntimeDeps. This makes effects rerun on any rerender, sostartRumViewFncan fire duplicatestartViewcalls even whenpathname/consent did not change, and consent/init logic is re-executed unnecessarily. This can inflate telemetry, increase cost, and distort behavior.- Missing test coverage for the default (non-injected) dependency path: existing tests only pass a stable
depsobject, so they do not catch the production identity-churn issue above. Add a regression test assertingstartViewis not called repeatedly on rerenders withoutpathname/consent changes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f72c0cd1ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (SAFE_VIEW_SEGMENTS.has(segment)) { | ||
| return segment; | ||
| } | ||
|
|
||
| return ":slug"; |
There was a problem hiding this comment.
normalizePathnameForViewName falls back to :slug for every segment not in SAFE_VIEW_SEGMENTS, but the allowlist does not include several canonical static routes used by this app (for example privacy-policy and terms-of-use from src/components/common.ts). In practice, many distinct pages collapse into identical view names (e.g. /privacy-policy and /locations both normalize to route:/:slug), which degrades route-level RUM accuracy and can skew page-specific dashboards/experiments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
No blocking security/privacy/auth/data-loss issues found in the Datadog Browser RUM instrumentation diff. Consent gating, initialization guard, and event/path sanitization are present, and the added tests cover core consent/init/view and sanitization behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3251112e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const interval = runtimeDeps.setIntervalFn(() => { | ||
| setHasConsent(runtimeDeps.getConsentStatus()); | ||
| }, CONSENT_REFRESH_INTERVAL_MS); |
There was a problem hiding this comment.
This effect always starts a 1s interval even when hasDatadogConfiguration is false (for example the documented default NEXT_PUBLIC_DATADOG_ENABLED=false), so every client session still wakes up every second to re-read consent/cookies despite RUM being a no-op. In that disabled path this recurring timer does unnecessary background work on all page loads and tabs; short-circuiting the polling effect when configuration is missing avoids the avoidable CPU/battery overhead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I can’t approve this safely. The new Datadog init component appears to access window during render, which can break SSR/hydration paths in Next.js.
Blocking issues
src/components/datadog-rum-init.tsx:useMemo(() => deps ?? createDeps(), [deps])callscreateDeps()during render, andcreateDeps()directly readswindow(window.clearInterval,window.setInterval). In Next.js App Router, client components can be rendered on the server; this can throwReferenceError: window is not definedand break page rendering. Move allwindowaccess intouseEffector guard withtypeof window !== 'undefined'before dereferencing.
There was a problem hiding this comment.
The Datadog Browser RUM integration is gated behind explicit env/config checks and consent, initializes only on the client, includes a single-init guard, and adds targeted sanitization in beforeSend to reduce accidental PII leakage. The PR also includes meaningful unit tests for consent flow, initialization/start-view sequencing, route normalization, and event sanitization. No blocking security, privacy, or regression issues were identified from the provided diff.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b3131e516
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sessionReplaySampleRate: DATADOG_SESSION_REPLAY_ENABLED | ||
| ? Number.isFinite(DATADOG_SESSION_REPLAY_SAMPLE_RATE) | ||
| ? DATADOG_SESSION_REPLAY_SAMPLE_RATE | ||
| : 100 |
There was a problem hiding this comment.
When NEXT_PUBLIC_DATADOG_SESSION_REPLAY_ENABLED is true and NEXT_PUBLIC_DATADOG_SESSION_REPLAY_SAMPLE_RATE is malformed (for example an empty string or non-numeric value), this branch falls back to 100, which enables replay collection for all sessions instead of defaulting to a safe disabled value. In a misconfigured deploy this can unexpectedly capture far more session replay data (and cost) than intended, so the fallback should be conservative.
Useful? React with 👍 / 👎.
1 participant
Motivation
Description
@datadog/browser-rumand added a client-only initializer atsrc/components/datadog-rum-init.tsxthat reads configuration fromNEXT_PUBLIC_DATADOG_*env vars, guards against duplicate initialization, and no-ops when required IDs are missing.applicationId,clientToken,site,service,env,version,sessionSampleRate,sessionReplaySampleRate,trackUserInteractions,trackResources,trackLongTasks,defaultPrivacyLevel) and gates session replay behindNEXT_PUBLIC_DATADOG_SESSION_REPLAY_ENABLED(default off).allowedTracingUrls(includeswindow.location.originplus a configurableNEXT_PUBLIC_DATADOG_TRACING_ORIGINS), and implementstrackViewsManuallyplusstartViewwith normalized route names to reduce dynamic-route cardinality.beforeSendsanitization to strip query/hash fragments and redact email-like strings, setsapp_namecontext, wiresDatadogRumInitinto the App Router root layout atsrc/app/layout.tsx, and documents required vars and a consent TODO inREADME.md.Testing
npm install @datadog/browser-rumto add the SDK and updatedpackage.jsonandpackage-lock.jsonsuccessfully.npm run check-typeswhich passed after minor TypeScript narrowings were applied tobeforeSend(type errors were resolved).npm run lintwhich completed (existing unrelated lint warnings remain in other files and were not introduced by this change).npm run buildwhich failed in this environment due to repeated Google Fonts fetch errors fromfonts.gstatic.comunrelated to the RUM changes, so build failure is environmental and not caused by the Datadog instrumentation.Codex Task