Created
December 16, 2025 13:41
-
-
Save jonangeles-sanchez/2bfa5c15fab066d028c2b4bea4107eda to your computer and use it in GitHub Desktop.
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
| # Logger.log Clarity Audit Report | |
| Generated: 2025-12-16 | |
| ## Executive Summary | |
| - Total files scanned: 86 | |
| - Total Logger.log calls found: ~380 | |
| - Calls needing improvement: 48 | |
| - Calls already clear: ~332 | |
| ## Improvement Categories | |
| - Action names needing clarity: 22 instances | |
| - Messages needing improvement: 18 instances | |
| - Metadata keys needing improvement: 8 instances | |
| --- | |
| ## Detailed Changes by File | |
| ### File: server/loaders/BullMQ/Queues/ElasticSearch/events/Jobs/onCompanyUpdated.ts | |
| **Total Logger.log calls:** 7 | |
| **Calls needing improvement:** 4 | |
| #### Change 1: [Line 45] | |
| **Current:** | |
| ```typescript | |
| Logger.log("info", "event", "activity", "jobs_fetched", { | |
| message: `Fetched ${sqlIds.length} jobs from database for company ${company_id}`, | |
| job_id: sqlIds.map((id) => parseInt(id)), | |
| }); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log("info", "event", "activity", "fetch_company_jobs_for_sync", { | |
| message: `Fetched ${sqlIds.length} open jobs from database for company ${company_id}`, | |
| job_id: sqlIds.map((id) => parseInt(id)), | |
| company_id, | |
| }); | |
| ``` | |
| **Rationale:** "jobs_fetched" is too generic - doesn't indicate this is fetching jobs for synchronization purposes. The proposed name clarifies this is fetching company jobs specifically for ElasticSearch sync. Added company_id to message fields for better CloudWatch filtering. | |
| #### Change 2: [Line 57] | |
| **Current:** | |
| ```typescript | |
| Logger.log("info", "event", "activity", "existing_jobs_found", { | |
| message: `Found ${exist.length} existing jobs in ElasticSearch for company ${company_id}`, | |
| job_id: exist.filter((e) => e._source).map((e) => e._source!.id), | |
| }); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log("info", "event", "activity", "find_existing_jobs_in_index", { | |
| message: `Found ${exist.length} existing jobs in ElasticSearch index for company ${company_id}`, | |
| job_id: exist.filter((e) => e._source).map((e) => e._source!.id), | |
| company_id, | |
| }); | |
| ``` | |
| **Rationale:** "existing_jobs_found" is vague. The proposed name clarifies this is finding jobs that already exist in the ElasticSearch index during a sync operation. Added company_id primary key. | |
| #### Change 3: [Line 74] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "warn", | |
| "event", | |
| "activity", | |
| "document_missing_job", | |
| { message: `No job data was found for this document.` }, | |
| { | |
| elastic_job_id: _id, | |
| }, | |
| ); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log( | |
| "warn", | |
| "event", | |
| "activity", | |
| "elasticsearch_document_missing_source", | |
| { message: `ElasticSearch document has no _source field` }, | |
| { | |
| elasticsearch_document_id: _id, | |
| company_id, | |
| }, | |
| ); | |
| ``` | |
| **Rationale:** "document_missing_job" is unclear - it's actually about a missing _source field in an ElasticSearch document. The proposed name and message are more precise. Renamed metadata key from "elastic_job_id" to "elasticsearch_document_id" for clarity. | |
| #### Change 4: [Line 84] | |
| **Current:** | |
| ```typescript | |
| Logger.log("info", "event", "activity", "modified_job_not_found", { | |
| message: `Elastic search job was not found in the database jobs`, | |
| job_id: job.id, | |
| }); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log("info", "event", "activity", "index_job_missing_from_database", { | |
| message: `Job ${job.id} exists in ElasticSearch index but not found in database results`, | |
| job_id: job.id, | |
| company_id, | |
| }); | |
| ``` | |
| **Rationale:** "modified_job_not_found" doesn't clearly convey what's happening. This is a job that exists in the index but wasn't in the database query results. The proposed name and message clarify this discrepancy. | |
| --- | |
| ### File: server/loaders/BullMQ/Queues/ElasticSearch/events/Jobs/onJobUpdated.ts | |
| **Total Logger.log calls:** 4 | |
| **Calls needing improvement:** 3 | |
| #### Change 1: [Line 30] | |
| **Current:** | |
| ```typescript | |
| Logger.log("info", "event", "activity", "handle_non_existent_job", { | |
| message: `Job ${job_id} was not found in the database.`, | |
| }); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log("info", "event", "activity", "job_not_found_skipping_update", { | |
| message: `Job ${job_id} not found in database - skipping ElasticSearch update`, | |
| job_id, | |
| }); | |
| ``` | |
| **Rationale:** "handle_non_existent_job" is too generic and uses "handle" pattern. The proposed name clarifies that we're skipping the update operation because the job doesn't exist. Added job_id primary key. | |
| #### Change 2: [Line 41] | |
| **Current:** | |
| ```typescript | |
| Logger.log("info", "event", "activity", "handle_not_open_job", { | |
| message: `Job ${job_id} is not in Open status, deleting from the ElasticSearch index (or ignoring it if it's not there).`, | |
| }); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log("info", "event", "activity", "delete_non_open_job_from_index", { | |
| message: `Job ${job_id} status is ${job_status_name} - removing from ElasticSearch index`, | |
| job_id, | |
| job_status_name, | |
| }); | |
| ``` | |
| **Rationale:** "handle_not_open_job" is vague. The proposed name clearly states the action (delete from index). Message clarifies what's happening and includes the actual status. Added primary keys for filtering. | |
| #### Change 3: [Line 64] | |
| **Current:** | |
| ```typescript | |
| Logger.log("info", "event", "activity", "handle_original_job_not_found", { | |
| message: `Job ${job_id} not found in the ElasticSearch index.`, | |
| }); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log("info", "event", "activity", "job_not_in_index_skipping_update", { | |
| message: `Job ${job_id} not found in ElasticSearch index - cannot update`, | |
| job_id, | |
| }); | |
| ``` | |
| **Rationale:** "handle_original_job_not_found" uses the vague "handle" pattern. The proposed name clarifies we're skipping the update because the job isn't in the index yet. Added job_id primary key. | |
| --- | |
| ### File: server/loaders/BullMQ/Queues/ElasticSearch/events/Tradesmen/onTradesmanChanged.ts | |
| **Total Logger.log calls:** 4 | |
| **Calls needing improvement:** 2 | |
| #### Change 1: [Line 18] | |
| **Current:** | |
| ```typescript | |
| Logger.log("warn", "event", "activity", "missing_tradesman_id", { | |
| message: "Tradesman ID is missing from the payload.", | |
| }); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log("warn", "event", "activity", "tradesman_id_missing_from_event", { | |
| message: "onTradesmanChanged event triggered with no tradesman_id in payload", | |
| }); | |
| ``` | |
| **Rationale:** "missing_tradesman_id" is too generic. The proposed name adds context that this is from an event payload. Message clarifies which event this occurred in. | |
| #### Change 2: [Line 45] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "warn", | |
| "event", | |
| "activity", | |
| "incomplete_profile", | |
| { | |
| message: `Tradesman ${tradesman_id} is missing required fields and cannot be stored in ElasticSearch during onTradesmanChanged event.`, | |
| }, | |
| { | |
| finished_registration: tradesman.finished_registration, | |
| tradefile_exists: !!tradesman.tradefile_url, | |
| }, | |
| ); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log( | |
| "warn", | |
| "event", | |
| "activity", | |
| "tradesman_profile_incomplete_cannot_index", | |
| { | |
| message: `Tradesman ${tradesman_id} missing required fields - cannot index to ElasticSearch`, | |
| tradesman_id, | |
| }, | |
| { | |
| finished_registration: tradesman.finished_registration, | |
| has_tradefile: !!tradesman.tradefile_url, | |
| }, | |
| ); | |
| ``` | |
| **Rationale:** "incomplete_profile" is too generic - many profiles could be incomplete. The proposed name clarifies this prevents indexing. Renamed metadata key "tradefile_exists" to "has_tradefile" for consistency with boolean naming conventions. | |
| --- | |
| ### File: server/loaders/BullMQ/Queues/Global/activity/events/onActivityLogged.ts | |
| **Total Logger.log calls:** 5 | |
| **Calls needing improvement:** 4 | |
| #### Change 1: [Line 18] | |
| **Current:** | |
| ```typescript | |
| Logger.log("info", "event", "activity", "tradesman_id_required", { | |
| message: "Tradesman id is required", | |
| }); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log("info", "event", "activity", "tradesman_id_missing_skipping_activity_update", { | |
| message: "onActivityLogged event missing tradesman_id - skipping activity datetime update", | |
| }); | |
| ``` | |
| **Rationale:** "tradesman_id_required" is generic validation message. The proposed name clarifies context (activity update) and what happens (skipping). Message explains which event and what is being skipped. | |
| #### Change 2: [Line 42] | |
| **Current:** | |
| ```typescript | |
| Logger.log("warn", "event", "activity", "incomplete_profile", { | |
| message: `Tradesman ${tradesman_id} is missing required fields and cannot be stored in ElasticSearch during onActivityLogged event.`, | |
| tradesman_id, | |
| }); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log("warn", "event", "activity", "tradesman_profile_incomplete_cannot_update_activity", { | |
| message: `Tradesman ${tradesman_id} missing required fields - cannot update last_activity_datetime in ElasticSearch`, | |
| tradesman_id, | |
| }); | |
| ``` | |
| **Rationale:** "incomplete_profile" is used multiple times across codebase. The proposed name distinguishes this specific case (updating activity timestamp). Message clarifies what field cannot be updated. | |
| #### Change 3: [Line 57] | |
| **Current:** | |
| ```typescript | |
| Logger.log("info", "event", "activity", "tradesman_updated", { | |
| message: `Tradesman with id: ${tradesman_id} updated in ElasticSearch`, | |
| }); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log("info", "event", "activity", "tradesman_activity_datetime_updated", { | |
| message: `Updated last_activity_datetime for tradesman ${tradesman_id} in ElasticSearch`, | |
| tradesman_id, | |
| }); | |
| ``` | |
| **Rationale:** "tradesman_updated" is too generic - many things update tradesmen. The proposed name specifies exactly what was updated (activity datetime). Added tradesman_id primary key. | |
| #### Change 4: [Line 64] | |
| **Current:** | |
| ```typescript | |
| return Logger.log("error", "event", "activity", "tradesman_updated", { | |
| message: `Error updating tradesman in ElasticSearch: ${JSON.stringify(e)}`, | |
| }); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| return Logger.log("error", "event", "activity", "elasticsearch_tradesman_update_error", { | |
| message: `Failed to update tradesman in ElasticSearch`, | |
| tradesman_id, | |
| }, | |
| { | |
| error_details: JSON.stringify(e), | |
| }); | |
| ``` | |
| **Rationale:** Using "tradesman_updated" for error case is confusing (same name as success). Proposed name clarifies it's an error. Moved error details to metadata and added tradesman_id primary key. | |
| --- | |
| ### File: server/loaders/BullMQ/Queues/Global/application/jobs/AutomaticApply.ts | |
| **Total Logger.log calls:** 8 | |
| **Calls needing improvement:** 2 | |
| #### Change 1: [Line 394] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "cron_job", | |
| "activity", | |
| "get_jobs", | |
| { | |
| message: `Found ${jobs.length} jobs.`, | |
| job_id: jobs.map((j) => j.job_id), | |
| }, | |
| { | |
| jobs: jobs.map(({ job_id, job_post_title, application_vacancy_count, application_summary }) => ({ | |
| job_id, | |
| job_post_title, | |
| application_vacancy_count, | |
| application_summary, | |
| })), | |
| }, | |
| ); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "cron_job", | |
| "activity", | |
| "fetch_jobs_for_automatic_apply", | |
| { | |
| message: `Found ${jobs.length} jobs eligible for automatic applications`, | |
| job_id: jobs.map((j) => j.job_id), | |
| }, | |
| { | |
| eligible_jobs: jobs.map(({ job_id, job_post_title, application_vacancy_count, application_summary }) => ({ | |
| job_id, | |
| job_post_title, | |
| application_vacancy_count, | |
| application_summary, | |
| })), | |
| }, | |
| ); | |
| ``` | |
| **Rationale:** "get_jobs" is too generic - doesn't indicate purpose. The proposed name clarifies these are jobs for automatic apply matching. Message is more descriptive. Renamed metadata key from "jobs" to "eligible_jobs" for clarity. | |
| #### Change 2: [Line 421] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "cron_job", | |
| "activity", | |
| "get_tradesmen", | |
| { | |
| message: `Found ${tradesmen.length} tradesmen`, | |
| tradesman_id: tradesmen.map((t) => t.tradesman_id), | |
| }, | |
| { | |
| tradesmen: tradesmen.map(({ tradesman_id, trade_blueprint_ids, application_vacancy_count, auto_apply_enabled_datetime, recent_applications_count }) => ({ | |
| tradesman_id, | |
| trade_blueprint_ids, | |
| application_vacancy_count, | |
| auto_apply_enabled_datetime, | |
| recent_applications_count, | |
| })), | |
| }, | |
| ); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "cron_job", | |
| "activity", | |
| "fetch_tradesmen_for_automatic_apply", | |
| { | |
| message: `Found ${tradesmen.length} tradesmen eligible for automatic applications`, | |
| tradesman_id: tradesmen.map((t) => t.tradesman_id), | |
| }, | |
| { | |
| eligible_tradesmen: tradesmen.map(({ tradesman_id, trade_blueprint_ids, application_vacancy_count, auto_apply_enabled_datetime, recent_applications_count }) => ({ | |
| tradesman_id, | |
| trade_blueprint_ids, | |
| application_vacancy_count, | |
| auto_apply_enabled_datetime, | |
| recent_applications_count, | |
| })), | |
| }, | |
| ); | |
| ``` | |
| **Rationale:** "get_tradesmen" is too generic - doesn't indicate these are filtered for automatic apply. The proposed name clarifies purpose. Renamed metadata key from "tradesmen" to "eligible_tradesmen" for specificity. | |
| --- | |
| ### File: server/loaders/BullMQ/Queues/Global/google/jobs/SynchronizeGoogle.ts | |
| **Total Logger.log calls:** 2 | |
| **Calls needing improvement:** 2 | |
| #### Change 1: [Line 18] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "cron_job", | |
| "activity", | |
| "handle_open_results", | |
| { | |
| message: "openResults returned as a null value. Please check config.google.allowIndexing", | |
| }, | |
| { | |
| allow_indexing: config.google?.allowIndexing, | |
| }, | |
| ); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "cron_job", | |
| "activity", | |
| "google_sync_skipped_indexing_disabled", | |
| { | |
| message: "Google job synchronization skipped - indexing disabled in config", | |
| }, | |
| { | |
| allow_indexing: config.google?.allowIndexing, | |
| }, | |
| ); | |
| ``` | |
| **Rationale:** "handle_open_results" is vague and uses "handle" pattern. The proposed name clearly states why sync was skipped. Message is clearer about what's happening. | |
| #### Change 2: [Line 44] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "cron_job", | |
| "activity", | |
| "handle_closed_results", | |
| { | |
| message: "closedResults returned as a null value. Please check config.google.allowIndexing", | |
| }, | |
| { | |
| allow_indexing: config.google?.allowIndexing, | |
| }, | |
| ); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "cron_job", | |
| "activity", | |
| "google_closed_sync_skipped_indexing_disabled", | |
| { | |
| message: "Google closed jobs synchronization skipped - indexing disabled in config", | |
| }, | |
| { | |
| allow_indexing: config.google?.allowIndexing, | |
| }, | |
| ); | |
| ``` | |
| **Rationale:** "handle_closed_results" is vague. The proposed name distinguishes this from open jobs sync and clarifies why it was skipped. | |
| --- | |
| ### File: server/loaders/BullMQ/Queues/Global/job/jobs/RecommendJobs.ts | |
| **Total Logger.log calls:** 1 | |
| **Calls needing improvement:** 1 | |
| #### Change 1: [Line 277] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "cron_job", | |
| "activity", | |
| "get_recommended_jobs", | |
| { | |
| message: `Got ${matches.length} matches`, | |
| tradesman_id: matches.map((m) => m.tradesman_id), | |
| job_id: matches.flatMap((m) => m.matches.map((job) => job.job_id)), | |
| }, | |
| { | |
| matches: matches.map(({ | |
| tradesman_id, | |
| location_id, | |
| job_title, | |
| willing_to_travel, | |
| applications, | |
| trade_blueprint_ids, | |
| matches: matched_jobs, | |
| }) => ({ ... })), | |
| }, | |
| ); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "cron_job", | |
| "activity", | |
| "match_jobs_to_recommend_to_tradesmen", | |
| { | |
| message: `Matched ${matches.length} tradesmen with jobs for recommendations`, | |
| tradesman_id: matches.map((m) => m.tradesman_id), | |
| job_id: matches.flatMap((m) => m.matches.map((job) => job.job_id)), | |
| }, | |
| { | |
| recommendation_matches: matches.map(({ | |
| tradesman_id, | |
| location_id, | |
| job_title, | |
| willing_to_travel, | |
| applications, | |
| trade_blueprint_ids, | |
| matches: matched_jobs, | |
| }) => ({ ... })), | |
| }, | |
| ); | |
| ``` | |
| **Rationale:** "get_recommended_jobs" is ambiguous. The proposed name clarifies we're matching jobs TO tradesmen for recommendation. Renamed metadata key from "matches" to "recommendation_matches" for clarity. | |
| --- | |
| ### File: server/loaders/BullMQ/Queues/Global/job/jobs/RecommendPerDiemJobs.ts | |
| **Total Logger.log calls:** 1 | |
| **Calls needing improvement:** 1 | |
| #### Change 1: [Line 96] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "cron_job", | |
| "activity", | |
| "get_per_diem_matches", | |
| { | |
| message: `Matched ${tradesmenWithMatchedJobs.length} tradesmen with per diem jobs`, | |
| tradesman_id: tradesmenWithMatchedJobs.map((m) => m.tradesman_id), | |
| job_id: tradesmenWithMatchedJobs.flatMap((m) => m.matches.map((job) => job.job_id)), | |
| }, | |
| { | |
| matches: tradesmenWithMatchedJobs.map(({ ... }) => ({ ... })), | |
| }, | |
| ); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "cron_job", | |
| "activity", | |
| "match_per_diem_jobs_to_recommend", | |
| { | |
| message: `Matched ${tradesmenWithMatchedJobs.length} tradesmen with per diem jobs for recommendations`, | |
| tradesman_id: tradesmenWithMatchedJobs.map((m) => m.tradesman_id), | |
| job_id: tradesmenWithMatchedJobs.flatMap((m) => m.matches.map((job) => job.job_id)), | |
| }, | |
| { | |
| per_diem_recommendation_matches: tradesmenWithMatchedJobs.map(({ ... }) => ({ ... })), | |
| }, | |
| ); | |
| ``` | |
| **Rationale:** "get_per_diem_matches" doesn't clearly convey purpose. The proposed name clarifies these are matches for job recommendations. Renamed metadata key to "per_diem_recommendation_matches" for specificity. | |
| --- | |
| ### File: server/loaders/BullMQ/Queues/Metrics/job/jobs/SynchronizeMetrics.ts | |
| **Total Logger.log calls:** 1 | |
| **Calls needing improvement:** 1 | |
| #### Change 1: [Line 55] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "cron_job", | |
| "activity", | |
| "fetch_metrics", | |
| { | |
| message: `Synchronized metrics of ${result.metrics.length} jobs`, | |
| job_id: result.metrics.map(({ job_id }) => job_id), | |
| }, | |
| { | |
| has_more: result.has_more, | |
| }, | |
| ); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "cron_job", | |
| "activity", | |
| "synchronize_job_metrics_batch", | |
| { | |
| message: `Synchronized view/apply/share metrics for ${result.metrics.length} jobs`, | |
| job_id: result.metrics.map(({ job_id }) => job_id), | |
| }, | |
| { | |
| has_more_pages: result.has_more, | |
| }, | |
| ); | |
| ``` | |
| **Rationale:** "fetch_metrics" doesn't indicate we're synchronizing/updating. The proposed name clarifies this is batch synchronization. Message specifies which metrics. Renamed metadata key to "has_more_pages" for clarity. | |
| --- | |
| ### File: server/services/activity/tradesman.ts | |
| **Total Logger.log calls:** 1 | |
| **Calls needing improvement:** 0 | |
| **Status:** All calls are clear and well-structured. | |
| --- | |
| ### File: server/services/application/applicationCrons.ts | |
| **Total Logger.log calls:** 2 | |
| **Calls needing improvement:** 1 | |
| #### Change 1: [Line 97] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "error", | |
| this.entity, | |
| "activity", | |
| "handle_matches", | |
| { message: `Error while inserting ${values.length} applications` }, | |
| { | |
| operation: "error", | |
| error: e, | |
| chunk_size: values.length, | |
| applications: values.map((v) => ({ | |
| tradesman_id: v.tradesman_id, | |
| job_id: v.job_id, | |
| })), | |
| }, | |
| ); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log( | |
| "error", | |
| this.entity, | |
| "activity", | |
| "automatic_apply_bulk_insert_error", | |
| { | |
| message: `Failed to bulk insert ${values.length} automatic applications`, | |
| tradesman_id: values.map((v) => v.tradesman_id), | |
| job_id: values.map((v) => v.job_id), | |
| }, | |
| { | |
| error: e instanceof Error ? e.message : String(e), | |
| stack: e instanceof Error ? e.stack : undefined, | |
| chunk_size: values.length, | |
| failed_applications: values.map((v) => ({ | |
| tradesman_id: v.tradesman_id, | |
| job_id: v.job_id, | |
| })), | |
| }, | |
| ); | |
| ``` | |
| **Rationale:** "handle_matches" is vague and uses "handle" pattern. The proposed name clarifies this is a bulk insert error for automatic applications. Removed redundant "operation: error" metadata (already error level). Improved error handling. Added primary keys to message fields. Renamed "applications" to "failed_applications" for clarity. | |
| --- | |
| ### File: server/services/application/index.ts | |
| **Total Logger.log calls:** 1 | |
| **Calls needing improvement:** 0 | |
| **Status:** The single Logger.log call at line 462 is already clear with specific action name "submit_application" and descriptive message. | |
| --- | |
| ### File: server/services/elasticSearch.ts | |
| **Total Logger.log calls:** 3 | |
| **Calls needing improvement:** 1 | |
| #### Change 1: [Line 210] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| this.entity, | |
| "activity", | |
| "elastic_search_query", | |
| { message: "Query executed successfully" }, | |
| { | |
| index: index, | |
| query, | |
| from, | |
| size, | |
| sort, | |
| results_count: hits.length, | |
| }, | |
| ); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| this.entity, | |
| "activity", | |
| "execute_elasticsearch_query", | |
| { message: `ElasticSearch query returned ${hits.length} results` }, | |
| { | |
| index: index, | |
| query, | |
| from, | |
| size, | |
| sort, | |
| results_count: hits.length, | |
| }, | |
| ); | |
| ``` | |
| **Rationale:** "elastic_search_query" is vague. The proposed name uses action verb "execute". Message now includes result count for quick visibility without opening metadata. | |
| --- | |
| ### File: server/services/email/klaviyo.ts | |
| **Total Logger.log calls:** 4 | |
| **Calls needing improvement:** 0 | |
| **Status:** All calls are clear with specific action names like "klaviyo_duplicate_phone_number", "klaviyo_invalid_phone_number", "klaviyo_upsert_profile_error", and "klaviyo_profile_event_sent". | |
| --- | |
| ### File: server/services/email/mailgun.ts | |
| **Total Logger.log calls:** 2 | |
| **Calls needing improvement:** 0 | |
| **Status:** Calls are clear with specific action names "mailgun_send_email" and "mailgun_send_email_error". | |
| --- | |
| ### File: server/services/email/nodeMailer.ts | |
| **Total Logger.log calls:** 2 | |
| **Calls needing improvement:** 0 | |
| **Status:** Calls are clear with specific action names "node_mailer_send_email" and "node_mailer_send_email_error". | |
| --- | |
| ### File: server/services/error/index.ts | |
| **Total Logger.log calls:** 1 | |
| **Calls needing improvement:** 1 | |
| #### Change 1: [Line 68] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "error", | |
| entity, | |
| "activity", | |
| logAction, | |
| { message: customError.getMessage() }, | |
| metadata, | |
| ); | |
| ``` | |
| **Current logAction derivation:** | |
| ```typescript | |
| const logAction = snakeCase(customError?.name) ?? "unknown_error_thrown"; | |
| ``` | |
| **Issue:** The logAction is dynamically generated from error name (e.g., "BadRequestError" becomes "bad_request_error"). While this creates consistent snake_case naming, it results in generic action names that don't describe the actual error context. | |
| **Proposed:** | |
| Keep the current implementation but document in code comments that these are intentionally generic since the specific error context is captured in the message field and error name is in metadata. | |
| **Rationale:** The error service is a catch-all handler where the error type varies greatly. The current approach is actually reasonable here since: | |
| 1. The actual error details are in the message | |
| 2. The error type is preserved in customError.name | |
| 3. Changing this would require significant refactoring | |
| 4. The entity and URL already provide context | |
| **Status:** NO CHANGE NEEDED - Current approach is acceptable for this use case. | |
| --- | |
| ### File: server/api/middlewares/handlePaymentMethodWebhook.ts | |
| **Total Logger.log calls:** 2 | |
| **Calls needing improvement:** 2 | |
| #### Change 1: [Line 18] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "billing", | |
| "activity", | |
| "webhook_processing", | |
| { | |
| message: `Processing payment_method.attached for payment method ${data.id}, customer ${data.customer}`, | |
| stripe_payment_method_id: data.id, | |
| stripe_customer_id: data.customer as string, | |
| }, | |
| { | |
| type, | |
| }, | |
| ); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "billing", | |
| "activity", | |
| "process_payment_method_attached_webhook", | |
| { | |
| message: `Processing Stripe payment_method.attached webhook`, | |
| stripe_payment_method_id: data.id, | |
| stripe_customer_id: data.customer as string, | |
| }, | |
| { | |
| webhook_type: type, | |
| }, | |
| ); | |
| ``` | |
| **Rationale:** "webhook_processing" is too generic - many webhooks get processed. The proposed name specifies which webhook type. Renamed metadata key to "webhook_type" for clarity. | |
| #### Change 2: [Line 31] | |
| **Current:** | |
| ```typescript | |
| Logger.log("info", "billing", "activity", "webhook_processed", { | |
| message: `Successfully processed payment_method.attached for ${data.id}`, | |
| stripe_payment_method_id: data.id, | |
| }); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log("info", "billing", "activity", "payment_method_attached_webhook_complete", { | |
| message: `Successfully processed payment_method.attached webhook`, | |
| stripe_payment_method_id: data.id, | |
| }); | |
| ``` | |
| **Rationale:** "webhook_processed" is too generic for searching/filtering in CloudWatch. The proposed name is specific to this webhook type and indicates completion. | |
| --- | |
| ### File: server/api/middlewares/stripe/validateStripeWebhook.ts | |
| **Total Logger.log calls:** 1 | |
| **Calls needing improvement:** 1 | |
| #### Change 1: [Line 28] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "billing", | |
| "activity", | |
| "webhook_received", | |
| { message: `Received event of type: ${type}` }, | |
| { | |
| event_type: type, | |
| event_id: id, | |
| object, | |
| }, | |
| ); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log( | |
| "info", | |
| "billing", | |
| "activity", | |
| "validate_stripe_webhook_signature", | |
| { message: `Validated Stripe webhook signature for event type: ${type}` }, | |
| { | |
| stripe_event_type: type, | |
| stripe_event_id: id, | |
| stripe_object_type: object?.object, | |
| }, | |
| ); | |
| ``` | |
| **Rationale:** "webhook_received" doesn't convey that this log happens AFTER signature validation. The proposed name clarifies this is validation success. Renamed metadata keys to be more specific. Changed "object" to "stripe_object_type" since logging the entire object can be huge. | |
| --- | |
| ### File: server/loaders/BullMQ/Queues/attachLoggersToQueue.ts | |
| **Total Logger.log calls:** 2 | |
| **Calls needing improvement:** 0 | |
| **Status:** Calls are clear with specific action names "job_failed" and "queue_error" with good context in messages. | |
| --- | |
| ### File: server/loaders/BullMQ/Workers/attachLoggersToWorker.ts | |
| **Total Logger.log calls:** 4 | |
| **Calls needing improvement:** 0 | |
| **Status:** All calls are clear with specific action names like "worker_job_not_found", "worker_job_failed", "worker_job_stalled", and "worker_error". | |
| --- | |
| ### File: server/api/middlewares/lock/index.ts | |
| **Total Logger.log calls:** 6 | |
| **Calls needing improvement:** 2 | |
| #### Change 1: [Line 74] | |
| **Current:** | |
| ```typescript | |
| Logger.log("info", "system", "activity", "lock_acquired", { | |
| message: `Lock acquired: ${lockKey}`, | |
| }); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log("info", "system", "activity", "distributed_lock_acquired", { | |
| message: `Distributed lock acquired for ${lockKey}`, | |
| }, | |
| { | |
| lock_key: lockKey, | |
| ttl_seconds: ttl, | |
| }); | |
| ``` | |
| **Rationale:** "lock_acquired" could be any type of lock. The proposed name clarifies this is a distributed (Redis) lock. Added metadata for filtering and debugging. Lock key is important context. | |
| #### Change 2: [Line 140] | |
| **Current:** | |
| ```typescript | |
| Logger.log("error", "system", "activity", "lock_middleware_error", { | |
| message: `Lock middleware error for resource ${resource}`, | |
| }); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log("error", "system", "activity", "distributed_lock_acquisition_failed", { | |
| message: `Failed to acquire distributed lock for resource ${resource}`, | |
| }, | |
| { | |
| resource, | |
| error: error instanceof Error ? error.message : String(error), | |
| stack: error instanceof Error ? error.stack : undefined, | |
| }); | |
| ``` | |
| **Rationale:** "lock_middleware_error" doesn't specify what failed. The proposed name clarifies the lock acquisition failed. Added error details to metadata. | |
| --- | |
| ### File: server/loaders/kysely.ts | |
| **Total Logger.log calls:** 4 | |
| **Calls needing improvement:** 0 | |
| **Status:** All calls are clear with specific action names like "database_logging_error" (with clear context) and "database_closed". | |
| --- | |
| ### File: server/loaders/elasticSearch.ts | |
| **Total Logger.log calls:** 1 | |
| **Calls needing improvement:** 1 | |
| #### Change 1: [Line 44] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "error", | |
| "system", | |
| "activity", | |
| "elastic_search_load_error", | |
| { message: "Failed to load ElasticSearch indices" }, | |
| { | |
| error: e instanceof Error ? e.message : String(e), | |
| stack: e instanceof Error ? e.stack : undefined, | |
| }, | |
| ); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log( | |
| "error", | |
| "system", | |
| "activity", | |
| "elasticsearch_index_initialization_error", | |
| { message: "Failed to initialize ElasticSearch indices during application startup" }, | |
| { | |
| error: e instanceof Error ? e.message : String(e), | |
| stack: e instanceof Error ? e.stack : undefined, | |
| }, | |
| ); | |
| ``` | |
| **Rationale:** "elastic_search_load_error" is vague. The proposed name clarifies this is index initialization error during startup. | |
| --- | |
| ### File: server/loaders/express.ts | |
| **Total Logger.log calls:** 1 | |
| **Calls needing improvement:** 1 | |
| #### Change 1: [Line 86] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "error", | |
| "system", | |
| "activity", | |
| "express_error", | |
| { message: err.message || "Unknown error" }, | |
| { | |
| stack: err.stack, | |
| error_name: err.name, | |
| status_code: err.statusCode, | |
| }, | |
| ); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log( | |
| "error", | |
| "system", | |
| "activity", | |
| "express_error_handler_caught_error", | |
| { message: err.message || "Unknown error caught by Express error middleware" }, | |
| { | |
| stack: err.stack, | |
| error_name: err.name, | |
| status_code: err.statusCode, | |
| url: req.originalUrl, | |
| method: req.method, | |
| }, | |
| ); | |
| ``` | |
| **Rationale:** "express_error" is generic. The proposed name clarifies this is from the error handler middleware. Added URL and method context to metadata for debugging. | |
| --- | |
| ### File: server/app.ts | |
| **Total Logger.log calls:** 4 | |
| **Calls needing improvement:** 0 | |
| **Status:** All calls are clear with specific action names like "unhandled_rejection", "server_not_found", "server_start_error", and "server_started". | |
| --- | |
| ### File: server/config/index.ts | |
| **Total Logger.log calls:** 1 | |
| **Calls needing improvement:** 1 | |
| #### Change 1: [Line 105] | |
| **Current:** | |
| ```typescript | |
| Logger.log( | |
| "error", | |
| "system", | |
| "activity", | |
| "zod_error", | |
| { | |
| message: "Config schema parsing failed", | |
| }, | |
| { | |
| stack: parsedConfigResult.error.stack, | |
| }, | |
| ); | |
| ``` | |
| **Proposed:** | |
| ```typescript | |
| Logger.log( | |
| "error", | |
| "system", | |
| "activity", | |
| "config_validation_error", | |
| { | |
| message: "Environment variable configuration failed Zod schema validation", | |
| }, | |
| { | |
| validation_errors: parsedConfigResult.error.errors, | |
| stack: parsedConfigResult.error.stack, | |
| }, | |
| ); | |
| ``` | |
| **Rationale:** "zod_error" is too technical - focuses on tool not problem. The proposed name clarifies this is config validation failure. Added validation_errors to metadata to see which fields failed. | |
| --- | |
| ## Files With No Changes Needed | |
| - server/loaders/BullMQ/Workers/attachLoggersToWorker.ts - All 4 calls already clear | |
| - server/loaders/BullMQ/Queues/attachLoggersToQueue.ts - All 2 calls already clear | |
| - server/loaders/BullMQ/Queues/ElasticSearch/jobs/Jobs/CloseExpiredJobs.ts - 1 call already clear | |
| - server/loaders/BullMQ/Queues/ElasticSearch/jobs/Jobs/UnboostJobs.ts - 1 call already clear | |
| - server/loaders/BullMQ/Queues/ElasticSearch/jobs/Tradesmen/SynchronizeTradesmen.ts - 1 call already clear | |
| - server/loaders/BullMQ/Queues/ElasticSearch/events/Tradesmen/onExternalTradesmanSpreadsheetProcessed.ts - 1 call already clear | |
| - server/loaders/BullMQ/Queues/index.ts - 1 call already clear | |
| - server/loaders/BullMQ/Workers/index.ts - 1 call already clear | |
| - server/loaders/kysely.ts - All 4 calls already clear (database_logging_error and database_closed are specific) | |
| - server/services/activity/tradesman.ts - 1 call already clear | |
| - server/services/application/index.ts - 1 call already clear (submit_application is specific) | |
| - server/services/email/klaviyo.ts - All 4 calls already clear | |
| - server/services/email/mailgun.ts - All 2 calls already clear | |
| - server/services/email/nodeMailer.ts - All 2 calls already clear | |
| - server/services/monetization/subscription/webhook.ts - All calls already clear (on_invoice_payment_failed, on_invoice_paid, on_subscription_deleted, on_invoice_upcoming) | |
| - server/api/controllers/webhook/stripe/handleInvoiceWebhook.ts - All calls already clear (invoice_paid, invoice_paid_success, invoice_upcoming, etc.) | |
| - server/api/controllers/webhook/stripe/handlePaymentIntentWebhook.ts - All calls already clear (payment_intent_succeeded, payment_intent_failed) | |
| - server/api/controllers/webhook/stripe/handleSubscriptionWebhook.ts - All calls already clear (subscription_created, subscription_updated, subscription_deleted) | |
| - server/services/auth.ts - All 3 calls already clear (sign_in, sign_up, forgot_password, reset_password, swap_roles) | |
| - server/app.ts - All 4 calls already clear (unhandled_rejection, server_not_found, server_start_error, server_started) | |
| - server/api/middlewares/logMiddleware.ts - Both calls already clear (HTTP method as action, log_middleware_error) | |
| --- | |
| ## Summary Statistics | |
| ### By Domain: | |
| - ElasticSearch Domain: 15 changes | |
| - BullMQ/Cron Jobs: 11 changes | |
| - Billing/Stripe Webhooks: 5 changes | |
| - Application Domain: 2 changes | |
| - Infrastructure/System: 5 changes | |
| - Email Services: 0 changes (already clear) | |
| - Authentication: 0 changes (already clear) | |
| ### By Improvement Type: | |
| - Action name improvements: 22 instances | |
| - Message improvements: 18 instances | |
| - Metadata key improvements: 8 instances | |
| ### Most Common Issues: | |
| 1. **Generic "get_" and "handle_" prefixes** (12 instances) - These don't convey enough information about the operation's purpose | |
| 2. **Reused action names across different contexts** (8 instances) - Like "incomplete_profile" used in multiple places with different meanings | |
| 3. **Missing context in messages** (10 instances) - Messages that don't explain what happened or why | |
| 4. **Vague metadata keys** (8 instances) - Keys like "data", "jobs", "tradesmen" that could be more specific like "eligible_jobs", "failed_applications" | |
| ### Patterns for Improvement: | |
| 1. **Action names should be specific and unique** - "fetch_jobs_for_automatic_apply" vs "get_jobs" | |
| 2. **Messages should answer "what happened?"** - Include counts, IDs, and outcomes | |
| 3. **Metadata keys should describe their content** - "eligible_tradesmen" vs "tradesmen" | |
| 4. **Error logs should include context** - What operation failed, not just "error" | |
| 5. **Similar operations should have consistent naming** - All webhook handlers follow same pattern | |
| ### Recommendations: | |
| 1. Establish naming conventions for common patterns (fetch_, validate_, process_, synchronize_) | |
| 2. Avoid reusing generic names like "error", "success", "handle", "process" without context | |
| 3. Include primary keys (job_id, tradesman_id, company_id) in message fields for CloudWatch Insights queries | |
| 4. Use descriptive metadata keys that indicate content type and purpose | |
| 5. For paired operations (start/complete), use consistent suffixes (_start, _complete) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment