docs(instrument): describe provider-aware metric caches - #4178
Conversation
|
@coderabbitai full_review, thanks! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughUpdates NICo observability documentation for process-global meter-provider installation, provider replacement behavior, cached Event instruments, and steady-state metric emission costs. ChangesObservability instrumentation documentation
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🐇✅ ✅ Action performedFull review finished. |
![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 `@docs/observability/instrumentation.md`:
- Around line 109-118: Add a concise staged-rollout warning to the provider
installation and replacement guidance, instructing operators to validate
experimental NICo behavior and production configurations in non-critical
environments before deploying to critical production systems. Keep the existing
provider behavior description unchanged.
🪄 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: Enterprise
Run ID: 950fa530-5814-47cc-8a3a-6521ca3ed14f
📒 Files selected for processing (1)
docs/observability/instrumentation.md
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/observability/instrumentation.md`:
- Around line 319-321: Update the steady-state cost description near the Event
instrumentation discussion to avoid claiming only two lock-free loads and an
add; describe the path as a cached instrument lookup, label-array
materialization, and counter update.
- Around line 109-118: Update the provider-installation guidance around
carbide_instrument::set_meter_provider to retain the startup recommendation and
initial pre-install observation behavior, but remove claims that cached Event or
RED instruments follow later provider replacements or that overlapping
observations may reach either provider. State that these instruments remain
cached through provider swaps and only use the provider obtained when
initialized.
🪄 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: Enterprise
Run ID: f6d48ea3-3ae1-4151-8324-5a3c48dd172b
📒 Files selected for processing (1)
docs/observability/instrumentation.md
The Event guide still said each metric handle bound to the first global provider for the process lifetime, while NVIDIA#4176 makes those caches follow provider installation and replacement. Update the quick start to name the framework setter, explain early observations and the replacement boundary, and revise the steady-path cost. This is the documentation half of NVIDIA#4176. No tests added; docs-only change. This supports NVIDIA#4177 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
🐇✅ ✅ Action performedFull review finished. |
|
Closing this companion with #4176. It documents the generation-aware replacement contract from that code PR, so merging it on its own would describe behavior we are not landing. If we return to #4174 with the smaller one-shot readiness guard, we can document the contract that actually gets implemented then. Thanks! |
1 participant
The Event guide still says each metric handle binds to the first global provider for the process lifetime, while #4176 makes those caches follow provider installation and replacement.
This companion updates the quick start to name the framework setter, explains how early observations and provider replacement behave, and revises the steady-path cost. It also makes the wrapper requirement explicit so callers do not bypass the cache-generation update with OpenTelemetry's global setter.
Related issues
This supports #4177
Parent code issue: #4174
Type of Change
Breaking Changes
Testing
Validation:
git diff --checkAdditional Notes
This PR is the documentation half of #4176 and must merge after that code PR. The generated
docs/observability/core_metrics.mdcatalogue is intentionally unchanged because the code PR does not add or rename a metric.