Gate console.time/timeEnd performance logs behind DEBUG flag in data …#71
Conversation
|
@ayash911 is attempting to deploy a commit to the Yash Pouranik's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaced unguarded console.time()/console.timeEnd() in data controller with perf_hooks performance timing guarded by an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces conditional performance timing in the insertData and getAllData controllers based on the DEBUG environment variable. The review feedback points out a concurrency bug where static labels in console.time can cause inaccurate results in an asynchronous environment and suggests caching the environment variable lookup to improve performance.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/public-api/src/controllers/data.controller.js (1)
15-15: Implementation is correct; consider extracting debug check for DRY.The gating works as intended. However, the
process.env.DEBUG === 'true'check is repeated multiple times across the file. Consider extracting to a constant or helper at the top of the file for maintainability.♻️ Optional: Extract debug flag to constant
const { validateData, validateUpdateData } = require("@urbackend/common"); +const isDebug = process.env.DEBUG === 'true'; + // Validate MongoDB ObjectId const isValidId = (id) => mongoose.Types.ObjectId.isValid(id);Then use it consistently:
- if (process.env.DEBUG === 'true') console.time("insert data") + if (isDebug) console.time("insert data")Also applies to: 51-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public-api/src/controllers/data.controller.js` at line 15, Extract the repeated process.env.DEBUG === 'true' check into a single top-level constant (e.g., IS_DEBUG) and replace all inline checks (used around console.time("insert data") and console.timeEnd and any other occurrences in this file) with that constant; update the insert data timing blocks and any other debug gating to reference IS_DEBUG so the check is DRY and easy to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/public-api/src/controllers/data.controller.js`:
- Line 15: Extract the repeated process.env.DEBUG === 'true' check into a single
top-level constant (e.g., IS_DEBUG) and replace all inline checks (used around
console.time("insert data") and console.timeEnd and any other occurrences in
this file) with that constant; update the insert data timing blocks and any
other debug gating to reference IS_DEBUG so the check is DRY and easy to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a647a31-dcde-47b4-8f6d-05a42bee37fd
📒 Files selected for processing (1)
apps/public-api/src/controllers/data.controller.js
|
@coderabbitai ready to merge? or still any issues |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 35979
✅ What's good:
The concurrency issue ( Would you like me to open an issue to track the concurrency concern, or address it directly in this PR? |
|
is it a good practice to use console.time in production lvl apps like this @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes.
❌ Why
|
| Use Case | Recommended Tool |
|---|---|
| Quick local debug timing | performance.now() (more accurate than console.time) |
| High-precision benchmarking | process.hrtime.bigint() |
| Production observability | A structured logger like pino or winston with log levels |
| Full APM/tracing | Datadog, New Relic, OpenTelemetry |
In Node.js programs, prioritize process.hrtime, followed by performance.now, and lastly Date.now — based on considerations of precision and clock synchronization.
📌 Verdict on this PR
The PR's approach of gating console.time behind DEBUG === 'true' is a reasonable pragmatic fix for the immediate noise issue (as required by issue #69), but for a proper production-grade API, the long-term recommendation would be to replace these timers entirely with a structured logger that supports log levels (e.g., logger.debug(...)) — so performance tracing integrates cleanly with your log aggregation pipeline rather than printing raw strings to stdout.
Would you like me to open a follow-up issue to track replacing console.time with a structured logger?
|
look @coderabbitai I think we should merge this PR and then create a new issue to replace them with real world tools? what say? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
Let me create that follow-up issue now! 🏁 Script executed: Length of output: 52
It covers:
This PR ( |
|
@ayash911 is there anything u wanna update or may i manually review an then merge? |
Fixes concurrency bug and potential memory leaks by replacing globally-scoped console.time with performance.now, using the module level isDebug constant.
|
@yash-pouranik i have refactoed the code please review |
|
@ayash911 okay |
|
@ayash911 have you tested locally?? |
| const { getCompiledModel } = require("@urbackend/common"); | ||
| const {QueryEngine} = require("@urbackend/common"); | ||
| const { validateData, validateUpdateData } = require("@urbackend/common"); | ||
| const { performance } = require('perf_hooks'); |
There was a problem hiding this comment.
You havent added this package to dependencies?? It seems like you are just adding the code, not actually running?
|
@coderabbitai does this also solve #73 ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5211
Bottom line: The refactored PR solves the So the flow would be:
|
|
@ayash911 please update dependency, this is the last thing remaining, i resolved the conflict , then will merge this |
ayash911
force-pushed
the
feature/gate-debug-timers
branch
from
332ce98 to
c94e9cc
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/public-api/package.json (1)
17-17: Preferworkspace:*for local monorepo dependencies instead of wildcard.At Line 17,
"@urbackend/common": "*"should use the workspace protocol since@urbackend/commonis a local package in this monorepo. This ensures reproducible installs and explicit workspace linkage.Proposed change
- "@urbackend/common": "*", + "@urbackend/common": "workspace:*",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public-api/package.json` at line 17, The dependency entry "@urbackend/common": "*" in package.json should use the workspace protocol for local monorepo linkage; replace the wildcard version with "workspace:*" for the "@urbackend/common" dependency in apps/public-api's package.json so installs use the workspace protocol and preserve explicit local linking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/public-api/package.json`:
- Line 17: The dependency entry "@urbackend/common": "*" in package.json should
use the workspace protocol for local monorepo linkage; replace the wildcard
version with "workspace:*" for the "@urbackend/common" dependency in
apps/public-api's package.json so installs use the workspace protocol and
preserve explicit local linking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44c6211f-032e-4d76-80d1-35e03e5bc341
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
apps/public-api/package.jsonapps/public-api/src/controllers/data.controller.js
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/public-api/src/controllers/data.controller.js
|
@ayash911 I told you that i resolved merge conflicts still, so you should have to pull new chnages and then do required updates, but you again did yourself and force pushed?? seriously? Like the data.controller.js file now has too many errors I can see, check the copmare button just after you commit, It shows you have removed imp piece of work in this file - data.controller.js |
|
@coderabbitai feels like this file data.controller.js has lost some code? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8882 🏁 Script executed: Length of output: 7474
The PR branch actually has more lines than const { performance } = require('perf_hooks');
const isDebug = process.env.DEBUG === 'true';
// in insertData:
let start;
if (isDebug) start = performance.now();
// ...
if (isDebug) console.log(`[DEBUG] insert data took ${(performance.now() - start).toFixed(2)}ms`);
// in getAllData:
let start;
if (isDebug) start = performance.now();
// ...
if (isDebug) console.log(`[DEBUG] getall took ${(performance.now() - start).toFixed(2)}ms`);The file is complete and correct. The PR is safe to merge! 🚀 |
|
okay lets merge, Thank you @ayash911 for the PR and continuously updating according to the reviews. |
|
Dont forget to Star the REPO @ayash911 |
2 participants
Internal performance logs (
console.time()) in the data.controller.js have been successfully gated behind aDEBUGenvironment variable. This ensures production logs remain clean while keeping performance tracing available for development.🚀 Pull Request Description
Fixes #69
Gated high-frequency
console.time()andconsole.timeEnd()performance logs in apps/public-api/src/controllers/data.controller.js behind aprocess.env.DEBUG === 'true'flag. This significantly reduces log noise in production environments while allowing developers to enable performance tracing by setting the flag.🛠️ Type of Change
🧪 Testing & Validation
Backend Verification:
npm run lintin theapps/public-apiworkspace and it passed.npm test(Note: Local environment limitations restricted fulljestexecution, but logic was manually verified for zero-impact on API responses).Frontend Verification:
npm run lintin thefrontend/directory.📸 Screenshots / Recordings (Optional)
N/A
✅ Checklist
Built with ❤️ for urBackend.
Summary by CodeRabbit