feat: reconcile-to-request trace correlation via span links - #207
feat: reconcile-to-request trace correlation via span links#207JuanmaBM wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Persist W3C traceparent/tracestate on every resource at create/update time, propagate through gRPC ObjectReference, and attach OTel span links on control-plane reconcile spans so Jaeger can navigate from a reconciliation back to the originating API request. API server: - TraceMeta embeddable struct (json:"-", gorm columns) - Proto: traceparent/tracestate on ObjectReference (fields 6-7) - All 7 plugins: model embed, migration, service injection, gRPC presenter - Service layer calls CaptureTraceContext(ctx) before dao.Create/Replace Control plane: - StartReconcileSpan accepts traceparent, creates span link via parseTraceparentLink - All 10 reconciler call sites updated (event-driven pass traceparent, continuous pass "") - 12 unit tests for traceparent parsing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
57e3635 to
f43d2aa
Compare
Add table-driven tests for CaptureTraceContext covering valid/invalid span contexts, tracestate propagation, unsampled flags, and idempotency. Document traceparent and tracestate fields in the ObjectReference proto message with W3C Trace Context format and emptiness semantics. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f43d2aa to
de18e0b
Compare
Amber reviewStatus: Complete VerdictCOMMENT — This is a clean, well-scoped, spec-backed observability feature. Trace context is captured from the active span at persistence time, kept out of REST responses ( Amber Analysis The design is correct for the asynchronous reconcile problem: a span link preserves causal reference without implying temporal containment, and the reconcile span stays an independently-sampled root (RTC-03 / CP-OBS-02). Error handling is exemplary — Confidence: High on the correctness of the parsing/link logic and security posture; Medium on the update-path nulling behavior (depends on the DAO's Findings1. [Minor] Spec says "a single gormigrate migration"; implementation ships seven per-plugin migrations — Spec Consistency 2. [Minor] Server-initiated RoleBinding creation paths bypass 3. [Minor] Update while telemetry is disabled may null a previously-stored Positives worth noting: Cross-PR coordinationI reviewed the other open PRs in
No other open PR (UI/web-console, e2e, kind/deps) intersects the trace-correlation data model, proto, or reconcile-span interface. Findings Summary (ordered by severity, highest first)
Convention Checklist
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — This is a clean, well-scoped, spec-backed observability feature. Trace context is captured from the active span at persistence time, kept out of REST responses (json:"-"), flows over gRPC via ObjectReference, and is attached as a best-effort span link (not a parent-child edge) on reconcile roots. No blockers: no panic(), robust parsing with silent fallback, additive nullable columns with a rollback, and solid table-driven tests. A few minor items and cross-PR coordination are noted below.
Amber Analysis
The design is correct for the asynchronous reconcile problem: a span link preserves causal reference without implying temporal containment, and the reconcile span stays an independently-sampled root (RTC-03 / CP-OBS-02). Error handling is exemplary — parseTraceparentLink validates part count, hex length, and zero IDs, and any malformed/empty value degrades silently to a normal root with no error, exactly as RTC-03 requires. Secrets/privacy are respected: traceparent/tracestate are opaque hex identifiers, excluded from REST via json:"-" (RTC-05), and never logged. Migration IDs are unique per plugin (2026082500000001–0007), so there is no gormigrate collision, and each migration is reversible.
Confidence: High on the correctness of the parsing/link logic and security posture; Medium on the update-path nulling behavior (depends on the DAO's Replace semantics, which I could not execute — Go toolchain unavailable in the review sandbox).
Findings
1. [Minor] Spec says "a single gormigrate migration"; implementation ships seven per-plugin migrations — Spec Consistency
specs/platform/reconcile-trace-correlation.spec.md:21 (RTC-01) states "A single gormigrate migration SHALL add the columns to all resource tables," but the implementation adds an independent migrationAddTraceContext() to each of the 7 plugins. The per-plugin approach is defensible (plugins own their schema, IDs stay unique), but the spec text and the code disagree. Reconcile one to the other so the spec remains the source of truth.
2. [Minor] Server-initiated RoleBinding creation paths bypass CaptureTraceContext — Completeness
plugins/roleBindings/service.go calls rb.CaptureTraceContext(ctx) in Create, but CreateGatewayOwnerBinding and SyncJWTRoles persist bindings via the DAO directly and will store NULL trace context. That is acceptable (these are internal, non-request flows with no meaningful originating request span), but worth confirming it is intentional so those reconciles are knowingly link-less.
3. [Minor] Update while telemetry is disabled may null a previously-stored traceparent — Correctness (low confidence)
On Replace, CaptureTraceContext leaves the pointer fields nil when no valid span is active. Depending on whether the DAO's Replace uses gorm Save (writes nil → NULL) versus Updates with a struct (skips zero values), an update performed with telemetry off could wipe a prior trace context. RTC-01's "overwrite on update" plus "NULL when telemetry disabled" arguably permits this, but please confirm the intended behavior since it is a silent data change on an existing column.
Positives worth noting: parseTraceparentLink rejects all-zero IDs and bad hex; TestParseTraceparentLink and TestCaptureTraceContext cover the meaningful edge cases including unsampled flags and idempotency; the only modified pre-existing test (otel_test.go) merely threads the new "" argument through — no weakened assertion or removed guarantee (Test Diff Scrutiny: clean).
Cross-PR coordination
I reviewed the other open PRs in openshift-online/hypershell (titles, bodies, changed-file lists, and relevant diffs). Material items requiring maintainer coordination:
-
#151
spec(control-plane): gate gateway re-provisioning, #179fix(control-plane): reconcile existing Keycloak clients, #194feat(control-plane): adopt upstream OpenShell Helm chart— Competing interface change + change-order. This PR changes the signature of the shared helpercpotel.StartReconcileSpan(ctx, kind, eventType) → (…, traceparent)and updates all 10 call sites, and it rewrites the top ofGatewayReconciler.Handleto passgw.GetMetadata().GetTraceparent(). All three of those PRs also modifyGatewayReconciler.Handle(and #179/#194 also touchreconciler/health.go) around the same region. None of them currently adopt the new 4-argument signature. Decision needed: whichever merges after #207 must update itsStartReconcileSpancall sites (and any newly-added reconcile spans, e.g. #194's newinternal/gatewaypackage) to the 4-arg form; whichever merges first forces #207 to rebase its call sites. The designs are compatible — this is a merge-order and interface-adoption decision, not a design clash. -
#151 (gateways plugin data model) — Additive overlap, compatible. Both #151 and #207 add a field + migration to the
gatewaysplugin (model.go,migration.go,plugin.goregistration,grpc_presenter.go) and edit the proto layer (#207:common.proto/ObjectReference; #151:gateways.proto/Gateway). Migration IDs are distinct (2026082500000002vs2026081912000006) and the added fields are different, so there is no logical or schema conflict — only textual merge coordination in the shared presenter/model files. -
#185
control-plane-world-sync, #200control-plane reconciliation contract, #148 (branch-build spec) — No material conflict. #185/#148/#207 each append a row tospecs/index.spec.md(trivial textual merge only). Conceptually #185's periodic world-sync is consistent with #207's design: after a resync-triggered reconcile, the span link still points to the last user mutation's trace, which is #207's intended overwrite-on-update semantics. #200 defines a reconciliation contract but does not touch the observability specs or the span-link mechanism. No design decision required.
No other open PR (UI/web-console, e2e, kind/deps) intersects the trace-correlation data model, proto, or reconcile-span interface.
Findings Summary (ordered by severity, highest first)
- [Minor] Spec claims a single migration; code ships 7 per-plugin migrations — Spec Consistency (spec L21)
- [Minor] Server-initiated RoleBinding creation paths bypass
CaptureTraceContext— Completeness (service.go L197) - [Minor] Update with telemetry disabled may null a stored
traceparent(DAO-dependent) — Correctness (tracemeta.go L21)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
| Errors handled without failing reconcile (silent fallback per RTC-03) | Pass |
No secrets in logs or responses (json:"-", opaque IDs) |
Pass |
| Input validated (traceparent part/hex/zero-ID checks) | Pass |
| Reconcile span remains root + span link, not parent-child | Pass |
| DB migration reversible + nullable columns for pre-existing rows | Pass |
| Migration IDs unique across plugins | Pass |
| Test Diff Scrutiny (no weakened pre-existing assertions) | Pass |
Proto/generated .pb.go regenerated, not hand-edited |
Pass (assumed; toolchain unavailable to re-verify) |
| Spec matches implementation | Fail (single vs per-plugin migration) |
|
|
||
| The API server SHALL capture the W3C Trace Context (`traceparent` header value, and `tracestate` when present) from the inbound request context on every create and update write, and SHALL persist both values on the resource row in PostgreSQL. The trace context SHALL be stored as plain text columns (`traceparent` and `tracestate`) on the shared `api.Meta` base, so every resource type inherits the field without per-plugin schema changes. | ||
|
|
||
| Because the `api.Meta` base struct is defined in the upstream `rh-trex-ai` framework and cannot be modified in-tree, the trace context columns SHALL be added via a local embeddable struct (for example `TraceMeta`) that each resource model embeds alongside `api.Meta`. A single gormigrate migration SHALL add the columns to all resource tables. |
There was a problem hiding this comment.
RTC-01 states "A single gormigrate migration SHALL add the columns to all resource tables," but the implementation registers a separate migrationAddTraceContext() in each of the 7 plugins. The per-plugin approach is reasonable (plugins own their schema and the IDs stay unique), but please reconcile the spec text with the code so the spec stays authoritative.
| return nil, errors.Forbidden("platform:admin can only be assigned via Keycloak") | ||
| } | ||
|
|
||
| rb.CaptureTraceContext(ctx) |
There was a problem hiding this comment.
CaptureTraceContext is called here in Create, but the server-initiated paths CreateGatewayOwnerBinding and SyncJWTRoles persist bindings via the DAO directly and will store NULL trace context. That is probably fine (no meaningful originating request span), but worth confirming it is intentional so those reconciles are knowingly link-less.
|
|
||
| // CaptureTraceContext extracts the active span's W3C traceparent and | ||
| // tracestate from ctx and stores them. When no valid span is active (OTel | ||
| // disabled or no sampled span), the fields are left nil. |
There was a problem hiding this comment.
On Replace, when no valid span is active (telemetry disabled), these pointers stay nil. Depending on whether the DAO's Replace uses gorm Save (writes nil -> NULL) vs Updates with a struct (skips zero values), an update performed with telemetry off could null out a previously-stored traceparent. RTC-01's overwrite-on-update arguably permits this, but please confirm the intended behavior since it is a silent change to existing data.
2 participants
Summary
traceparent/tracestateon every resource at create/update time via aTraceMetaembeddable struct (json:"-"keeps fields out of REST responses)ObjectReference(proto fields 6-7) so the control plane receives it on watch eventsChanges
API Server
pkg/api/tracemeta.go—TraceMetastruct withCaptureTraceContext(ctx)methodproto/common.proto—optional string traceparent/tracestateonObjectReferenceTraceMetaALTER TABLE ADD COLUMN IF NOT EXISTS traceparent/tracestateCaptureTraceContext(ctx)beforedao.Create/dao.ReplaceTraceparent/TracestatetoObjectReferenceControl Plane
StartReconcileSpanacceptstraceparentstring, creates span link viaparseTraceparentLink"")Spec
specs/platform/reconcile-trace-correlation.spec.md— 5 requirements (RTC-01 through RTC-05)Test plan
go build ./...andgo vet ./...clean on both API server and control plane🤖 Generated with Claude Code