pal-e-docs

Scope Review: board-landscaping-observability#1312 (Issue #89 -- Grafana Faro RUM)

review-1312-2026-06-04 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Board item: board-landscaping-observability#1312
Forgejo issue: ldraney/landscaping-assistant #89 -- "Deploy Grafana Faro for frontend real user monitoring"
Reviewed: 2026-06-04 by QA agent

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- Child of #43, Phase 5, depends on #85
  • [x] Repo -- Two repos: pal-e-platform + landscaping-assistant
  • [x] User Story -- Clear "who wants what and why"
  • [x] Context -- Sufficient background for a fresh-context agent
  • [ ] File Targets -- Present but underspecified (see findings)
  • [x] Acceptance Criteria -- 4 testable conditions
  • [x] Test Expectations -- 2 integration-level items
  • [ ] Constraints -- Present but missing critical packaging constraint (see findings)
  • [x] Checklist -- Covers both repos
  • [x] Related -- References project and parent issue

Traceability

  • [x] story:observability -- present on board item #1312
  • [x] arch:platform, arch:rails-app -- present on board item #1312
  • [x] Forgejo issue #89 -- exists and is open
  • [x] parent:43 -- label present, lineage matches

File Targets

  • [ ] "OTel Collector config (pal-e-platform)" -- No specific file path given. This is in a different repo (pal-e-platform) which is outside the scope of direct verification here, but the issue must name the exact file (e.g., terraform/otel-collector.tf or a values file).
  • [ ] app/javascript/application.js -- File exists (verified). However, the issue says "or layout" which is ambiguous. The file currently contains a hand-rolled error reporter (reportClientError, window.onerror, unhandledrejection handlers at lines 5-25) that Faro would replace. The issue does not mention this existing code or whether it should be removed.
  • [ ] Missing target: config/importmap.rb -- Would need modification to pin any Faro JS, but see packaging blocker below.
Assessment: Targets are NOT specific enough for an agent to act on without guessing.

Repo Placement

This is a multi-repo ticket spanning forgejo_admin/pal-e-platform (OTel Collector Faro receiver) and ldraney/landscaping-assistant (frontend SDK). The issue correctly identifies both repos and the checklist has separate PR checkboxes for each. This is acceptable as structured -- it acts as a tracking issue with work in two repos.
However, the two repos have independent scopes. The platform-side work (Faro receiver endpoint on OTel Collector) could be its own ticket. Consider whether splitting would improve parallelism.

Dependencies

  • [ ] #85 (Tempo + OTel Collector) -- PENDING. Issue #85 is open, in backlog column on the board, with no labels indicating progress. The OTel Collector itself (deployed in #85) is a hard prerequisite -- Faro has nowhere to send telemetry without it. The Faro trace propagation AC ("Frontend spans propagate TraceID to backend") explicitly requires Tempo. This dependency is correctly documented in the issue but is unresolved.

Acceptance Criteria

  • "Faro SDK loaded in production frontend" -- Testable. Can verify via browser devtools network tab or page source.
  • "Core Web Vitals (LCP, FID, CLS) visible in Grafana" -- Testable but FID is deprecated. Google replaced FID with INP (Interaction to Next Paint) in March 2024. Faro SDK v1.4+ reports INP. Should say INP, not FID.
  • "JS errors captured with stack traces in Loki" -- Testable. The test expectation ("Intentional JS error appears in Loki within 30s") maps to this.
  • "Frontend spans propagate TraceID to backend (W3C trace context headers)" -- Testable but blocked by #85. Cannot verify without Tempo.

Blast Radius

  • Files touched: 2-3 in landscaping-assistant (JS entrypoint, possibly importmap config, possibly layout), 1+ in pal-e-platform (OTel Collector config)
  • What could break: If the SDK packaging is wrong, page loads could fail (JS errors blocking Turbo). If the Faro receiver is misconfigured, the OTel Collector could reject all telemetry. Existing window.onerror handler conflict with Faro's error capture is a risk.
  • Rollback: Straightforward -- revert JS changes, Faro is additive observability that does not affect app functionality.

Decomposition Assessment

