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:
|
| return "", false | ||
| } | ||
|
|
||
| func TestResourceService_Get_SetsSpanAttributes(t *testing.T) { |
There was a problem hiding this comment.
None of the four new tests exercise the no-op tracing path, even though "no-op safe" is called out in the acceptance criteria. Worth adding a test that calls one of these methods without an active tracer installed.
There was a problem hiding this comment.
Every other test that calls one of the methods and doesn't set up the tracer makes sure of this already
| return "", false | ||
| } | ||
|
|
||
| func TestResourceService_Get_SetsSpanAttributes(t *testing.T) { |
There was a problem hiding this comment.
These tests only cover the happy path. Get/Patch/Delete tag the span with resource_id/resource_type even on a failed lookup (attributes are set before the DAO call), which is useful for debugging 404s, but that behavior isn't verified by a test.
| if err != nil { | ||
| return nil, handleCreateError(kind, err) | ||
| } | ||
| trace.SpanFromContext(ctx).SetAttributes( | ||
| attribute.String("hyperfleet.resource_id", resource.ID), | ||
| attribute.String("hyperfleet.resource_type", kind), | ||
| ) |
There was a problem hiding this comment.
Create sets span attributes after the DAO call (since resource.ID isn't known until insert succeeds), so a failed create gets no resource_id/resource_type on the span at all. This differs from Get/Patch/Delete, which do tag failed lookups. resource_type (kind) is known upfront here though, so it could be set before the DAO call. Not covered by any test either way.
| func TestResourceService_Get_SetsSpanAttributes(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestDescriptors() | ||
|
|
||
| tp, exporter := setupTestTracer(t) | ||
|
|
||
| mockDao := newMockResourceDao() | ||
| svc, _, _ := newTestResourceService(mockDao) | ||
| mockDao.addResource(testResource("Channel", "ch-1", "stable")) | ||
|
|
||
| ctx, span := otel.Tracer("test").Start(context.Background(), "test") | ||
| _, svcErr := svc.Get(ctx, "Channel", "ch-1") | ||
| span.End() | ||
| Expect(svcErr).To(BeNil()) | ||
|
|
||
| if err := tp.ForceFlush(context.Background()); err != nil { | ||
| t.Fatalf("failed to flush spans: %v", err) | ||
| } | ||
| spans := exporter.GetSpans() | ||
|
|
||
| resourceID, found := findSpanAttribute(spans, "hyperfleet.resource_id") | ||
| Expect(found).To(BeTrue(), "hyperfleet.resource_id attribute not found") | ||
| Expect(resourceID).To(Equal("ch-1")) | ||
|
|
||
| resourceType, found := findSpanAttribute(spans, "hyperfleet.resource_type") | ||
| Expect(found).To(BeTrue(), "hyperfleet.resource_type attribute not found") | ||
| Expect(resourceType).To(Equal("Channel")) | ||
| } | ||
|
|
||
| func TestResourceService_Create_SetsSpanAttributes(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestDescriptors() | ||
|
|
||
| tp, exporter := setupTestTracer(t) | ||
|
|
||
| mockDao := newMockResourceDao() | ||
| svc, _, _ := newTestResourceService(mockDao) | ||
| resource := testResource("Channel", "ch-new", "beta") | ||
|
|
||
| ctx, span := otel.Tracer("test").Start(context.Background(), "test") | ||
| result, svcErr := svc.Create(ctx, "Channel", resource, nil) | ||
| span.End() | ||
| Expect(svcErr).To(BeNil()) | ||
|
|
||
| if err := tp.ForceFlush(context.Background()); err != nil { | ||
| t.Fatalf("failed to flush spans: %v", err) | ||
| } | ||
| spans := exporter.GetSpans() | ||
|
|
||
| resourceID, found := findSpanAttribute(spans, "hyperfleet.resource_id") | ||
| Expect(found).To(BeTrue(), "hyperfleet.resource_id attribute not found") | ||
| Expect(resourceID).To(Equal(result.ID)) | ||
| Expect(resourceID).ToNot(BeEmpty(), "resource_id should not be empty") | ||
|
|
||
| resourceType, found := findSpanAttribute(spans, "hyperfleet.resource_type") | ||
| Expect(found).To(BeTrue(), "hyperfleet.resource_type attribute not found") | ||
| Expect(resourceType).To(Equal("Channel")) | ||
| } | ||
|
|
||
| func TestResourceService_Patch_SetsSpanAttributes(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestDescriptors() | ||
|
|
||
| tp, exporter := setupTestTracer(t) | ||
|
|
||
| mockDao := newMockResourceDao() | ||
| svc, _, _ := newTestResourceService(mockDao) | ||
| mockDao.addResource(testResource("Channel", "ch-1", "stable")) | ||
|
|
||
| patch := &api.ResourcePatch{Spec: map[string]interface{}{"key": "updated"}} | ||
| ctx, span := otel.Tracer("test").Start(context.Background(), "test") | ||
| _, svcErr := svc.Patch(ctx, "Channel", "ch-1", patch) | ||
| span.End() | ||
| Expect(svcErr).To(BeNil()) | ||
|
|
||
| if err := tp.ForceFlush(context.Background()); err != nil { | ||
| t.Fatalf("failed to flush spans: %v", err) | ||
| } | ||
| spans := exporter.GetSpans() | ||
|
|
||
| resourceID, found := findSpanAttribute(spans, "hyperfleet.resource_id") | ||
| Expect(found).To(BeTrue(), "hyperfleet.resource_id attribute not found") | ||
| Expect(resourceID).To(Equal("ch-1")) | ||
|
|
||
| resourceType, found := findSpanAttribute(spans, "hyperfleet.resource_type") | ||
| Expect(found).To(BeTrue(), "hyperfleet.resource_type attribute not found") | ||
| Expect(resourceType).To(Equal("Channel")) | ||
| } | ||
|
|
||
| func TestResourceService_Delete_SetsSpanAttributes(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestDescriptors() | ||
|
|
||
| tp, exporter := setupTestTracer(t) | ||
|
|
||
| mockDao := newMockResourceDao() | ||
| svc, _, _ := newTestResourceService(mockDao) | ||
| mockDao.addResource(testResource("Channel", "ch-1", "stable")) | ||
|
|
||
| ctx, span := otel.Tracer("test").Start(context.Background(), "test") | ||
| _, svcErr := svc.Delete(ctx, "Channel", "ch-1") | ||
| span.End() | ||
| Expect(svcErr).To(BeNil()) | ||
|
|
||
| if err := tp.ForceFlush(context.Background()); err != nil { | ||
| t.Fatalf("failed to flush spans: %v", err) | ||
| } | ||
| spans := exporter.GetSpans() | ||
|
|
||
| resourceID, found := findSpanAttribute(spans, "hyperfleet.resource_id") | ||
| Expect(found).To(BeTrue(), "hyperfleet.resource_id attribute not found") | ||
| Expect(resourceID).To(Equal("ch-1")) | ||
|
|
||
| resourceType, found := findSpanAttribute(spans, "hyperfleet.resource_type") | ||
| Expect(found).To(BeTrue(), "hyperfleet.resource_type attribute not found") | ||
| Expect(resourceType).To(Equal("Channel")) | ||
| } |
There was a problem hiding this comment.
These four tests share the same setup/flush/assert scaffolding; only the method under test (and a couple minor per-case details) really changes. pkg/middleware/otel_test.go already shows a table-driven pattern for span tests in this repo, worth following the same shape here to cut the duplication.
…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>
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