Merge pull request #51 from Timer2Ticket/20273-better-logging-context

20273 better logging context

Author: hejdamax

src/app.ts

@@ -56,7 +56,7 @@ cron.schedule('*/10 * * * * *', () => {
           // console.log(' -> Repeating job');
           job.start().then(resRepeated => {
             if (resRepeated) {
-              // console.log(' -> Job repeated and now successfully done.');
+              //console.log(' -> Job repeated and now successfully done.');
               return;
             }
 

src/jobs/config_sync_job.ts

@@ -36,8 +36,7 @@ export class ConfigSyncJob extends SyncJob {
     //if we get boolean, a problem occurred while getting data from service (most likely 401 or 403 error)
     //we dont have all the service objects and stop the job
     if (typeof objectsToSync === "boolean") {
-      let message: string = 'Problem occurred while getting primary service objects.';
-      this._jobLog.errors.push(this._errorService.createConfigJobError(message));
+      await this.updateJobLog(primarySyncedService.errors)
       return false;
     }
 
@@ -51,8 +50,7 @@ export class ConfigSyncJob extends SyncJob {
       const allServiceObjects = await syncedService.getAllServiceObjects(configLastSyncAt);
       //same as above, there was a problem communication problem with service. We stop the job.
       if (typeof allServiceObjects === "boolean") {
-        let message: string = 'Problem occurred while getting secondary service objects';
-        this._jobLog.errors.push(this._errorService.createConfigJobError(message));
+        await this.updateJobLog(syncedService.errors)
         return false;
       }
 
@@ -103,11 +101,6 @@ export class ConfigSyncJob extends SyncJob {
         }
       } catch (ex) {
         operationsOk = false;
-        //TODO figure out better way to work with extra context.
-        //temporary console log to test if this is the correct place.
-        console.log(`User: ${this._user.username} experienced error in config job for object: ${objectToSync.name}, ${objectToSync.type}, ${objectToSync.id}`);
-        let context = this._sentryService.createExtraContext('Object_to_sync', JSON.parse(JSON.stringify(objectToSync)));
-        this._sentryService.logError(ex, context);
       }
     }
 
@@ -123,7 +116,7 @@ export class ConfigSyncJob extends SyncJob {
     // even if some api operations were not ok, persist changes to the mappings - better than nothing
     await databaseService.updateUserMappings(this._user);
 
-    await databaseService.updateJobLog(this._jobLog);
+    await this.updateJobLog(Array.from(secondaryServicesWrappersMap.values()).flatMap(wrapper => wrapper.syncedService?.errors ?? []));
 
     return operationsOk;
   }
@@ -152,7 +145,7 @@ export class ConfigSyncJob extends SyncJob {
           continue;
         }
         // firstly create object in the service, then create serviceObject with newly acquired id
-        const newObject = await this._createServiceObjectInService(serviceWrapper, objectToSync);
+        const newObject = await serviceWrapper.syncedService.createServiceObject(objectToSync.id, objectToSync.name, objectToSync.type);
         mappingsObject = new MappingsObject(newObject.id, newObject.name, serviceDefinition.name, newObject.type);
       }
 
@@ -189,45 +182,27 @@ export class ConfigSyncJob extends SyncJob {
         // mappingObject is missing, create a new one and add to mapping (maybe new service was added)
         // create a real object in the service and add mappingObject
         // firstly create object in the service, then create serviceObject with newly acquired id
-        const newObject = await this._createServiceObjectInService(serviceWrapper, objectToSync);
+        const newObject = await serviceWrapper.syncedService.createServiceObject(objectToSync.id, objectToSync.name, objectToSync.type);
         const newMappingsObject = new MappingsObject(newObject.id, newObject.name, serviceDefinition.name, newObject.type);
         mapping.mappingsObjects.push(newMappingsObject);
       } else {
         // scenario b), c), e)
         // check if mapping corresponds with real object in the service
-        const objectBasedOnMapping = await serviceWrapper.allServiceObjects
+        const objectBasedOnMapping = serviceWrapper.allServiceObjects
           .find(serviceObject => serviceObject.id === mappingsObject.id && serviceObject.type === mappingsObject.type);
 
         if (!objectBasedOnMapping) {
           // scenario e), create new object in the service
-          const newObject = await this._createServiceObjectInService(serviceWrapper, objectToSync);
+          const newObject = await serviceWrapper.syncedService.createServiceObject(objectToSync.id, objectToSync.name, objectToSync.type);
           mappingsObject.id = newObject.id;
           mappingsObject.name = newObject.name;
           mappingsObject.lastUpdated = Date.now();
         } else if (objectBasedOnMapping.name !== serviceWrapper.syncedService.getFullNameForServiceObject(objectToSync)) {
           // scenario b)
           // name is incorrect => maybe mapping was outdated or/and real object was outdated
-          let updatedObject = null;
-          try {
-                updatedObject = await serviceWrapper.syncedService.updateServiceObject(
-                mappingsObject.id, new ServiceObject(objectToSync.id, objectToSync.name, objectToSync.type)
-            );
-          } catch (ex: any) {
-            if (ex.status !== 400) {
-              throw ex;
-            }
-            // 400 ~ maybe object already exists and cannot be updated (for example object needs to be unique - name)?
-            // => try to find it and use it for the mapping
-            const serviceObjectName = serviceWrapper.syncedService.getFullNameForServiceObject(new ServiceObject(objectToSync.id, objectToSync.name, objectToSync.type));
-            updatedObject = serviceWrapper.allServiceObjects.find(serviceObject => serviceObject.name === serviceObjectName);
-            if (!updatedObject) {
-              const context = [
-                this._sentryService.createExtraContext('Object_to_sync', {'id': objectToSync.id, 'name': objectToSync.name, 'type': objectToSync.type})
-              ]
-              this._sentryService.logError(ex, context);
-              throw ex;
-            }
-          }
+          const updatedObject = await serviceWrapper.syncedService.updateServiceObject(
+              mappingsObject.id, new ServiceObject(objectToSync.id, objectToSync.name, objectToSync.type)
+          );
           // console.log(`ConfigSyncJob: Updated object ${updatedObject.name}`);
           mappingsObject.name = updatedObject.name;
           mappingsObject.lastUpdated = Date.now();
@@ -240,28 +215,4 @@ export class ConfigSyncJob extends SyncJob {
 
     return true;
   }
-
-  private async _createServiceObjectInService(serviceWrapper: SyncedServiceWrapper, objectToSync: ServiceObject): Promise<ServiceObject> {
-    let newObject;
-    try {
-      newObject = await serviceWrapper.syncedService.createServiceObject(objectToSync.id, objectToSync.name, objectToSync.type);
-    } catch (ex: any) {
-      if (ex.status !== 400) {
-        throw ex;
-      }
-      // 400 ~ maybe object already exists and cannot be created (for example object needs to be unique - name)?
-      // => try to find it and use it for the mapping
-      const serviceObjectName = serviceWrapper.syncedService.getFullNameForServiceObject(new ServiceObject(objectToSync.id, objectToSync.name, objectToSync.type));
-      newObject = serviceWrapper.allServiceObjects.find(serviceObject => serviceObject.name === serviceObjectName);
-      if (!newObject) {
-        let context = [
-          this._sentryService.createExtraContext('Object_to_sync', {'id': objectToSync.id, 'name': objectToSync.name, 'type': objectToSync.type})
-        ]
-        this._sentryService.logError(ex, context);
-        throw ex;
-      }
-      // console.log(`ConfigSyncJob: Creating mapping, but object exists, using real object ${newObject.name}`);
-    }
-    return newObject;
-  }
 }

src/jobs/remove_obsolete_mappings_job.ts

@@ -45,21 +45,29 @@ export class RemoveObsoleteMappingsJob extends SyncJob {
                         mapping.primaryObjectType == objectToRemove.type &&
                         (mapping.primaryObjectType == 'issue' || mapping.primaryObjectType == 'project' )));
             }
+        } else {
+            await this.updateJobLog(primarySyncedService.errors);
         }
 
         // case 2 - remove mappings with non-existing service object in primary service
         const mappingChunks = this._chunkArray(userMappings.filter(mapping => mapping.primaryObjectType == 'issue'), 50);
         for (const chunk of mappingChunks) {
-            const issues = await primarySyncedService.getServiceObjects(chunk.map(mapping => mapping.primaryObjectId));
-            if (issues.length !== chunk.length) {
-                // some issues were not found - find mappings to remove
-                const foundIssueIds = new Set(issues.map(issue => issue.id));
-                const notFoundMappings = chunk.filter(mapping => !foundIssueIds.has(mapping.primaryObjectId));
-                obsoleteMappings.push(...notFoundMappings);
+            try {
+                const issues = await primarySyncedService.getServiceObjects(chunk.map(mapping => mapping.primaryObjectId));
+                if (issues.length !== chunk.length) {
+                    // some issues were not found - find mappings to remove
+                    const foundIssueIds = new Set(issues.map(issue => issue.id));
+                    const notFoundMappings = chunk.filter(mapping => !foundIssueIds.has(mapping.primaryObjectId));
+                    obsoleteMappings.push(...notFoundMappings);
+                }
+            } catch (err: any) {
+                await this.updateJobLog(primarySyncedService.errors);
+                // keep old behaviour
+                // throw err;
             }
         }
 
-        console.log('Obsolete mappings: ', obsoleteMappings);
+        //console.log('Obsolete mappings: ', obsoleteMappings);
 
         // remove duplicates
         obsoleteMappings = Array.from(new Set(obsoleteMappings));
@@ -72,7 +80,9 @@ export class RemoveObsoleteMappingsJob extends SyncJob {
         // persist changes in the mappings
         // even if some api operations were not ok, persist changes to the mappings - better than nothing
         await databaseService.updateUserMappings(this._user);
-        await databaseService.updateJobLog(this._jobLog);
+
+        await this.updateJobLog([]);
+
         return operationsOk;
     }
 
@@ -90,20 +100,16 @@ export class RemoveObsoleteMappingsJob extends SyncJob {
             if (!syncedService) continue;
             let operationOk = true;
             try {
-                operationOk = await syncedService.deleteServiceObject(mappingObject.id, mappingObject.type);
+                operationOk = await syncedService.deleteServiceObject(mappingObject.id, mappingObject.type, mappingObject.name);
             } catch (ex: any) {
                 if (ex.status === 404) {
                     // service object is missing, it is ok to delete the mapping
                     operationOk = true;
                 } else {
-                    const context = [
-                        this._sentryService.createExtraContext('Mapping to delete', {
-                            'id': mappingObject.id,
-                            'type': mappingObject.type
-                        })
-                    ]
-                    this._sentryService.logError(ex, context);
-                    // console.error('err: ConfigSyncJob: delete; exception');
+                    operationOk &&= ex.response.ok;
+                    if (syncedService.errors.length > 0) {
+                        this._jobLog.errors.push(syncedService.errors[syncedService.errors.length - 1]);
+                    }
                 }
             }
             operationsOk &&= operationOk;
@@ -154,6 +160,9 @@ export class RemoveObsoleteMappingsJob extends SyncJob {
                     const relatedTimeEntriesFromApi = await service.getTimeEntriesRelatedToMappingObjectForUser(mapping, this._user);
                     if (!relatedTimeEntriesFromApi) {
                         operationsOk = false;
+                        if (service.errors.length > 0) {
+                            this._jobLog.errors.push(service.errors[service.errors.length - 1]);
+                        }
                     } else {
                         for (const timeEntryFromApi of relatedTimeEntriesFromApi) {
                             const foundTESO = await databaseService.getTimeEntrySyncedObjectForArchiving(timeEntryFromApi.id, primaryObjectServiceName, this._user._id);
@@ -192,6 +201,9 @@ export class RemoveObsoleteMappingsJob extends SyncJob {
                             await togglService.syncedService.replaceTimeEntryDescription(toggleTimeEntry, timeEntryToArchive.issueName)
                         } catch (exception) {
                             operationsOk = false;
+                            if (togglService.syncedService.errors.length > 0) {
+                                this._jobLog.errors.push(togglService.syncedService.errors[togglService.syncedService.errors.length - 1]);
+                            }
                         }
                     }
 
@@ -201,12 +213,9 @@ export class RemoveObsoleteMappingsJob extends SyncJob {
             }
 
             // and remove all obsolete mappings from user's mappings
-            this._user.mappings
-                = this._user
-                .mappings
-                .filter(
-                    mapping => obsoleteMappings.find(obsoleteMapping => obsoleteMapping === mapping)
-                        === undefined) ?? [];
+            this._user.mappings = this._user.mappings.filter(
+                mapping => obsoleteMappings.find(obsoleteMapping => obsoleteMapping === mapping) === undefined
+            ) ?? [];
         }
         return operationsOk;
     }

src/jobs/sync_job.ts

@@ -2,6 +2,8 @@ import { JobLog } from "../models/job_log";
 import { User } from "../models/user";
 import {SentryService} from "../shared/sentry_service";
 import {ErrorService} from "../shared/error_service";
+import {Timer2TicketError} from "../models/timer2TicketError";
+import {databaseService} from "../shared/database_service";
 
 export abstract class SyncJob {
   protected _user: User;
@@ -34,4 +36,13 @@ export abstract class SyncJob {
    * Does the job, returns true if successfully done, false otherwise and needs to be repeated
    */
   protected abstract _doTheJob(): Promise<boolean>;
+
+  protected async updateJobLog(errors: Timer2TicketError[]) : Promise<void>
+  {
+    this._jobLog.errors = this._jobLog.errors.concat(errors);
+    const updated = await databaseService.updateJobLog(this._jobLog);
+    if (updated instanceof JobLog) {
+      this._jobLog = updated;
+    }
+  }
 }