-
Notifications
You must be signed in to change notification settings - Fork 149
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: main
Are you sure you want to change the base?
fix: fix active req caculation, add hws metrics #4053
Conversation
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. |
PR Review: Fix active request calculation and add hibernating websocket metricsCritical IssuesCompilation Error: Missing Function DefinitionLocation: engine/packages/pegboard-gateway/src/lib.rs:292 The function init_ns_metrics_exporter is called but never defined. This will cause a compilation failure. Major IssuesInconsistent Error Handling PatternLocation: engine/packages/pegboard-gateway/src/lib.rs:619-700 The error handling for metrics has nested if-else blocks that could be flattened for better readability. When ingress metrics fail, the error is logged. When egress metrics fail, there is a nested condition that makes the code harder to follow. Recommendation: Flatten the error handling so both metrics operations are independent. Potential Data Accuracy IssueLocation: engine/packages/pegboard-gateway/src/lib.rs:624-632 Using size_hint() for request body size might not be accurate. The original code used body_bytes.len() which was accurate. This change means metrics might now be less accurate than before. Code Quality IssuesCritical Bug Fix Without ExplanationLocation: engine/packages/pegboard/src/workflows/actor/metrics.rs:67 Changed destroy from true to false. This is a critical logic change that fixes what appears to be a significant bug (actors were being incorrectly marked for destruction). This deserves better documentation and context in the PR description. Unused Parameter WarningLocation: engine/packages/gasoline/src/db/kv/mod.rs:624 The worker_id parameter is prefixed with underscore but the change is not explained. Test CoverageNo tests are included. This PR makes significant changes to metrics tracking logic. Consider adding unit tests for hibernating websocket metrics and integration tests verifying the destroy fix. SummaryMust Fix:
Should Fix:
Consider:
|
0a34c06 to
e4b540a
Compare
22c21f0 to
399c5f7
Compare
e4b540a to
93864b6
Compare

No description provided.