feat: reconcile-to-request trace correlation via span links - #207
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: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
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.
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>
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>
- Reconcile RTC-01 spec text with the shipped per-plugin migration approach (migrationAddTraceContext per plugin, unique IDs) instead of a single migration. - Document that an update with telemetry disabled intentionally overwrites stored trace context to NULL (gorm Save writes nil -> NULL), matching RTC-01 most-recent-mutation semantics; add spec scenario and a code comment on CaptureTraceContext. - Document that server-initiated RoleBinding paths (CreateGatewayOwnerBinding, SyncJWTRoles) intentionally skip CaptureTraceContext and are knowingly link-less. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
de18e0b to
c7e61cb
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
APPROVE. This is a clean, well-specified observability change: it threads W3C Trace Context from API-server writes through the database and gRPC watch stream into control-plane reconcile spans as OTel span links, with a dedicated spec (RTC-01..RTC-05), good unit coverage, and correct privacy handling (json:"-" keeps trace context out of REST responses). Only minor, non-blocking notes below.
Amber Analysis
The design choice of a span link ("caused by") rather than a synthetic parent-child relationship is the correct OTel model for temporally-decoupled reconciliation, and it is documented well in specs/platform/reconcile-trace-correlation.spec.md. The shared TraceMeta embeddable is a sound way to add columns uniformly without editing the upstream api.Meta, per-plugin migrations keep schema ownership local, and migration IDs are unique across the six plugins (no gormigrate collision). Error handling is safe throughout: parseTraceparentLink silently returns false on empty/malformed input (RTC-03), the proto getters are nil-safe for delete events, and sanitizeError keeps identifiers out of exported telemetry. Regenerated common.pb.go matches the common.proto change (fields 6/7 as optional), and no pre-existing test assertion was flipped - the only edits to existing tests are additive or the mechanical 3-arg to 4-arg signature update.
Notes (Minor)
-
parseTraceparentLinkreconstructs the linkedSpanContextfromtraceparentonly and never carriestracestate, even though RTC-03 says the control plane SHALL parse "thetraceparent(andtracestatewhen present)".tracestateis persisted, shipped over gRPC (field 7), and read into the reconciler, but then dropped. Either wire it into the link'sSpanContextConfig.TraceStateor tighten the spec wording so the implementation and requirement agree. -
The trace-context migrations use IDs dated
2026-08-25(2026082500000002.....0007), which sort earlier than migrations already present on the same tables (e.g.gatewayshas2026082813000007). This is cosmetic because gormigrate applies migrations in registration order andmigrationAddTraceContextis registered last, so it still runs last - but the out-of-sequence timestamp is confusing to a future reader who expects ID order to reflect apply order. Consider a timestamp later than the existing migrations.
Cross-PR coordination
No material cross-PR coordination issue requires maintainer action.
Findings Summary (ordered by severity, highest first)
- [Minor]
tracestateis persisted and transported but dropped when building the span link, diverging from RTC-03 - Spec Consistency (reconcile.go L101) - [Minor] Trace-context migration IDs are dated earlier than existing migrations on the same tables (cosmetic; order is registration-based) - Convention/Clarity (migration.go L12)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / malformed input handled gracefully |
Pass |
| No secrets in logs or responses | Pass |
Trace context absent from routine REST responses (json:"-") |
Pass |
OpenAPI/proto client not hand-edited (common.pb.go regenerated) |
Pass |
| Unique gormigrate migration IDs across plugins | Pass |
| Reconcile pattern (context propagation, span-per-reconcile) | Pass |
| Test Diff Scrutiny (no flipped assertions, additive tests) | Pass |
| Conventional commit messages | Pass |
| return trace.Link{}, false | ||
| } | ||
|
|
||
| sc := trace.NewSpanContext(trace.SpanContextConfig{ |
There was a problem hiding this comment.
The linked SpanContext is built from traceparent only; tracestate is never applied to SpanContextConfig.TraceState. RTC-03 states the control plane SHALL parse "the traceparent (and tracestate when present)". Since tracestate is already persisted (field 7) and delivered to the reconciler, consider carrying it into the link, or adjust the spec so the requirement matches the implementation. Minor.
|
|
||
| func migrationAddTraceContext() *gormigrate.Migration { | ||
| return &gormigrate.Migration{ | ||
| ID: "2026082500000002", |
There was a problem hiding this comment.
This migration ID (2026082500000002, dated 2026-08-25) sorts earlier than migrations already on the gateways table (e.g. 2026082813000007). It works because gormigrate applies migrations in registration order and this one is registered last, but the out-of-sequence timestamp is misleading. Consider a timestamp later than the existing migrations for readability. Cosmetic/Minor.
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