HYPERFLEET-1490 - feat: add hyperfleet.resource_id span attribute to service layer - #328
HYPERFLEET-1490 - feat: add hyperfleet.resource_id span attribute to service layer#328Ruclo wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe resource service now records Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/services/resource_test.go`:
- Around line 3160-3163: Update the t.Cleanup callback around tp.Shutdown to
check and report its error, and ensure all four ForceFlush results are checked
and reported rather than ignored. Use the test’s existing error-reporting
mechanism so incomplete span export or shutdown failures fail the test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 30542a9b-4b85-44d4-9812-6fa69da75e45
📒 Files selected for processing (2)
pkg/services/resource.gopkg/services/resource_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
Risk Score: 1 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 274 lines (>200) | +1 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
Computed by hyperfleet-risk-scorer
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
| trace.SpanFromContext(ctx).SetAttributes( | ||
| attribute.String("hyperfleet.resource_id", id), | ||
| attribute.String("hyperfleet.resource_type", kind), | ||
| ) |
There was a problem hiding this comment.
hyperfleet.resource_type is set to the Kind (e.g. "Cluster") here and at the other three call sites, but the Tracing Standard and Sentinel's existing spans use the plural form (e.g. "clusters"). This mismatch means a TraceQL query on resource_type won't match across API and Sentinel spans for the same resource. Consider using the descriptor's plural form instead.
| func setupTestTracer(t *testing.T) (*sdktrace.TracerProvider, *tracetest.InMemoryExporter) { | ||
| t.Helper() | ||
| exporter := tracetest.NewInMemoryExporter() | ||
| tp := sdktrace.NewTracerProvider( | ||
| sdktrace.WithSampler(sdktrace.AlwaysSample()), | ||
| sdktrace.WithSyncer(exporter), | ||
| ) | ||
| prev := otel.GetTracerProvider() | ||
| otel.SetTracerProvider(tp) | ||
| t.Cleanup(func() { | ||
| if err := tp.Shutdown(context.Background()); err != nil { | ||
| t.Errorf("failed to shutdown tracer: %v", err) | ||
| } | ||
| otel.SetTracerProvider(prev) | ||
| }) | ||
| return tp, exporter |
There was a problem hiding this comment.
setupTestTracer swaps the global otel TracerProvider for the test, then each test starts its span via the global otel.Tracer rather than the tp returned by this helper. Using tp.Tracer(...) directly would avoid touching process-global state, which would remove a potential source of flakiness if t.Parallel() is ever added to these tests.
|
A few service methods aren't instrumented with these span attributes and would miss tracing coverage:
|
…service layer Set hyperfleet.resource_id and hyperfleet.resource_type span attributes on Get, Create, Patch, and Delete operations for resource-level trace correlation in Tempo. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| if svcErr := validateKind(kind); svcErr != nil { | ||
| return nil, svcErr | ||
| } | ||
| trace.SpanFromContext(ctx).SetAttributes( | ||
| attribute.String("hyperfleet.resource_id", id), | ||
| attribute.String("hyperfleet.resource_type", registry.MustGet(kind).Plural), | ||
| ) | ||
| resource, err := s.resourceDao.Get(ctx, kind, id) | ||
| if err != nil { | ||
| return nil, handleGetError(kind, "id", id, err) |
There was a problem hiding this comment.
The same two attributes end up getting set in four different orderings across the 8 instrumented methods here (Get/Patch/Delete tag both together after validateKind, Create splits type-early/id-late, GetByOwner/ProcessAdapterStatus/ForceDelete tag id before validateKind, GetByID tags id before the DAO call). None of these are unsafe, but on an invalid kind, some methods leave the span fully untagged while others still tag resource_id. Worth picking one ordering convention and applying it everywhere for consistency.
2 participants
Summary
hyperfleet.resource_idandhyperfleet.resource_typespan attributes on Get, Create, Patch, and Delete operations inpkg/services/resource.gotracetest.InMemoryExporterverifying span attributes are set for all 4 CRUD methodsTest plan
make lintpasses (0 issues)make testpasses (1428 tests)🤖 Generated with Claude Code