From 57cdccf5fa3726e2218a90381052b1e66853d6be Mon Sep 17 00:00:00 2001 From: Phillip Wirth Date: Wed, 23 Oct 2024 18:19:07 +0200 Subject: [PATCH 1/5] BC-8308 add warning if an error occurs during healtch check --- .../src/modules/health/health-api.module.ts | 2 ++ .../server/src/modules/health/uc/health.uc.ts | 26 +++++++++++++++++-- .../internal-server/internal-server.module.ts | 5 +++- 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/apps/server/src/modules/health/health-api.module.ts b/apps/server/src/modules/health/health-api.module.ts index 21cb4da0302..b2bc135cf98 100644 --- a/apps/server/src/modules/health/health-api.module.ts +++ b/apps/server/src/modules/health/health-api.module.ts @@ -1,11 +1,13 @@ import { Module } from '@nestjs/common'; +import { LoggerModule } from '@src/core/logger'; import { HealthController } from './controller'; import { HealthCheckRepo } from './repo'; import { HealthService } from './service'; import { HealthUC } from './uc'; @Module({ + imports: [LoggerModule], controllers: [HealthController], providers: [HealthCheckRepo, HealthService, HealthUC], }) diff --git a/apps/server/src/modules/health/uc/health.uc.ts b/apps/server/src/modules/health/uc/health.uc.ts index a50767dfd98..2603f1d4589 100644 --- a/apps/server/src/modules/health/uc/health.uc.ts +++ b/apps/server/src/modules/health/uc/health.uc.ts @@ -1,5 +1,7 @@ import { Injectable } from '@nestjs/common'; +import { Loggable, LoggableMessage } from '@shared/common/loggable'; +import { Logger } from '@src/core/logger'; import { HealthService } from '../service'; import { HealthConfig } from '../health.config'; import { HealthStatuses, HealthStatusCheck, HealthStatus } from '../domain'; @@ -25,9 +27,28 @@ function hasMessage(error: unknown): error is { message: string } { ); } +class HealthCheckErrorLoggable implements Loggable { + constructor(private readonly error: unknown) {} + + getLogMessage(): LoggableMessage { + if (this.error instanceof Error) { + return { + message: this.error.message, + stack: this.error.stack, + }; + } + return { + message: String(this.error), + stack: undefined, + }; + } +} + @Injectable() export class HealthUC { - constructor(private readonly healthService: HealthService) {} + constructor(private readonly healthService: HealthService, private readonly logger: Logger) { + this.logger.setContext(HealthUC.name); + } checkSelfHealth(): HealthStatus { // This health check verifies just the correct module setup and doesn't include @@ -43,7 +64,7 @@ export class HealthUC { async checkOverallHealth(): Promise { // The below check allows for turning off the MongoDB dependency on the health check - // it shouldn't be typically used, but if this health check will be used e.g. in the k8s - // liveness or readiness probes and, for any reason, there would be a need to stop + // liveliness or readiness probes and, for any reason, there would be a need to stop // including MongoDB check in the overall API health checks, the HEALTH_CHECKS_EXCLUDE_MONGODB // config var can be set to 'true' to disable it. This way, as currently only this single // MongoDB check is included in the overall API health checks, the whole health check will @@ -70,6 +91,7 @@ export class HealthUC { await this.healthService.upsertHealthCheckById(healthCheckID); } catch (error) { + this.logger.warning(new HealthCheckErrorLoggable(error)); // If any error occurred in the database operation execution it should be indicated // as a MongoDB check failure (and thus the whole health check should fail). diff --git a/apps/server/src/modules/internal-server/internal-server.module.ts b/apps/server/src/modules/internal-server/internal-server.module.ts index dae816e533a..40887e59004 100644 --- a/apps/server/src/modules/internal-server/internal-server.module.ts +++ b/apps/server/src/modules/internal-server/internal-server.module.ts @@ -1,8 +1,10 @@ import { Module } from '@nestjs/common'; import { MikroOrmModule } from '@mikro-orm/nestjs'; -import { DB_URL, DB_USERNAME, DB_PASSWORD } from '@src/config'; +import { DB_URL, DB_USERNAME, DB_PASSWORD, createConfigModuleOptions } from '@src/config'; import { HealthApiModule, HealthEntities } from '@src/modules/health'; +import { ConfigModule } from '@nestjs/config'; +import { serverConfig } from '@modules/server'; @Module({ imports: [ @@ -16,6 +18,7 @@ import { HealthApiModule, HealthEntities } from '@src/modules/health'; // debug: true, // use it only for the local queries debugging }), HealthApiModule, + ConfigModule.forRoot(createConfigModuleOptions(serverConfig)), ], }) export class InternalServerModule {} From 0b8bc4175ba63d4a98be290f01139f9b6449bad2 Mon Sep 17 00:00:00 2001 From: Phillip Wirth Date: Thu, 24 Oct 2024 10:06:12 +0200 Subject: [PATCH 2/5] generic error loggable --- .../loggables/generic-error-loggable.ts | 18 ++++++++++++++++ .../server/src/core/logger/loggables/index.ts | 1 + .../server/src/modules/health/uc/health.uc.ts | 21 ++----------------- 3 files changed, 21 insertions(+), 19 deletions(-) create mode 100644 apps/server/src/core/logger/loggables/generic-error-loggable.ts create mode 100644 apps/server/src/core/logger/loggables/index.ts diff --git a/apps/server/src/core/logger/loggables/generic-error-loggable.ts b/apps/server/src/core/logger/loggables/generic-error-loggable.ts new file mode 100644 index 00000000000..918acb5600f --- /dev/null +++ b/apps/server/src/core/logger/loggables/generic-error-loggable.ts @@ -0,0 +1,18 @@ +import { Loggable, LoggableMessage } from '@shared/common/loggable'; + +export class GenericErrorLoggable implements Loggable { + constructor(private readonly error: unknown) {} + + getLogMessage(): LoggableMessage { + if (this.error instanceof Error) { + return { + message: this.error.message, + stack: this.error.stack, + }; + } + return { + message: String(this.error), + stack: undefined, + }; + } +} diff --git a/apps/server/src/core/logger/loggables/index.ts b/apps/server/src/core/logger/loggables/index.ts new file mode 100644 index 00000000000..221094b5b14 --- /dev/null +++ b/apps/server/src/core/logger/loggables/index.ts @@ -0,0 +1 @@ +export * from './generic-error-loggable'; diff --git a/apps/server/src/modules/health/uc/health.uc.ts b/apps/server/src/modules/health/uc/health.uc.ts index 2603f1d4589..d1a6583569d 100644 --- a/apps/server/src/modules/health/uc/health.uc.ts +++ b/apps/server/src/modules/health/uc/health.uc.ts @@ -1,7 +1,7 @@ import { Injectable } from '@nestjs/common'; -import { Loggable, LoggableMessage } from '@shared/common/loggable'; import { Logger } from '@src/core/logger'; +import { GenericErrorLoggable } from '@src/core/logger/loggables'; import { HealthService } from '../service'; import { HealthConfig } from '../health.config'; import { HealthStatuses, HealthStatusCheck, HealthStatus } from '../domain'; @@ -27,23 +27,6 @@ function hasMessage(error: unknown): error is { message: string } { ); } -class HealthCheckErrorLoggable implements Loggable { - constructor(private readonly error: unknown) {} - - getLogMessage(): LoggableMessage { - if (this.error instanceof Error) { - return { - message: this.error.message, - stack: this.error.stack, - }; - } - return { - message: String(this.error), - stack: undefined, - }; - } -} - @Injectable() export class HealthUC { constructor(private readonly healthService: HealthService, private readonly logger: Logger) { @@ -91,7 +74,7 @@ export class HealthUC { await this.healthService.upsertHealthCheckById(healthCheckID); } catch (error) { - this.logger.warning(new HealthCheckErrorLoggable(error)); + this.logger.warning(new GenericErrorLoggable(error)); // If any error occurred in the database operation execution it should be indicated // as a MongoDB check failure (and thus the whole health check should fail). From 31a72ab083b5c014721d886747e2ec347d813015 Mon Sep 17 00:00:00 2001 From: Phillip Wirth Date: Thu, 24 Oct 2024 10:42:35 +0200 Subject: [PATCH 3/5] fixed tests --- apps/server/src/modules/health/uc/health.uc.spec.ts | 5 +++++ .../internal-server/internal-server-test.module.ts | 9 ++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/apps/server/src/modules/health/uc/health.uc.spec.ts b/apps/server/src/modules/health/uc/health.uc.spec.ts index 95265162a94..7a07007307c 100644 --- a/apps/server/src/modules/health/uc/health.uc.spec.ts +++ b/apps/server/src/modules/health/uc/health.uc.spec.ts @@ -7,6 +7,7 @@ import { HealthUC } from './health.uc'; import { HealthService } from '../service'; import { HealthStatuses } from '../domain'; import { HealthConfig } from '../health.config'; +import { Logger } from '@src/core/logger'; describe(HealthUC.name, () => { let configBefore: IConfig; @@ -23,6 +24,10 @@ describe(HealthUC.name, () => { provide: HealthService, useValue: createMock(), }, + { + provide: Logger, + useValue: createMock(), + }, ], }).compile(); uc = module.get(HealthUC); diff --git a/apps/server/src/modules/internal-server/internal-server-test.module.ts b/apps/server/src/modules/internal-server/internal-server-test.module.ts index 5b565be67a2..14b8196ac02 100644 --- a/apps/server/src/modules/internal-server/internal-server-test.module.ts +++ b/apps/server/src/modules/internal-server/internal-server-test.module.ts @@ -2,6 +2,9 @@ import { Module } from '@nestjs/common'; import { MongoMemoryDatabaseModule } from '@infra/database'; import { HealthApiModule, HealthEntities } from '@src/modules/health'; +import { ConfigModule } from '@nestjs/config'; +import { createConfigModuleOptions } from '@src/config'; +import { serverConfig } from '@modules/server'; /** * Internal server module used for testing. @@ -9,6 +12,10 @@ import { HealthApiModule, HealthEntities } from '@src/modules/health'; * the in-memory MongoDB implementation for testing purposes. */ @Module({ - imports: [MongoMemoryDatabaseModule.forRoot({ entities: [...HealthEntities] }), HealthApiModule], + imports: [ + MongoMemoryDatabaseModule.forRoot({ entities: [...HealthEntities] }), + HealthApiModule, + ConfigModule.forRoot(createConfigModuleOptions(serverConfig)), + ], }) export class InternalServerTestModule {} From f70eed583b1484db339e5b9d1c1b81b91da7f925 Mon Sep 17 00:00:00 2001 From: Phillip Wirth Date: Thu, 24 Oct 2024 15:46:03 +0200 Subject: [PATCH 4/5] BC-8308 move generic-error-loggable again --- apps/server/src/core/logger/loggables/index.ts | 1 - apps/server/src/modules/health/uc/health.uc.ts | 2 +- .../common/loggable}/generic-error-loggable.ts | 2 +- apps/server/src/shared/common/loggable/index.ts | 1 + 4 files changed, 3 insertions(+), 3 deletions(-) delete mode 100644 apps/server/src/core/logger/loggables/index.ts rename apps/server/src/{core/logger/loggables => shared/common/loggable}/generic-error-loggable.ts (97%) diff --git a/apps/server/src/core/logger/loggables/index.ts b/apps/server/src/core/logger/loggables/index.ts deleted file mode 100644 index 221094b5b14..00000000000 --- a/apps/server/src/core/logger/loggables/index.ts +++ /dev/null @@ -1 +0,0 @@ -export * from './generic-error-loggable'; diff --git a/apps/server/src/modules/health/uc/health.uc.ts b/apps/server/src/modules/health/uc/health.uc.ts index d1a6583569d..00f3b533dbc 100644 --- a/apps/server/src/modules/health/uc/health.uc.ts +++ b/apps/server/src/modules/health/uc/health.uc.ts @@ -1,7 +1,7 @@ import { Injectable } from '@nestjs/common'; import { Logger } from '@src/core/logger'; -import { GenericErrorLoggable } from '@src/core/logger/loggables'; +import { GenericErrorLoggable } from '@shared/common/loggable'; import { HealthService } from '../service'; import { HealthConfig } from '../health.config'; import { HealthStatuses, HealthStatusCheck, HealthStatus } from '../domain'; diff --git a/apps/server/src/core/logger/loggables/generic-error-loggable.ts b/apps/server/src/shared/common/loggable/generic-error-loggable.ts similarity index 97% rename from apps/server/src/core/logger/loggables/generic-error-loggable.ts rename to apps/server/src/shared/common/loggable/generic-error-loggable.ts index 918acb5600f..eb919f72d4e 100644 --- a/apps/server/src/core/logger/loggables/generic-error-loggable.ts +++ b/apps/server/src/shared/common/loggable/generic-error-loggable.ts @@ -1,4 +1,4 @@ -import { Loggable, LoggableMessage } from '@shared/common/loggable'; +import { Loggable, LoggableMessage } from '@shared/common/loggable/index'; export class GenericErrorLoggable implements Loggable { constructor(private readonly error: unknown) {} diff --git a/apps/server/src/shared/common/loggable/index.ts b/apps/server/src/shared/common/loggable/index.ts index 41cf4b21ac5..fb56190ba36 100644 --- a/apps/server/src/shared/common/loggable/index.ts +++ b/apps/server/src/shared/common/loggable/index.ts @@ -1,2 +1,3 @@ export { ReferencedEntityNotFoundLoggable } from './referenced-entity-not-found-loggable'; export { Loggable, LoggableMessage } from './interfaces'; +export { GenericErrorLoggable } from './generic-error-loggable'; From 9392a3fbb281913f236d4d0d958350fc792263a4 Mon Sep 17 00:00:00 2001 From: Phillip Wirth Date: Thu, 24 Oct 2024 17:07:04 +0200 Subject: [PATCH 5/5] good good? --- .../error/loggable/error.loggable.spec.ts | 13 ++++++++++- .../src/core/error/loggable/error.loggable.ts | 23 +++++++++++++------ .../src/modules/health/uc/health.uc.spec.ts | 2 +- .../server/src/modules/health/uc/health.uc.ts | 4 ++-- .../common/loggable/generic-error-loggable.ts | 18 --------------- .../src/shared/common/loggable/index.ts | 1 - 6 files changed, 31 insertions(+), 30 deletions(-) delete mode 100644 apps/server/src/shared/common/loggable/generic-error-loggable.ts diff --git a/apps/server/src/core/error/loggable/error.loggable.spec.ts b/apps/server/src/core/error/loggable/error.loggable.spec.ts index 79527f9a687..ca51a241871 100644 --- a/apps/server/src/core/error/loggable/error.loggable.spec.ts +++ b/apps/server/src/core/error/loggable/error.loggable.spec.ts @@ -1,7 +1,7 @@ import { NotFound } from '@feathersjs/errors'; import { BadRequestException, HttpStatus, ValidationError } from '@nestjs/common'; import { ApiProperty } from '@nestjs/swagger'; -import { ApiValidationError, BusinessError } from '@shared/common'; +import { ApiValidationError, BusinessError, ErrorLogMessage } from '@shared/common'; import { PrivacyProtect } from '@shared/controller'; import { ErrorLoggable } from './error.loggable'; @@ -158,5 +158,16 @@ describe('ErrorLoggable', () => { expect(message).toEqual(expectedMessage); }); }); + + describe('when error is of unknown type', () => { + it('should return ErrorLogMessage with appropriate error type', () => { + const errorLoggable = new ErrorLoggable('something broke'); + + const message = errorLoggable.getLogMessage() as ErrorLogMessage; + expect(message.data).toBeUndefined(); + expect(message.error).toBeInstanceOf(Error); + expect(message.type).toBe('Unhandled or Unknown Error'); + }); + }); }); }); diff --git a/apps/server/src/core/error/loggable/error.loggable.ts b/apps/server/src/core/error/loggable/error.loggable.ts index b13f100a1f9..4eaf33793de 100644 --- a/apps/server/src/core/error/loggable/error.loggable.ts +++ b/apps/server/src/core/error/loggable/error.loggable.ts @@ -1,29 +1,38 @@ import { ValidationError } from '@nestjs/common'; import { ApiValidationError } from '@shared/common'; import { getMetadataStorage } from 'class-validator'; +import util from 'util'; import { Loggable } from '../../logger/interfaces'; import { ErrorLogMessage, LogMessageDataObject, ValidationErrorLogMessage } from '../../logger/types'; import { ErrorUtils } from '../utils/error.utils'; export class ErrorLoggable implements Loggable { - constructor(private readonly error: Error, private readonly data?: LogMessageDataObject) {} + readonly actualError: Error; + + constructor(private readonly error: unknown, private readonly data?: LogMessageDataObject) { + if (this.error instanceof Error) { + this.actualError = error; + } else { + this.actualError = new Error(util.inspect(error)); + } + } private readonly classValidatorMetadataStorage = getMetadataStorage(); getLogMessage(): ErrorLogMessage | ValidationErrorLogMessage { let logMessage: ErrorLogMessage | ValidationErrorLogMessage = { - error: this.error, + error: this.actualError, type: '', data: this.data, }; - if (this.error instanceof ApiValidationError) { - logMessage = this.createLogMessageForValidationErrors(this.error); - } else if (ErrorUtils.isFeathersError(this.error)) { + if (this.actualError instanceof ApiValidationError) { + logMessage = this.createLogMessageForValidationErrors(this.actualError); + } else if (ErrorUtils.isFeathersError(this.actualError)) { logMessage.type = 'Feathers Error'; - } else if (ErrorUtils.isBusinessError(this.error)) { + } else if (ErrorUtils.isBusinessError(this.actualError)) { logMessage.type = 'Business Error'; - } else if (ErrorUtils.isNestHttpException(this.error)) { + } else if (ErrorUtils.isNestHttpException(this.actualError)) { logMessage.type = 'Technical Error'; } else { logMessage.type = 'Unhandled or Unknown Error'; diff --git a/apps/server/src/modules/health/uc/health.uc.spec.ts b/apps/server/src/modules/health/uc/health.uc.spec.ts index 7a07007307c..591c4aa5b65 100644 --- a/apps/server/src/modules/health/uc/health.uc.spec.ts +++ b/apps/server/src/modules/health/uc/health.uc.spec.ts @@ -3,11 +3,11 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { IConfig } from '@hpi-schul-cloud/commons/lib/interfaces/IConfig'; import { Configuration } from '@hpi-schul-cloud/commons'; +import { Logger } from '@src/core/logger'; import { HealthUC } from './health.uc'; import { HealthService } from '../service'; import { HealthStatuses } from '../domain'; import { HealthConfig } from '../health.config'; -import { Logger } from '@src/core/logger'; describe(HealthUC.name, () => { let configBefore: IConfig; diff --git a/apps/server/src/modules/health/uc/health.uc.ts b/apps/server/src/modules/health/uc/health.uc.ts index 00f3b533dbc..fedf18c1527 100644 --- a/apps/server/src/modules/health/uc/health.uc.ts +++ b/apps/server/src/modules/health/uc/health.uc.ts @@ -1,7 +1,7 @@ import { Injectable } from '@nestjs/common'; import { Logger } from '@src/core/logger'; -import { GenericErrorLoggable } from '@shared/common/loggable'; +import { ErrorLoggable } from '@src/core/error/loggable'; import { HealthService } from '../service'; import { HealthConfig } from '../health.config'; import { HealthStatuses, HealthStatusCheck, HealthStatus } from '../domain'; @@ -74,7 +74,7 @@ export class HealthUC { await this.healthService.upsertHealthCheckById(healthCheckID); } catch (error) { - this.logger.warning(new GenericErrorLoggable(error)); + this.logger.warning(new ErrorLoggable(error)); // If any error occurred in the database operation execution it should be indicated // as a MongoDB check failure (and thus the whole health check should fail). diff --git a/apps/server/src/shared/common/loggable/generic-error-loggable.ts b/apps/server/src/shared/common/loggable/generic-error-loggable.ts deleted file mode 100644 index eb919f72d4e..00000000000 --- a/apps/server/src/shared/common/loggable/generic-error-loggable.ts +++ /dev/null @@ -1,18 +0,0 @@ -import { Loggable, LoggableMessage } from '@shared/common/loggable/index'; - -export class GenericErrorLoggable implements Loggable { - constructor(private readonly error: unknown) {} - - getLogMessage(): LoggableMessage { - if (this.error instanceof Error) { - return { - message: this.error.message, - stack: this.error.stack, - }; - } - return { - message: String(this.error), - stack: undefined, - }; - } -} diff --git a/apps/server/src/shared/common/loggable/index.ts b/apps/server/src/shared/common/loggable/index.ts index fb56190ba36..41cf4b21ac5 100644 --- a/apps/server/src/shared/common/loggable/index.ts +++ b/apps/server/src/shared/common/loggable/index.ts @@ -1,3 +1,2 @@ export { ReferencedEntityNotFoundLoggable } from './referenced-entity-not-found-loggable'; export { Loggable, LoggableMessage } from './interfaces'; -export { GenericErrorLoggable } from './generic-error-loggable';