feat(manifest): observability metrics for path resolution - #835
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change adds Prometheus metrics for manifest resolution outcomes, unresolved root requests, and resolution duration. Data handlers record metrics for index-based and manifest-parsing resolution paths. Tests cover labels, counters, histograms, and unresolved cases. ChangesManifest resolution metrics
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #835 +/- ##
===========================================
+ Coverage 79.43% 79.46% +0.02%
===========================================
Files 138 138
Lines 53401 53439 +38
Branches 4087 4087
===========================================
+ Hits 42420 42465 +45
+ Misses 10928 10921 -7
Partials 53 53 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/routes/data/handlers.ts (1)
1384-1414: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse TSDoc for the exported metrics recorder.
Convert the line-comment block into a TSDoc block for
recordManifestResolutionMetrics. Document the input fields and root-path behavior.As per coding guidelines, “Add or improve TSDoc comments on code you touch.”
🤖 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 `@src/routes/data/handlers.ts` around lines 1384 - 1414, Replace the line-comment block above recordManifestResolutionMetrics with a TSDoc comment. Document the function’s purpose, the source, resolution, manifestPath, and durationMs inputs, and clarify that unresolved root paths increment the corresponding metric.Source: Coding guidelines
src/routes/data/manifest-resolution-metrics.test.ts (1)
14-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the new test helpers.
Add TSDoc comments for
counterValue,histogramCount, andresolution. State the expected metric shape and label behavior.As per coding guidelines, “Add or improve TSDoc comments on code you touch.”
🤖 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 `@src/routes/data/manifest-resolution-metrics.test.ts` around lines 14 - 49, Add TSDoc comments to the counterValue, histogramCount, and resolution helpers. Document the expected metric data shape, how label filters are matched, the fallback value when no metric matches, and that resolution creates a default ManifestResolution with optional field overrides.Source: Coding guidelines
src/metrics.ts (1)
1728-1732: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep operational metric documentation in
help.Remove the comments that duplicate each metric’s operational behavior. Keep only code-local context that the
helpfield cannot express. Based on learnings, Prometheus metrics insrc/metrics.tsmust use theirhelpfield as operational documentation and must not duplicate that purpose in comments or TSDoc.Also applies to: 1739-1748
🤖 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 `@src/metrics.ts` around lines 1728 - 1732, Remove the operational behavior comments adjacent to the manifest path resolution metrics, including the related block around the additional referenced lines, while preserving any code-local context that is not expressible through the metrics’ help fields. Keep the metric definitions and their help values unchanged.Source: Learnings
🤖 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.
Nitpick comments:
In `@src/metrics.ts`:
- Around line 1728-1732: Remove the operational behavior comments adjacent to
the manifest path resolution metrics, including the related block around the
additional referenced lines, while preserving any code-local context that is not
expressible through the metrics’ help fields. Keep the metric definitions and
their help values unchanged.
In `@src/routes/data/handlers.ts`:
- Around line 1384-1414: Replace the line-comment block above
recordManifestResolutionMetrics with a TSDoc comment. Document the function’s
purpose, the source, resolution, manifestPath, and durationMs inputs, and
clarify that unresolved root paths increment the corresponding metric.
In `@src/routes/data/manifest-resolution-metrics.test.ts`:
- Around line 14-49: Add TSDoc comments to the counterValue, histogramCount, and
resolution helpers. Document the expected metric data shape, how label filters
are matched, the fallback value when no metric matches, and that resolution
creates a default ManifestResolution with optional field overrides.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b056a44-295d-478a-b234-2eef086f9834
📒 Files selected for processing (3)
src/metrics.tssrc/routes/data/handlers.tssrc/routes/data/manifest-resolution-metrics.test.ts
Addresses CodeRabbit review on #835: - Convert the metric-definition comments to `/** */` block form, matching the existing convention in metrics.ts (e.g. the optimistic-tx counters). The operational context (index-vs-data effectiveness signal, malformed-manifest signal) is retained — it is not expressible via the `help` fields alone. - Add TSDoc to recordManifestResolutionMetrics documenting its inputs. - Add TSDoc to the counterValue / histogramCount / resolution test helpers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LPbcF2M3P1XcM8mU8FWEf9
|
Thanks @coderabbitai — addressed in fac809f: 1. 2. 3. Test helpers — added TSDoc to Lint clean, metrics tests 4/4. |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
Instruments the two manifest resolution sites in the data handler:
- `manifest_resolutions_total{source,resolution_type}` — counts resolutions
split by source (`index` = served from the persistent/cached index without
parsing the body, vs `data` = on-demand body parse) and how the path
resolved (path / index / fallback / unresolved). The index-vs-data ratio is
the effectiveness signal for the resolution index and cache.
- `manifest_unresolved_root_total` — root/index requests that resolved to
nothing, i.e. a malformed-manifest signal that today only surfaces as a user
404.
- `manifest_resolution_duration_seconds{source}` — resolution latency by
source.
`index` is counted only when the index actually determined the outcome
(complete); misses fall through and are counted at the data-parse site.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LPbcF2M3P1XcM8mU8FWEf9
Addresses CodeRabbit review on #835: - Convert the metric-definition comments to `/** */` block form, matching the existing convention in metrics.ts (e.g. the optimistic-tx counters). The operational context (index-vs-data effectiveness signal, malformed-manifest signal) is retained — it is not expressible via the `help` fields alone. - Add TSDoc to recordManifestResolutionMetrics documenting its inputs. - Add TSDoc to the counterValue / histogramCount / resolution test helpers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LPbcF2M3P1XcM8mU8FWEf9
fac809f to
407d9ea
Compare
1 participant
Adds the observability layer (1e) for manifest path resolution. Independent of the resolution PR (#833) — touches only
metrics.tsand the data handler, no overlap — and is meaningful now (data-path metrics) and richer once #833 lands (index-path metrics start firing).Metrics
manifest_resolutions_total{source,resolution_type}— resolutions bysource(index= served from the persistent/cached index without parsing the body, vsdata= on-demand body parse) andresolution_type(path/index/fallback/unresolved). The index-vs-data ratio is the effectiveness signal for the resolution index + cache.manifest_unresolved_root_total— root/index requests that resolved to nothing (a malformed-manifest signal that today only surfaces as a user 404).manifest_resolution_duration_seconds{source}— resolution latency by source (bounded lookup vs body parse).Placement
Instruments the two existing resolution sites in
src/routes/data/handlers.ts.indexis counted only when the index actually determined the outcome (complete); a miss falls through and is counted at the data-parse site, so each resolution is counted once.Tests
manifest-resolution-metrics.test.ts(new, 4): verifies label routing (source × resolution_type), duration observation, and that the unresolved-root counter fires for an unresolved root but not for an unresolved sub-path. Handler suite unaffected (96/96); lint clean; zero new type errors.Notes
No new env var and no metrics doc exists in
docs/, so nothing to sync there.🤖 Generated with Claude Code
https://claude.ai/code/session_01LPbcF2M3P1XcM8mU8FWEf9