Fix for the data shown in network tab. #2851
Conversation
Owaiseimdad
requested review from
ajhollid,
mohicody,
Br0wnHammer and
karenvicent
as code owners
WalkthroughAdjusted the denominator in five rate calculations within the hourlyStats facet of buildHardwareDetailsPipeline in server/src/db/mongo/modules/monitorModuleQueries.js from dividing by 1000 to dividing by 1, effectively scaling the computed rates by 1/1000. No other parts of the pipeline were changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes P.S. Canadians divide by 1 to keep things simple; Americans ask why it isn’t divided by freedom units. Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/db/mongo/modules/monitorModuleQueries.js (1)
656-657: Align time‐base units across all delta metricsRight now, three of our “delta” fields are calculating rates in seconds and the other five in milliseconds, which will skew any combined charts or totals. Let’s pick one unit—either per-ms (divide by
(tLast – tFirst)/1) or per-sec (divide by(tLast – tFirst)/1000)—and apply it uniformly.Affected metrics (currently dividing by
(tLast - tFirst)/1000):
- server/src/db/mongo/modules/monitorModuleQueries.js:656–657
deltaErrOut- server/src/db/mongo/modules/monitorModuleQueries.js:711–712
deltaDropIn- server/src/db/mongo/modules/monitorModuleQueries.js:766–767
deltaDropOutThe other five delta metrics above these lines are still using
(tLast – tFirst)/1.Action items:
- If we standardize on per-millisecond rates: update the three listed to divide by 1 (i.e.
(tLast – tFirst)/1) and adjust any UI labels to “per ms.”- If we standardize on per-second rates: revert the other five metrics to divide by 1000 instead of 1 and label them “per s.”
Let’s get all eight speaking the same language—no more apples vs. maple syrup math! 🍁🇨🇦🏒🇺🇸
🧹 Nitpick comments (3)
server/src/db/mongo/modules/monitorModuleQueries.js (3)
400-439: Prefer $dateDiff for clarity and to avoid unit mistakesTo make intent explicit and prevent future regressions, compute elapsed time with $dateDiff and a single conversion point. Example for bytesSentPerSecond:
- $divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1000] }], + $let: { + vars: { + // Milliseconds between tFirst and tLast; preserves sub-second precision + dtMs: { $dateDiff: { startDate: "$$tFirst", endDate: "$$tLast", unit: "millisecond" } } + }, + in: { + $cond: [ + { $gt: ["$$dtMs", 0] }, + { $divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: ["$$dtMs", 1000] }] }, + 0 + ] + } + }Do the same for the other seven metrics. It reads better, is less error-prone, and makes the “per second” intent unambiguous.
432-433: Field naming vs. unitsThe field is named bytesSentPerSecond. If you intentionally switch to per‑millisecond, either:
- keep per‑second values and the current name, or
- rename to bytesSentPerMillisecond and update client-side labels/units.
Otherwise you’ll confuse users more than a border agent asking an American to convert Fahrenheit to Celsius on the spot.
171-793: Integration check: ensure UI labels/legends still match the unitGiven the PR description (“network tab data are already in seconds”), confirm the client expects per‑second for these series. If the backend flips to per‑ms while the UI still shows “B/s”, end users will read values that look 1000× lower.
Happy to scan the client for legends/formatters and propose a coordinated change if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
server/src/db/mongo/modules/monitorModuleQueries.js(5 hunks)
🔇 Additional comments (2)
server/src/db/mongo/modules/monitorModuleQueries.js (2)
425-437: Double-check theupdatedAtfield typeI couldn’t locate any Mongoose schema or explicit
timestampssettings forupdatedAtin the codebase, so it’s safest to manually verify:
- Inspect the schema definition for your “checks”/“monitor” models to see if
timestamps: trueis enabled (Mongoose will then use Date objects in milliseconds).- Search any DB migration or model-registration files for custom
updatedAthandling or default values.- Review any UI/consumer code to confirm whether
updatedAtvalues are treated as epoch seconds or JavaScript Date objects.Once you confirm whether
updatedAtis a Date (ms since epoch) or a numeric epoch in seconds, you can either remove the/1000division and switch to$dateDiff, or leave the division in place—just ensure all similar aggregations stay consistent.And hey, if it really is in seconds, you can divide all you want—just don’t ask your Canadian neighbors for snow removal tips, eh? 🍁
432-433: I've added scripts to verify the denominators in all five metric calculations. The results will confirm whether they’re mixing/1(ms) and/1000(s). Once we have the output, we can proceed with the correct recommendation.
| $divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1] }], | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Patch-ready diff (revert the five denominators back to seconds)
If updatedAt is a Date (milliseconds), apply this diff to fix all five changed spots:
@@
- $divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1] }],
+ $divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1000] }],
@@
- $divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1] }],
+ $divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1000] }],
@@
- $divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1] }],
+ $divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1000] }],
@@
- $divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1] }],
+ $divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1000] }],
@@
- $divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1] }],
+ $divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1000] }],Also applies to: 471-472, 510-511, 565-566, 601-602
🤖 Prompt for AI Agents
In server/src/db/mongo/modules/monitorModuleQueries.js around lines 432-433 (and
also update the same pattern at 471-472, 510-511, 565-566, 601-602), the
aggregation is dividing the millisecond timestamp difference by 1 which treats
updatedAt as seconds; change the denominator back to 1000 so the expression
becomes { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1000] } (apply this
exact replacement at all five locations).
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
Auto Pull Request Review from LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR addresses a unit conversion issue but introduces a critical inconsistency by not updating all network metrics, risking misleading data visualizations.
🚨 Critical Issues (P1)
- server/src/db/mongo/modules/monitorModuleQueries.js: Introduces inconsistent units across network metrics, potentially causing misleading visualizations where some metrics are in bytes/second and others remain in bytes/millisecond.
💡 Suggestions (P2)
- server/src/db/mongo/modules/monitorModuleQueries.js: Includes redundant division by 1 operations that add cognitive load without value, simplifying to direct subtraction would improve maintainability.
💡 LlamaPReview Community
Have feedback on this AI Code review tool? Join our GitHub Discussions to share your thoughts and help shape the future of LlamaPReview.
| { $gt: [{ $subtract: ["$$tLast", "$$tFirst"] }, 0] }, | ||
| { | ||
| $divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1000] }], | ||
| $divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1] }], |
There was a problem hiding this comment.
P2 | Confidence: High
Division by 1 is a redundant operation that adds cognitive load without value. The expression { $divide: [X, 1] } is equivalent to X. Removing this unnecessary operation would simplify the aggregation pipeline and improve readability without changing behavior.
| $divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1] }], | |
| $divide: [{ $subtract: ["$$last", "$$first"] }, { $subtract: ["$$tLast", "$$tFirst"] }] |
| }, | ||
| }, | ||
| in: { | ||
| $cond: [ |
There was a problem hiding this comment.
[Contextual Comment]
This comment refers to code near real line 428. Anchored to nearest_changed(429) line 429.
P1 | Confidence: High
The change replaces a division by 1000 with division by 1 in network metric calculations. While the PR description states the data is already in seconds, this change fundamentally alters the rate calculation from bytes-per-millisecond to bytes-per-second. However, there's a critical inconsistency: the change was applied to only 5 of 6 similar metric calculations (deltaBytesSent, deltaBytesRecv, deltaPacketsSent, deltaPacketsRecv, deltaErrIn), but deltaErrOut remains unchanged. This creates inconsistent units across related metrics, potentially causing misleading visualizations where 5 metrics use bytes/second while deltaErrOut uses bytes/millisecond (1000x larger). The frontend likely expects consistent units across all network metrics.
Code Suggestion:
$divide: [{ $subtract: ["$$last", "$$first"] }, { $divide: [{ $subtract: ["$$tLast", "$$tFirst"] }, 1] }],
There was a problem hiding this comment.
I don't believe this is correct, time stamps give differences in miliseconds, not seconds.
What leads you to conclude the time difference is in seconds?
More likely is that data returned from Capture is not consistent, perhaps returning bytes in some cases and kilobytes in others.
|
I want to share my setup If the values are inconsistent on capture side, please open an issue on Capture repo so I can deep dive into the problem and fix it if possible. |
|
@mertssmnoglu values are correct for me too, also Linux x86. Does anyone have MacOS they can test and confirm on? So far it seems only MacOS has data off by a factor of 1000. |
I can do on Mac guys (regardless of the fact that it's rare to deploy Capture on a Mac lol) |
|
@gorkem-bwl sure if you can test and verify that network stats are accurate that would be really great. I have thus far not been able to replicate the issue 🤔 |
Describe your changes
The data for network tab are already in seconds.
I validated with respect to apple's activity monitor and network tab of checkmate
Write your issue number after "Fixes "
#2814
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
<div>Add</div>, use):npm run formatin server and client directories, which automatically formats your code.Summary by CodeRabbit