Skip to content
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

Refactor logging #61

Merged
merged 7 commits into from
Nov 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@ export {NodeKit} from './nodekit';
export {AppContext} from './lib/context';
export {AppConfig, AppContextParams, AppDynamicConfig} from './types';
export {AppError} from './lib/app-error';
export {NodeKitLogger} from './lib/logging';
export * from './lib/public-consts';
58 changes: 39 additions & 19 deletions src/lib/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,21 @@ import {IncomingHttpHeaders} from 'http';

import {JaegerTracer} from 'jaeger-client';
import {FORMAT_HTTP_HEADERS, Span, SpanContext, Tags} from 'opentracing';
import pino from 'pino';

import {NodeKit} from '../nodekit';
import {AppConfig, AppContextParams, AppDynamicConfig, Dict} from '../types';

import {AppError} from './app-error';
import {REQUEST_ID_HEADER, REQUEST_ID_PARAM_NAME} from './consts';
import {extractErrorInfo} from './error-parser';
import {NodeKitLogger} from './logging';

type ContextParams = ContextInitialParams | ContextParentParams;

interface ContextInitialParams {
contextId?: string;
config: AppConfig;
logger: pino.Logger;
logger: NodeKitLogger;
tracer: JaegerTracer;
stats: AppTelemetrySendStats;
parentSpanContext?: SpanContext;
Expand Down Expand Up @@ -55,7 +55,7 @@ export class AppContext {

protected appParams: AppContextParams;
protected name: string;
private logger: pino.Logger;
private logger: NodeKitLogger;
private tracer: JaegerTracer;
private span?: Span;
private startTime: number;
Expand Down Expand Up @@ -115,25 +115,35 @@ export class AppContext {
}

logError(message: string, error?: AppError | Error | unknown, extra?: Dict) {
if (error) {
this.logger.error(
Object.assign({}, this.prepareExtra(extra), extractErrorInfo(error)),
this.prepareLogMessage(message),
);
} else if (extra) {
this.logger.error(this.prepareExtra(extra), this.prepareLogMessage(message));
} else {
this.logger.error(this.loggerExtra, this.prepareLogMessage(message));
}
const preparedMessage = this.prepareLogMessage(message);
const preparedExtra = this.prepareExtra(extra);

const logObject = this.getLogObject(error, extra);

this.logger.error(logObject, preparedMessage);

this.span?.setTag(Tags.SAMPLING_PRIORITY, 1);
this.span?.setTag(Tags.ERROR, true);
this.span?.log(
Object.assign({}, this.prepareExtra(extra), {
event: message,
stack: error instanceof Error && error?.stack,
}),
);
this.span?.log({
...preparedExtra,
event: message,
stack: error instanceof Error && error.stack,
});
}

logWarn(message: string, error?: AppError | Error | unknown, extra?: Dict) {
const preparedMessage = this.prepareLogMessage(message);
const preparedExtra = this.prepareExtra(extra);

const logObject = this.getLogObject(error, extra);

this.logger.warn(logObject, preparedMessage);

this.span?.log({
...preparedExtra,
event: message,
stack: error instanceof Error && error.stack,
});
}

create(name: string, params?: Omit<ContextParentParams, 'parentContext'>) {
Expand Down Expand Up @@ -263,4 +273,14 @@ export class AppContext {
private mergeExtra(extraParent: Dict | undefined, extraCurrent: Dict | undefined) {
return Object.assign({}, extraParent, extraCurrent);
}

private getLogObject(error: Error | unknown, extra: Dict | undefined) {
if (error) {
return {...this.prepareExtra(extra), ...extractErrorInfo(error)};
} else if (extra) {
return this.prepareExtra(extra);
} else {
return this.loggerExtra;
}
}
}
82 changes: 80 additions & 2 deletions src/lib/logging.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,79 @@
import pino from 'pino';
import {Dict} from '../types';

export interface NodeKitLogger {
info(message: string): void;
info(extra: Dict | undefined, message: string): void;
Copy link
Contributor

@resure resure Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't message be first argument (for consistency with context class loggers)? Or it would be better to be consistent with pino logger class methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention was to be consistent with pino first. I think it makes sense to do this rather than reorder the arguments of every call to the logger.


error(message: string): void;
error(extra: Dict | undefined, message: string): void;

warn(message: string): void;
warn(extra: Dict | undefined, message: string): void;

trace(message: string): void;
trace(extra: Dict | undefined, message: string): void;

debug(message: string): void;
debug(extra: Dict | undefined, message: string): void;
}

export class PinoLogger implements NodeKitLogger {
private logger: pino.Logger;

constructor(logger: pino.Logger) {
this.logger = logger;
}
info(message: string): void;
info(extra: Dict | undefined, message: string): void;
info(msgOrExtra: string | Dict | undefined, message?: string): void {
if (typeof msgOrExtra === 'string') {
this.logger.info(message);
} else {
this.logger.info(msgOrExtra, message);
}
}

warn(message: string): void;
warn(extra: Dict | undefined, message: string): void;
warn(msgOrExtra: string | Dict | undefined, message?: string): void {
if (typeof msgOrExtra === 'string') {
this.logger.info(message);
} else {
this.logger.info(msgOrExtra, message);
}
}

error(message: string): void;
error(extra: Dict | undefined, message: string): void;
error(msgOrExtra: string | Dict | undefined, message?: string): void {
if (typeof msgOrExtra === 'string') {
this.logger.error(message);
} else {
this.logger.error(msgOrExtra, message);
}
}

trace(message: string): void;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we use trace and debug somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#60 - will be used here. We're gonna have to merge this one first and rebase the opentracing one on top of this.

trace(extra: Dict | undefined, message: string): void;
trace(msgOrExtra: string | Dict | undefined, message?: string): void {
if (typeof msgOrExtra === 'string') {
this.logger.trace(message);
} else {
this.logger.trace(msgOrExtra, message);
}
}

debug(message: string): void;
debug(extra: Dict | undefined, message: string): void;
debug(msgOrExtra: string | Dict | undefined, message?: string): void {
if (typeof msgOrExtra === 'string') {
this.logger.debug(message);
} else {
this.logger.debug(msgOrExtra, message);
}
}
}

/**
* workaround to provide IntelliSense hints https://stackoverflow.com/a/61048124
Expand Down Expand Up @@ -34,9 +109,12 @@ export function initLogger({appName, devMode, destination, level = 'debug'}: Ini
transport: transportConfig,
};

let pinoInstance: pino.Logger;
if (destination && !devMode) {
return pino(options, destination);
pinoInstance = pino(options, destination);
} else {
return pino(options);
pinoInstance = pino(options);
}

return new PinoLogger(pinoInstance);
}
21 changes: 12 additions & 9 deletions src/nodekit.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import * as dotenv from 'dotenv';
import {JaegerTracer, initTracer} from 'jaeger-client';
import pino from 'pino';

import {NODEKIT_BASE_CONFIG} from './lib/base-config';
import {AppContext} from './lib/context';
import {DynamicConfigPoller, DynamicConfigSetup} from './lib/dynamic-config-poller';
import {loadFileConfigs} from './lib/file-configs';
import {initLogger} from './lib/logging';
import {NodeKitLogger, initLogger} from './lib/logging';
import {prepareClickhouseClient} from './lib/telemetry/clickhouse';
import {isTrueEnvValue} from './lib/utils/is-true-env';
import prepareSensitiveHeadersRedacter, {
Expand Down Expand Up @@ -38,7 +37,7 @@ export class NodeKit {
isTrueEnvValue: typeof isTrueEnvValue;
};

private logger: pino.Logger;
private logger: NodeKitLogger;
private tracer: JaegerTracer;

private shutdownHandlers: ShutdownHandler[];
Expand All @@ -65,12 +64,16 @@ export class NodeKit {
appLoggingLevel: process.env.APP_LOGGING_LEVEL || fileConfig.appLoggingLevel,
});

this.logger = initLogger({
appName: this.config.appName as string,
devMode: appDevMode,
destination: this.config.appLoggingDestination,
level: this.config.appLoggingLevel,
});
if (this.config.appLogger) {
this.logger = this.config.appLogger;
} else {
this.logger = initLogger({
appName: this.config.appName as string,
devMode: appDevMode,
destination: this.config.appLoggingDestination,
level: this.config.appLoggingLevel,
});
}

const redactSensitiveQueryParams = prepareSensitiveQueryParamsRedacter(
this.config.nkDefaultSensitiveQueryParams?.concat(
Expand Down
65 changes: 64 additions & 1 deletion src/tests/logging.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {NodeKit} from '..';
import {NodeKit, NodeKitLogger} from '..';
import {Dict} from '../types';

const setupNodeKit = () => {
const logger = {
Expand Down Expand Up @@ -136,3 +137,65 @@ test('check logging from nested ctx', () => {
msg: `[${ctxName}] log error ${logPostfix}`,
});
});

test('logging with a custom logger', () => {
const warnLog = jest.fn();
const debugLog = jest.fn();
const infoLog = jest.fn();
const errorLog = jest.fn();
const traceLog = jest.fn();
class CustomLogger implements NodeKitLogger {
warn(msgOrObject: string | Dict | undefined, message?: string) {
warnLog(msgOrObject, message);
}
debug(msgOrObject: string | Dict | undefined, message?: string) {
debugLog(msgOrObject, message);
}
info(msgOrObject: string | Dict | undefined, message?: string) {
infoLog(msgOrObject, message);
}
error(msgOrObject: string | Dict | undefined, message?: string) {
errorLog(msgOrObject, message);
}
trace(msgOrObject: string | Dict | undefined, message?: string) {
traceLog(msgOrObject, message);
}
}

const customLogger = new CustomLogger();
const nodekit = new NodeKit({
config: {
appLogger: customLogger,
},
});

const ctx = nodekit.ctx.create('test_ctx');

ctx.log('test');

expect(infoLog).toHaveBeenCalledWith(undefined, '[test_ctx] test');

const err = new Error('test errorLog');
ctx.logError('errorLog message', err);
expect(errorLog).toHaveBeenCalledWith(
{
err: {
message: 'test errorLog',
stack: err.stack,
},
},
'[test_ctx] errorLog message',
);

const warnErr = new Error('test warnLog');
ctx.logWarn('warnLog message', warnErr);
expect(warnLog).toHaveBeenCalledWith(
{
err: {
message: 'test warnLog',
stack: warnErr.stack,
},
},
'[test_ctx] warnLog message',
);
});
3 changes: 2 additions & 1 deletion src/types.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type {pino} from 'pino';

import {REQUEST_ID_PARAM_NAME, USER_ID_PARAM_NAME, USER_LANGUAGE_PARAM_NAME} from './lib/consts';
import type {LoggingLevel} from './lib/logging';
import type {LoggingLevel, NodeKitLogger} from './lib/logging';

export interface AppConfig {
appName?: string;
Expand All @@ -24,6 +24,7 @@ export interface AppConfig {

appLoggingDestination?: pino.DestinationStream;
appLoggingLevel?: LoggingLevel;
appLogger?: NodeKitLogger;

appTracingEnabled?: boolean;
appTracingServiceName?: string;
Expand Down
Loading