-
Notifications
You must be signed in to change notification settings - Fork 248
fix(metrics): proper count of pending blobs metrics #3038
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 34s —— View job Code Review CompleteI've reviewed the pending blobs metrics refactoring. This is a solid fix that corrects a significant bug in the metric counting logic while also simplifying the codebase. SummaryProblem Fixed: The previous implementation had a critical flaw where Solution: The metric update logic was moved from the Impact:
Code Quality & Architecture ✅Strengths:
Observations:
Error Handling & Resilience ✅No changes to error handling in this PR. The existing error handling patterns remain sound:
Security ✅No security implications from this change. The refactoring is purely internal to metrics reporting. Performance & Resource Efficiency ✅Performance Improvements:
Minor Consideration: Testing & Reliability ✅Test Updates:
Test Coverage: According to Codecov, all modified lines are covered by tests, and overall coverage remains stable at 55.33%. Suggestions: func TestDASubmissionLoop_UpdatesPendingBlobsMetric(t *testing.T) {
// Test that the metric correctly sums headers + data pending counts
// and updates on each ticker cycle
}Documentation & Developer Experience ✅CHANGELOG.md: Properly documented with link to PR. Code Comments: The existing comments remain clear. The comment at // Update metrics with current pending counts
s.metrics.DASubmitterPendingBlobs.Set(float64(headersNb + dataNb))Minor Enhancement: Consider updating the comment to be more explicit about why this is the correct place: // Update metrics with current pending counts (sum of headers + data)
// This is calculated here to ensure accurate total backlog reporting
s.metrics.DASubmitterPendingBlobs.Set(float64(headersNb + dataNb))Gemini's Suggestion - Granular Metrics 💡Gemini Code Assist suggested adding separate metrics for s.metrics.PendingHeadersCount.Set(float64(headersNb))
s.metrics.PendingDataCount.Set(float64(dataNb))
s.metrics.DASubmitterPendingBlobs.Set(float64(headersNb + dataNb))Assessment: This is a reasonable suggestion for enhanced observability. The values are already calculated, so adding these metrics has minimal overhead. However: Pros:
Cons:
Recommendation: This could be a nice follow-up enhancement if granular monitoring is valuable for your use case, but it's not required for this fix to be correct. I'd suggest opening a separate issue to track this as a potential improvement. Critical Issues 🔴None identified. This is a clean, well-executed fix. Minor Issues / Suggestions 🟡
Conclusion ✅This PR successfully fixes a significant bug in the Recommendation: APPROVE ✅ The PR is ready to merge as-is. The optional enhancements (granular metrics, test coverage for metric behavior) can be addressed in follow-up work if desired. Task List:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3038 +/- ##
==========================================
+ Coverage 55.33% 55.43% +0.09%
==========================================
Files 117 117
Lines 11685 11685
==========================================
+ Hits 6466 6477 +11
+ Misses 4494 4485 -9
+ 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.