fix(metrics): proper count of pending blobs metrics #3038
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Summary of ChangesHello @julienrbrt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the mechanism for tracking and reporting pending blobs metrics. It centralizes the calculation and update of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Claude finished @julienrbrt's task in 2m 10s —— View job Code Review CompleteI've performed a comprehensive review of this PR. Here's my detailed feedback: SummaryThis PR refactors the metrics tracking for pending DA blobs by moving the Overall Assessment: ✅ Approved with Minor ObservationsThe refactoring is correct and improves the code quality. The change ensures more consistent and accurate metrics reporting. Detailed Review1. Code Quality & Architecture ✅Strengths:
Observations:
2. Correctness & Concurrency ✅Race Condition Analysis:
Timing Analysis: // submitter.go:185-311
case <-ticker.C:
headersNb := s.cache.NumPendingHeaders() // Line 187
// ... header submission logic ...
dataNb := s.cache.NumPendingData() // Line 251
// ... data submission logic ...
s.metrics.DASubmitterPendingBlobs.Set(float64(headersNb + dataNb)) // Line 311The values are captured at the start of the ticker cycle, so the metric reflects the state at that moment. This is acceptable because:
Potential Issue - Stale Reads: Severity: Low - This is acceptable for metrics purposes and much better than the previous implementation which had similar timing issues plus additional complexity. 3. Performance & Resource Efficiency ✅Improvements:
No Performance Regressions: The removal of metric updates from retry paths is actually beneficial - it reduces unnecessary work during backpressure scenarios. 4. Error Handling & Resilience ✅No changes to error handling logic. The refactoring is purely about metrics placement, not error paths. 5. Testing & Reliability ✅Test Updates:
Test Coverage:
Missing Test Coverage:
6. Documentation & Code Clarity ✅CHANGELOG Entry: - Proper counting of `DASubmitterPendingBlobs` metrics. [#3038]Code Comments: // Update metrics with current pending counts
s.metrics.DASubmitterPendingBlobs.Set(float64(headersNb + dataNb))This clearly documents the intent. Suggestion for Future Enhancement: // Update metrics with current pending counts.
// We update here (in the ticker loop) rather than in submitToDA to ensure
// a consistent snapshot of total backlog across both headers and data.7. Security ✅No security implications. This is a pure refactoring of metrics collection. Additional ObservationsMock Cleanup ✅The removal of unused Metric SemanticsThe metric The new implementation correctly reports Recommendations1. Consider Adding Debug Logging (Optional)To aid in troubleshooting metric discrepancies, consider adding debug logging: // Update metrics with current pending counts
totalPending := headersNb + dataNb
s.logger.Debug().
Uint64("pending_headers", headersNb).
Uint64("pending_data", dataNb).
Uint64("total_pending_blobs", totalPending).
Msg("updating pending blobs metric")
s.metrics.DASubmitterPendingBlobs.Set(float64(totalPending))This would help operators correlate metric values with system behavior. 2. CI/CD ValidationEnsure that:
Based on the PR checks, Buf validation has passed. No blockers identified. ConclusionThis is a clean, well-executed refactoring that improves code quality and metric accuracy. The change:
No blocking issues found. The code is ready to merge. Great work on this cleanup! The centralized metric approach is more maintainable and accurate than the previous scattered updates. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly refactors the logic for counting pending blobs for metrics. The previous implementation was flawed as it would overwrite the metric with either the pending header or data count, instead of their sum. The new logic moves the metric update to the daSubmissionLoop, where it is periodically and correctly updated with the total number of pending items. This is a solid fix. I've also suggested a small improvement to update two other related metrics for more granular monitoring.
| } | ||
|
|
||
| // Update metrics with current pending counts | ||
| s.metrics.DASubmitterPendingBlobs.Set(float64(headersNb + dataNb)) |
There was a problem hiding this comment.
Since you're already calculating headersNb and dataNb, it would be beneficial to also update the PendingHeadersCount and PendingDataCount metrics here. This would provide more granular insight into the pending items.
s.metrics.PendingHeadersCount.Set(float64(headersNb))
s.metrics.PendingDataCount.Set(float64(dataNb))
s.metrics.DASubmitterPendingBlobs.Set(float64(headersNb + dataNb))
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3038 +/- ##
==========================================
- Coverage 55.33% 55.33% -0.01%
==========================================
Files 117 117
Lines 11685 11676 -9
==========================================
- Hits 6466 6461 -5
+ Misses 4494 4492 -2
+ Partials 725 723 -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:
|
Overview
Proper count of pending blobs metrics.