Extract OTEL provider initialization out of runServeCmd#46421
Conversation
Move the OTEL trace, metric, and log provider setup block out of runServeCmd into initOTELProviders in a new cmd/fleet/otel.go. Adds three unit tests covering the disabled path, the enabled trace+meter path, and the log-export-enabled path. Behavior is preserved — runServeCmd calls the new function and the global otel.SetTracerProvider/SetMeterProvider side effects are unchanged. Part of fleetdm#33370.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Hello @getvictor - As per the discussion on the main issue, I raised this PR to extract the init of Otel provider. Can you take a look, please? Thanks! 🙏 🙇 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR refactors OpenTelemetry provider initialization in Fleet's server startup. A new Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🧹 Nitpick comments (1)
cmd/fleet/otel_test.go (1)
36-42: ⚡ Quick winAdd
t.Cleanupto shut down OTEL providers created incmd/fleet/otel_test.gotests. The tests callinitOTELProviders(...)but this file has no correspondingShutdown/cleanup, so shut down the returned non-nillp/tp/mpint.Cleanupto avoid leaks and cross-test state interference.🤖 Prompt for 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. In `@cmd/fleet/otel_test.go` around lines 36 - 42, Tests calling initOTELProviders in cmd/fleet/otel_test.go currently leak OTEL providers; add a t.Cleanup that shuts down any non-nil providers returned (lp, tp, mp) to avoid cross-test interference. In the Cleanup closure, for each of lp, tp, mp check for nil and call the appropriate shutdown method (e.g., Shutdown or Close) with a short context (context.Background() or context.WithTimeout) and ignore or log errors; ensure the cleanup is registered immediately after the call to initOTELProviders so providers are always torn down.
🤖 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 `@cmd/fleet/otel.go`:
- Around line 49-51: The initOTELProviders function calls initFatal on error but
then continues, risking nil OTEL components; after each initFatal(...)
invocation inside initOTELProviders (the calls that follow error checks when
creating the OTEL resource, exporters, and providers—and the one near the
shutdown/setup), add an immediate return to stop further execution when
initFatal is invoked so res/exporters/providers aren't used when unset;
alternatively, if you prefer a single pattern, make initFatal always terminate
(or have initOTELProviders check a boolean result from initFatal and return on
failure) so the function exits whenever initFatal is called.
---
Nitpick comments:
In `@cmd/fleet/otel_test.go`:
- Around line 36-42: Tests calling initOTELProviders in cmd/fleet/otel_test.go
currently leak OTEL providers; add a t.Cleanup that shuts down any non-nil
providers returned (lp, tp, mp) to avoid cross-test interference. In the Cleanup
closure, for each of lp, tp, mp check for nil and call the appropriate shutdown
method (e.g., Shutdown or Close) with a short context (context.Background() or
context.WithTimeout) and ignore or log errors; ensure the cleanup is registered
immediately after the call to initOTELProviders so providers are always torn
down.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d3d24bc2-a20d-40ae-9364-c9f42e6fb549
📒 Files selected for processing (3)
cmd/fleet/otel.gocmd/fleet/otel_test.gocmd/fleet/serve.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46421 +/- ##
==========================================
+ Coverage 66.87% 66.90% +0.02%
==========================================
Files 2791 2792 +1
Lines 222293 222299 +6
Branches 11334 11334
==========================================
+ Hits 148667 148730 +63
+ Misses 60170 60111 -59
- Partials 13456 13458 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add return nil, nil, nil after each initFatal call inside initOTELProviders so the function exits safely when the caller's initFatal does not terminate (the case in tests where it is swapped for a recorder). Adds a shutdownOTELProviders helper in otel_test.go that calls Shutdown on each non-nil provider via t.Cleanup, with a 100ms context timeout so cleanup stays fast even when no OTLP collector is listening. Per review feedback on fleetdm#46421.
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Looks good. Thank you for the contribution.
2 participants
Extracts the OTEL trace, metric, and log provider setup out of
runServeCmdand intoinitOTELProvidersin a newcmd/fleet/otel.go. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166). Side effects (otel.SetTracerProvider,otel.SetMeterProvider) are preserved inside the extracted function, so runtime behavior is identical.Three unit tests in
cmd/fleet/otel_test.go:(nil, nil, nil)and never callsinitFatal.One honest note on coverage: the four
initFatalsites inside the function are paranoid wrapping for OTEL SDK constructors that don't dial at construction time, so the error paths are hard to drive in tests without mocking the SDK. The tests above exercise the success paths and the disabled gate, which is the bulk of the realistic flow.This continues the path toward
serve.go>60% coverage per the discussion on #33370 —serve.gois now ~100 lines shorter and the OTEL phase is testable as a unit. Remaining slices per the broader plan: MDM Apple init, datastore init, Redis init.Related issue: Refs #33370
Checklist for submitter
Summary by CodeRabbit
Refactor
Tests