pal-e-docs

Scope Review: #87 Deploy Grafana Pyroscope for continuous profiling

review-1310-2026-06-04 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- Child of #43, depends on #85 (Tempo)
  • [x] Repo -- Two repos: forgejo_admin/pal-e-platform (Helm + Terraform), ldraney/landscaping-assistant (app instrumentation)
  • [x] User Story -- Clear: platform operator wants continuous profiling with flame graphs linked to traces
  • [x] Context -- Strong motivation, references current gap, explains value proposition
  • [/] File Targets -- 4 files listed, but 2 have incorrect paths (see File Targets section)
  • [x] Acceptance Criteria -- 4 testable conditions
  • [/] Test Expectations -- 2 items, thin relative to constraints (see Acceptance Criteria section)
  • [x] Constraints -- 3 items: MinIO backend, perf benchmark, Tempo dependency
  • [x] Checklist -- Multi-repo PR checklist
  • [x] Related -- References parent #43, prerequisite #85, project slug

Traceability

  • [x] story:observability -- present on board item #1310
  • [x] arch:platform, arch:rails-app -- present on board item #1310 (correctly reflects dual-repo scope)
  • [x] Forgejo issue -- #87 exists and is open
  • [x] parent:43 -- label present, matches Lineage in issue body
  • [x] type:feature -- label present, matches issue Type header

File Targets

  • [!] terraform/pyroscope.tf (pal-e-platform) -- WRONG PATH. The pal-e-platform repo organizes all monitoring Helm releases inside terraform/modules/monitoring/main.tf. Existing releases (kube-prometheus-stack, loki-stack, blackbox-exporter) are resource blocks in that file. A top-level terraform/pyroscope.tf would be outside the monitoring module scope -- the namespace, provider config, and variable references are module-scoped. An agent following this target would produce broken Terraform.
  • [!] terraform/grafana.tf (pal-e-platform) -- WRONG PATH. The Grafana Loki datasource is a ConfigMap in terraform/modules/monitoring/main.tf. A top-level grafana.tf has no access to the module's resource references. Correct location: add a ConfigMap resource to terraform/modules/monitoring/main.tf or a new file within that module.
  • [x] Gemfile (landscaping-assistant) -- Exists. No pyroscope gem present yet. Correct target.
  • [x] config/initializers/pyroscope.rb (landscaping-assistant) -- Does not exist yet. Parent directory config/initializers/ exists. Correctly new.
Assessment: The two pal-e-platform file targets would misdirect an agent. The correct targets are resources within terraform/modules/monitoring/main.tf (or a new terraform/modules/monitoring/pyroscope.tf if the preference is to split by component). Also missing: terraform/modules/monitoring/variables.tf will likely need new variables (MinIO bucket config, Pyroscope version pin).

Repo Placement

This is correctly structured as a multi-repo ticket: platform infrastructure in pal-e-platform, app instrumentation in landscaping-assistant. The checklist includes separate PR checkboxes for each repo. However, it is filed as a single Forgejo issue in landscaping-assistant only. For a multi-repo change, the convention is either a tracking issue with child issues per repo, or a single issue that explicitly names both repos (which this does). Acceptable as-is given the scope is manageable, but worth noting that the pal-e-platform work is the larger portion.

Dependencies

  • [/] #85 Tempo -- OPEN (not yet started, also in backlog). The ticket correctly notes trace-to-profile linking depends on Tempo but Pyroscope can deploy standalone. This is well-handled -- the hard dependency is only on AC #4 (trace-to-profile correlation), not the full deployment. However, AC #4 is not marked as conditional in the Acceptance Criteria. An agent would try to verify trace linking and fail if Tempo is not deployed.
  • [x] MinIO -- Already deployed (used by existing Loki stack). No new dependency.
  • [x] Grafana -- Already deployed (kube-prometheus-stack). No new dependency.

Acceptance Criteria

  • AC1: Pyroscope deployed and healthy -- Testable. kubectl pod check + ready status.
  • AC2: Grafana Explore returns profiles -- Testable. Navigate to Grafana, query Pyroscope datasource.
  • AC3: Flame graphs visible for Rails endpoints -- Testable but vague. Which endpoints? After what traffic? Needs a concrete verification step (e.g., "hit /properties 10 times, flame graph shows ActionController and ActiveRecord frames").
  • AC4: Profiles link to traces -- Testable only if Tempo (#85) is deployed. Not marked conditional. Should say "conditional on #85" or be split to a follow-up ticket.
Test Expectations gap: Constraints require "benchmark before/after Puma request throughput" but this is absent from Test Expectations. The constraint creates an implicit acceptance criterion with no verification method.

Blast Radius

  • Files touched: 4 files across 2 repos (2 Terraform resources, 1 Gemfile entry, 1 initializer)
  • Services affected: Monitoring namespace (new pod), landscaping-assistant (new gem in runtime)
  • What could break: (1) Puma throughput degradation from profiling overhead -- mitigated by benchmark constraint. (2) MinIO storage growth from continuous profile data -- no retention policy mentioned in AC or constraints. (3) Monitoring namespace resource pressure from additional pod.
  • Rollback: Straightforward. Remove Helm release, remove gem + initializer, deploy. No data migration involved.

Decomposition Assessment

  • Discrete changes: 3 (Helm release + datasource, Gemfile addition, initializer config). Within the three-thing limit.
  • Time estimate: Within the five-minute rule for an agent -- the pattern is well-established (copy loki-stack Helm release pattern, add gem, add initializer).
  • Parallelization: The pal-e-platform PR and landscaping-assistant PR are independent and could be executed in parallel. The issue structure supports this (separate checklist items).
No decomposition needed. Scope is appropriate for a single ticket.

Technical Verification

  • pyroscope gem: CONFIRMED. Exists on RubyGems as pyroscope v1.0.7 (released 2026-05-15). Actively maintained by Pyroscope team. Apache-2.0 license. 331K+ downloads. No deprecation notices.
  • Helm chart: CONFIRMED. Chart name is pyroscope in the https://grafana.github.io/helm-charts repository. Matches the observability roadmap Technology Reference table (grafana/pyroscope).
  • Observability roadmap: docs/observability-roadmap.md exists and confirms Pyroscope as Phase 3. Architecture diagram, rollout plan, and technology reference are all consistent with the issue scope.

Recommendation

  • Fix Terraform file targets. Change terraform/pyroscope.tf and terraform/grafana.tf to either:
  • Add Puma benchmark to Test Expectations. The Constraints section requires "benchmark before/after" but Test Expectations has no corresponding verification step. Add: "Benchmark: compare Puma requests/sec with and without pyroscope gem (must not degrade >X%)".
  • Mark AC #4 (trace-to-profile linking) as conditional on #85. Either add "(requires #85 Tempo)" to the criterion, or split it to a follow-up ticket that depends on both #85 and #87.
  • Tighten AC #3. "Flame graphs visible for Rails endpoints" is vague. Specify a concrete verification: e.g., "After generating traffic to /properties, Pyroscope shows CPU flame graph with Rails framework frames (ActionController, ActiveRecord)".