Three-thing limit: This ticket has 4+ discrete changes:
  • Configure Faro receiver on OTel Collector (pal-e-platform)
  • Install and initialize Faro SDK (landscaping-assistant)
  • Handle Turbo Drive navigation in Faro config
  • Remove or reconcile existing error reporter code
The platform-side work (#1) is independent and could be a separate ticket. However, the landscaping-assistant changes (#2-4) are tightly coupled and should stay together. Borderline -- acceptable as-is if the platform work is simple, but splitting would be cleaner.
Five-minute rule: The packaging question (see Finding #1 below) adds significant ambiguity that could cause an agent to spend considerable time researching solutions. With the packaging strategy specified, this is a reasonable-sized ticket.

Findings

Finding 1 (BLOCKING): Faro SDK packaging incompatible with importmap-rails

The issue says to install @grafana/faro-web-sdk and @grafana/faro-web-tracing in app/javascript/application.js. However, landscaping-assistant uses importmap-rails with no Node.js toolchain, no package.json, no bundler. The Faro SDK is an npm package with deep dependency trees (OpenTelemetry JS SDK internals) that cannot be pinned via importmap.
The issue MUST specify a packaging strategy:
  • jsbundling-rails migration -- Switch to esbuild/vite. Significant prerequisite that should be its own ticket.
  • Vendored pre-built bundle -- Build Faro offline, place in app/assets/builds/, pin via importmap. Avoids toolchain change but requires a manual rebuild process for updates.
  • CDN script tag -- Load Faro from unpkg.com or cdn.jsdelivr.net in the layout. Simplest but adds external runtime dependency.

Finding 2 (BLOCKING): Observability roadmap references SvelteKit -- this app is Rails

The observability roadmap (docs/observability-roadmap.md) Phase 5 section (line 191) states: "@grafana/faro-web-sdk in SvelteKit apps". The architecture diagram (line 14) labels the frontend as SVELTE["SvelteKit Frontend"]. But landscaping-assistant is a Rails 8.1 app with Hotwire/Turbo/Stimulus. The issue body correctly identifies "Rails frontend uses Hotwire/Turbo" in the Constraints section, but the source document it references is wrong.
Either fix docs/observability-roadmap.md to reference Rails/Hotwire, or add a note in the issue that the roadmap's SvelteKit reference is incorrect.

Finding 3 (BLOCKING): File Targets underspecified

Two problems:
  • The pal-e-platform target has no file path -- just "OTel Collector config".
  • The landscaping-assistant target says "application.js or layout" -- ambiguous. An agent must know which.
  • Missing: no mention of removing the existing hand-rolled error reporter in app/javascript/application.js (lines 5-25) which conflicts with Faro's native error capture.

Finding 4 (Non-blocking): FID deprecated, should be INP

Acceptance criteria reference "Core Web Vitals (LCP, FID, CLS)". FID was replaced by INP as a Core Web Vital in March 2024. Update to "(LCP, INP, CLS)".

Finding 5 (Non-blocking): Test expectations lack unit-level coverage

Both test expectations are integration-level (requires Grafana + Loki). Consider adding a smoke test: "Faro SDK initializes without JS console errors on page load".

Finding 6 (Non-blocking): Bundle size constraint needs verification method

The constraint "Faro SDK bundle size must stay under 15KB gzipped" is good but the issue should specify how to verify it, especially since the packaging strategy affects measurement.

Recommendation

  • Specify the Faro SDK packaging strategy. Add a Constraints entry choosing between jsbundling-rails migration, vendored bundle, or CDN script tag. If jsbundling-rails, create a prerequisite ticket for the migration.
  • Fix file targets. Replace "OTel Collector config (pal-e-platform)" with the exact file path. Replace "application.js or layout" with a definitive choice. Add a note that the existing reportClientError / window.onerror / unhandledrejection code in app/javascript/application.js (lines 5-25) should be removed since Faro handles this natively.
  • Fix the observability roadmap. Update docs/observability-roadmap.md Phase 5 and the architecture diagram to reference Rails/Hotwire instead of SvelteKit.
  • Update FID to INP in acceptance criteria.
  • Add a smoke test expectation: "Faro SDK initializes without JS console errors on page load."