Skip to content

feat(rum): scope latest-metrics RUM to locale path prefix#1707

Open
alinarublea wants to merge 2 commits into
mainfrom
feature/rum-locale-path-filtering
Open

feat(rum): scope latest-metrics RUM to locale path prefix#1707
alinarublea wants to merge 2 commits into
mainfrom
feature/rum-locale-path-filtering

Conversation

@alinarublea

Copy link
Copy Markdown
Contributor

Why

Locale-specific sites (e.g. https://example.com/de) have no RUM domain key of their own — domain keys exist only for the main domain. Today wwwUrlResolver strips the path and the totalMetrics query aggregates RUM for the entire parent domain, so a locale site's latest-metrics reports domain-wide numbers rather than its own subtree.

What

Result-level path scoping for totalMetrics, opt-in via a new pathPrefix option:

  • spacecat-shared-utils — new getBaseURLPathPrefix(baseURL) helper: extracts the locale path (https://example.com/de/de), returns null at the domain root or for unparseable input.
  • spacecat-shared-rum-api-client — the totalMetrics handler now reads opts.pathPrefix and filters bundles to that subtree before aggregating. Matching is boundary-safe: /de matches /de and /de/... but not /design.

With no pathPrefix, behavior is byte-for-byte identical to today — no existing caller is affected.

How it fits together

domain stays the bare hostname (required by the bundler URL and for domain-key resolution on the main domain). The locale scoping happens purely at the result level. The consuming side — spacecat-api-service's getLatestSiteMetrics extracting the prefix from site.getBaseURL() and passing it through — lands in a follow-up PR in that repo (this shared release must publish first).

The prefix-matching logic is intentionally kept self-contained in rum-api-client rather than importing the new util, to avoid a cross-package version bump (rum-api-client pins an exact spacecat-shared-utils version).

Out of scope

Subdomain-based locales (de.example.com) — wwwUrlResolver returns the full subdomain host, a separate case to be addressed independently.

Tests

  • getBaseURLPathPrefix: root → null, trailing slash, single/multi-segment, schema-less, unparseable, null/undefined. url-helpers.js at 100% coverage.
  • totalMetrics: aggregates all without pathPrefix (regression guard); scopes to subtree with it; rejects look-alike sibling paths; drops bundles with missing/invalid URLs.
  • Both package suites green; coverage gates pass.

🤖 Generated with Claude Code

Locale-specific sites (e.g. https://example.com/de) have no RUM domain
key of their own — keys exist only for the main domain. Today the
totalMetrics query resolves the bare hostname and aggregates RUM for the
entire parent domain, so locale sites report domain-wide numbers instead
of their own subtree.

Add result-level path scoping:
- spacecat-shared-utils: getBaseURLPathPrefix(baseURL) extracts the
  locale path (e.g. '/de'), or null at the domain root.
- spacecat-shared-rum-api-client: the totalMetrics handler now reads
  opts.pathPrefix and filters bundles to that subtree before aggregating,
  with boundary-safe matching ('/de' does not match '/design').

Opt-in: with no pathPrefix the handler behaves exactly as before.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@MysticatBot MysticatBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey @alinarublea,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Approve - clean, well-scoped feature with solid test coverage.
Complexity: LOW - small diff, single package surface.
Changes: Adds opt-in locale path-prefix filtering to the totalMetrics RUM handler plus a getBaseURLPathPrefix utility for extracting the path prefix from a site's baseURL (6 files).

Non-blocking (3): minor issues and suggestions
  • nit: matchesPathPrefix does not normalize prefix - if a caller passes /de/ (trailing slash), the boundary check path.startsWith('/de//') silently fails to match nested paths. A defensive prefix.replace(/\/+$/, '') would make this robust against future callers that skip normalization - packages/spacecat-shared-rum-api-client/src/functions/total-metrics.js:22
  • nit: Silent catch blocks in both matchesPathPrefix and getBaseURLPathPrefix provide no observability signal when bundles or URLs are dropped due to parse failure. For a data-aggregation pipeline, systematically malformed URLs would silently degrade metrics with no alert - packages/spacecat-shared-rum-api-client/src/functions/total-metrics.js:23
  • suggestion: Add a test for empty-string pathPrefix to lock in the falsy-means-no-filter contract, since '' could arrive from upstream coercion of a null return - packages/spacecat-shared-rum-api-client/test/total-metrics.test.js:79

Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 8m 37s | Cost: $3.58 | Commit: 851176e272acc7e92d556b4533e2a1a80791e94a
If this code review was useful, please react with 👍. Otherwise, react with 👎.

@MysticatBot MysticatBot added ai-reviewed Reviewed by AI complexity:low AI-assessed PR complexity: LOW labels Jun 23, 2026
@github-actions

Copy link
Copy Markdown

This PR will trigger a minor release when merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed Reviewed by AI complexity:low AI-assessed PR complexity: LOW

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants