fix(dashboard-api): optimize getGlobalStats database connections concurrency#324
Open
Kirtan-pc wants to merge 1 commit into
Open
fix(dashboard-api): optimize getGlobalStats database connections concurrency#324Kirtan-pc wants to merge 1 commit into
Kirtan-pc wants to merge 1 commit into
Conversation
…urrency Fetch per-project user counts in parallel using Promise.all instead of sequentially to prevent linear slowdown with project counts.
Kirtan-pc
requested review from
Nitin-kumar-yadav1307 and
yash-pouranik
as code owners
Contributor
📝 WalkthroughWalkthrough
Analytics Controller: Parallel User Counting and Test Coverage
Sequence Diagram(s)sequenceDiagram
participant Client
participant getGlobalStats
participant Promise.all
participant getConnection
participant MongoDB
Client->>getGlobalStats: GET /api/analytics/global
getGlobalStats->>Promise.all: projects.map(async project => ...)
par Project A
Promise.all->>getConnection: getConnection(projectA._id)
getConnection-->>Promise.all: connA
Promise.all->>MongoDB: connA.collection('users').countDocuments()
MongoDB-->>Promise.all: countA
and Project B (fails)
Promise.all->>getConnection: getConnection(projectB._id)
getConnection-->>Promise.all: Error
Promise.all-->>Promise.all: catch → return 0
end
Promise.all-->>getGlobalStats: [countA, 0, ...]
getGlobalStats->>getGlobalStats: reduce → totalUsers
getGlobalStats-->>Client: res.json({ totalUsers, ... })
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/dashboard-api/src/__tests__/analytics.controller.test.js (1)
88-155: ⚡ Quick winThe “parallel” test currently allows a sequential regression.
Line 88-155 only checks call count and arguments; a
for...of awaitimplementation would still pass. Use a deferred firstgetConnectionpromise and assert the secondgetConnectionis called before the first resolves.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/dashboard-api/src/__tests__/analytics.controller.test.js` around lines 88 - 155, The test for the getGlobalStats method labeled 'should return aggregated global stats with user counts fetched in parallel' does not actually verify parallel execution because it only checks call counts and arguments. To properly test parallelism, create a deferred first promise for the initial mockGetConnection call that doesn't resolve immediately, setup the second mockGetConnection to track when it's called relative to the first promise's resolution, and add an assertion verifying that the second getConnection is invoked before the first promise resolves. This will ensure the implementation uses concurrent execution like Promise.all rather than sequential awaits like for...of await.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/dashboard-api/src/controllers/analytics.controller.js`:
- Around line 43-53: The code at lines 43-53 creates unbounded parallel database
connection tasks by mapping over all projects without concurrency limits, which
can cause connection spikes when there are many projects. Instead of directly
mapping all projects and using Promise.all on userCountPromises, implement a
bounded concurrency pattern such as a queue or semaphore that limits the maximum
number of concurrent getConnection and countDocuments operations. This ensures
only a small fixed number of projects are being processed in parallel at any
given time (e.g., 5-10 concurrent connections) while maintaining correct
aggregation of userCounts into totalUsers.
---
Nitpick comments:
In `@apps/dashboard-api/src/__tests__/analytics.controller.test.js`:
- Around line 88-155: The test for the getGlobalStats method labeled 'should
return aggregated global stats with user counts fetched in parallel' does not
actually verify parallel execution because it only checks call counts and
arguments. To properly test parallelism, create a deferred first promise for the
initial mockGetConnection call that doesn't resolve immediately, setup the
second mockGetConnection to track when it's called relative to the first
promise's resolution, and add an assertion verifying that the second
getConnection is invoked before the first promise resolves. This will ensure the
implementation uses concurrent execution like Promise.all rather than sequential
awaits like for...of await.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2de2de5-e419-41b1-bd63-09168990e616
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
apps/dashboard-api/src/__tests__/analytics.controller.test.jsapps/dashboard-api/src/controllers/analytics.controller.js
Comment on lines
+43
to
+53
| const userCountPromises = projects.map(async (project) => { | ||
| try { | ||
| const conn = await getConnection(project._id.toString()); | ||
| const userCount = await conn.collection('users').countDocuments(); | ||
| totalUsers += userCount; | ||
| return await conn.collection('users').countDocuments(); | ||
| } catch (err) { | ||
| console.error(`Failed to count users for project ${project._id}:`, err.message); | ||
| return 0; | ||
| } | ||
| } | ||
| }); | ||
| const userCounts = await Promise.all(userCountPromises); | ||
| const totalUsers = userCounts.reduce((sum, count) => sum + count, 0); |
Contributor
There was a problem hiding this comment.
Bound per-project DB counting concurrency to avoid connection spikes.
Line 43 fans out one getConnection + countDocuments task per project with no upper bound. With many projects, this can overload DB/socket resources and hurt endpoint stability. A small concurrency cap keeps the latency win while preventing burst pressure.
Proposed fix (bounded parallelism)
- const userCountPromises = projects.map(async (project) => {
- try {
- const conn = await getConnection(project._id.toString());
- return await conn.collection('users').countDocuments();
- } catch (err) {
- console.error(`Failed to count users for project ${project._id}:`, err.message);
- return 0;
- }
- });
- const userCounts = await Promise.all(userCountPromises);
- const totalUsers = userCounts.reduce((sum, count) => sum + count, 0);
+ const USER_COUNT_CONCURRENCY = 10;
+ let totalUsers = 0;
+
+ for (let i = 0; i < projects.length; i += USER_COUNT_CONCURRENCY) {
+ const batch = projects.slice(i, i + USER_COUNT_CONCURRENCY);
+ const batchCounts = await Promise.all(
+ batch.map(async (project) => {
+ try {
+ const conn = await getConnection(project._id.toString());
+ return await conn.collection("users").countDocuments();
+ } catch (err) {
+ console.error(`Failed to count users for project ${project._id}:`, err.message);
+ return 0;
+ }
+ })
+ );
+ totalUsers += batchCounts.reduce((sum, count) => sum + count, 0);
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/dashboard-api/src/controllers/analytics.controller.js` around lines 43 -
53, The code at lines 43-53 creates unbounded parallel database connection tasks
by mapping over all projects without concurrency limits, which can cause
connection spikes when there are many projects. Instead of directly mapping all
projects and using Promise.all on userCountPromises, implement a bounded
concurrency pattern such as a queue or semaphore that limits the maximum number
of concurrent getConnection and countDocuments operations. This ensures only a
small fixed number of projects are being processed in parallel at any given time
(e.g., 5-10 concurrent connections) while maintaining correct aggregation of
userCounts into totalUsers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Resolves #285 .
Previously,
getGlobalStatsinanalytics.controller.jsiterated over every developer project in a sequentialfor...ofloop, awaitinggetConnectionandcountDocumentsfor each project. Under real load with many projects, this caused a linear slowdown (O(N) latency) before returning statistics.This change parallelizes the connection and user count calls using
Promise.all(projects.map(...)), reducing the latency to O(1) matching the slowest single connection. It retains project-level error trapping so that a failure in one project's database connection does not break the entire endpoint.🛠️ Type of Change
🧪 Testing & Validation
Backend Verification:
apps/dashboard-apiand all tests passed.apps/dashboard-api/src/__tests__/analytics.controller.test.js.Frontend Verification:
npm run lintin thefrontend/directory.📸 Screenshots / Recordings (Optional)
✅ Checklist
Built with ❤️ for urBackend.
Summary by CodeRabbit
Tests
Refactor