-
Notifications
You must be signed in to change notification settings - Fork 148
fix: fix active req caculation, add hws metrics #4053
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
base: 01-22-do_not_merge_add_metrics_exporter_for_ee
Are you sure you want to change the base?
fix: fix active req caculation, add hws metrics #4053
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Pull Request Review: Fix active request calculation and add HWS metricsOverviewThis PR fixes a critical bug in active request tracking and adds new metrics for hibernating WebSocket connections. 🐛 Critical Bug FixLine 67 in engine/packages/pegboard/src/workflows/actor/metrics.rs: -let mut destroy = true;
+let mut destroy = false;Impact: The previous code incorrectly initialized destroy = true, causing the metrics workflow to terminate prematurely. The fix ensures the workflow continues until explicitly destroyed. ✅ Good catch! This bug would have prevented proper metrics collection for long-running actors. 📊 New Feature: Hibernating WebSocket MetricsAdded tracking for hibernating WebSocket (HWS) connections:
✅ Well-structured - Follows the same pattern as HTTP and regular WebSocket metrics. 🔧 Metrics Recording ImprovementsParallel Execution: Changed metrics recording from sequential to parallel using tokio::join! Benefits:
Suggestion: Consider implementing:
🔄 Gasoline Event System ChangesVersionCheckEvent Enhancement: Changed from empty marker to data-carrying event with inner_version field. ✅ Good: Provides better debugging info and follows the pattern of other events. Backward Compatibility: Falls back to event version for old events that lack inner_version. ✅ Excellent: Maintains compatibility with existing data. 🗑️ Configuration CleanupRemoved from engine/artifacts/config-schema.json:
❓ Question: Confirm these are unused and not referenced elsewhere in the codebase. 📝 Minor Changes✅ Unused parameter prefixed with underscore (engine/packages/gasoline/src/db/kv/mod.rs:624) 🧪 Testing Considerations
Recommended tests:
🔒 Security Considerations✅ No security concerns. Changes are internal metrics tracking only. ⚡ Performance Considerations✅ Improved: Parallel metrics recording reduces request latency 📋 SummaryStrengths:
Areas for Improvement:
Recommendations:
✅ Approval StatusApprove with suggestions - The critical bug fix is correct and new features are well-implemented. Adding test coverage (especially for the destroy flag fix) would significantly improve confidence. |
0a34c06 to
e4b540a
Compare

No description provided.