-
Notifications
You must be signed in to change notification settings - Fork 19
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
improve logger #1907
improve logger #1907
Conversation
WalkthroughThe pull request modifies the logging mechanism in the Changes
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/services/cronJobs/checkAndUpdateEndaomentProject.ts (3)
29-29
: Consider adding structured logging with project IDs.While reducing log verbosity is good, completely removing project IDs from logs could make production debugging more difficult. Consider using structured logging to maintain both brevity and debuggability.
- logger.debug('Projects fetched:', projects.length); + logger.debug('Projects fetched', { + count: projects.length, + projectIds: projects.map(p => p.id), + // Log IDs in batches of 10 to avoid excessive verbosity + sampleProjects: projects.slice(0, 10).map(p => ({ + id: p.id, + endaomentId: p.endaomentId + })) + });
Line range hint
31-45
: Enhance error handling and logging patterns.The current error handling could benefit from more structured logging and metrics tracking. Consider these improvements:
- Add correlation IDs for each job run
- Track success/failure metrics
- Use consistent error logging patterns
for (const project of projects) { + const correlationId = `job_${Date.now()}_proj_${project.id}`; try { // Fetch details from Endaoment API if (project.endaomentId) { + logger.debug('Processing project', { + correlationId, + projectId: project.id, + endaomentId: project.endaomentId + }); const orgData = await EndaomentService.fetchOrgDetails( project.endaomentId, ); // Update project details or mark as cancelled await EndaomentService.updateProjectDetails(project, orgData); + metrics.incrementSuccess('endaoment_project_update'); } else { logger.warn( - `Project ID ${project.id} does not have an endaomentId.`, + 'Project missing endaomentId', { + correlationId, + projectId: project.id + } ); + metrics.incrementWarning('endaoment_project_update'); } } catch (error) { - logger.error(`Failed to update project ID ${project.id}`, error); + logger.error('Failed to update project', { + correlationId, + projectId: project.id, + error: { + message: error.message, + stack: error.stack, + code: error.code + } + }); + metrics.incrementError('endaoment_project_update'); }
Line range hint
8-12
: Add job scheduling safeguards.The current cron job implementation could benefit from additional safeguards:
- Add retry mechanism for failed jobs
- Implement job locking to prevent concurrent runs
- Add timeout protection for long-running jobs
Consider using a job queue system like Bull or implementing these safeguards:
import { setTimeout } from 'timers/promises'; import { createLock } from './utils/locks'; const MAX_EXECUTION_TIME = 30 * 60 * 1000; // 30 minutes const LOCK_TTL = MAX_EXECUTION_TIME + 60 * 1000; // Add 1 minute buffer export const runCheckAndUpdateEndaomentProject = async () => { schedule(cronJobTime, async () => { const lock = createLock('endaoment_project_update', LOCK_TTL); try { const acquired = await lock.acquire(); if (!acquired) { logger.warn('Previous job still running, skipping'); return; } const timeoutPromise = setTimeout(MAX_EXECUTION_TIME); const jobPromise = processProjects(); await Promise.race([timeoutPromise, jobPromise]); } finally { await lock.release(); } }); };Also applies to: 20-21
Summary by CodeRabbit