From 9e9fea1659a62cceb5437e350a75fc75f36b4266 Mon Sep 17 00:00:00 2001 From: Pierre Gayvallet Date: Mon, 16 Dec 2019 12:25:45 +0100 Subject: [PATCH] Allow loggers to create child loggers via 'get' method (#52605) (#53106) * add 'Logger.get' method * updates generated doc * resolve merge conflicts --- .../server/kibana-plugin-server.logger.md | 1 + src/core/server/logging/logger.mock.ts | 46 +++++++++++++++++++ src/core/server/logging/logger.test.ts | 27 +++++++++-- src/core/server/logging/logger.ts | 19 +++++++- .../server/logging/logger_adapter.test.ts | 7 +++ src/core/server/logging/logger_adapter.ts | 4 ++ .../server/logging/logging_service.mock.ts | 22 ++++----- src/core/server/logging/logging_service.ts | 13 ++---- src/core/server/server.api.md | 1 + .../signals/__mocks__/es_results.ts | 11 +---- .../spaces_tutorial_context_factory.test.ts | 16 +++---- .../spaces_service/spaces_service.test.ts | 17 +++---- 12 files changed, 129 insertions(+), 55 deletions(-) create mode 100644 src/core/server/logging/logger.mock.ts diff --git a/docs/development/core/server/kibana-plugin-server.logger.md b/docs/development/core/server/kibana-plugin-server.logger.md index 96f327a798485..068f51f409f09 100644 --- a/docs/development/core/server/kibana-plugin-server.logger.md +++ b/docs/development/core/server/kibana-plugin-server.logger.md @@ -19,6 +19,7 @@ export interface Logger | [debug(message, meta)](./kibana-plugin-server.logger.debug.md) | Log messages useful for debugging and interactive investigation | | [error(errorOrMessage, meta)](./kibana-plugin-server.logger.error.md) | Logs abnormal or unexpected errors or messages that caused a failure in the application flow | | [fatal(errorOrMessage, meta)](./kibana-plugin-server.logger.fatal.md) | Logs abnormal or unexpected errors or messages that caused an unrecoverable failure | +| [get(childContextPaths)](./kibana-plugin-server.logger.get.md) | Returns a new [Logger](./kibana-plugin-server.logger.md) instance extending the current logger context. | | [info(message, meta)](./kibana-plugin-server.logger.info.md) | Logs messages related to general application flow | | [trace(message, meta)](./kibana-plugin-server.logger.trace.md) | Log messages at the most detailed log level | | [warn(errorOrMessage, meta)](./kibana-plugin-server.logger.warn.md) | Logs abnormal or unexpected errors or messages | diff --git a/src/core/server/logging/logger.mock.ts b/src/core/server/logging/logger.mock.ts new file mode 100644 index 0000000000000..a3bb07ea4c095 --- /dev/null +++ b/src/core/server/logging/logger.mock.ts @@ -0,0 +1,46 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Logger } from './logger'; + +export type MockedLogger = jest.Mocked & { context: string[] }; + +const createLoggerMock = (context: string[] = []) => { + const mockLog: MockedLogger = { + context, + debug: jest.fn(), + error: jest.fn(), + fatal: jest.fn(), + info: jest.fn(), + log: jest.fn(), + trace: jest.fn(), + warn: jest.fn(), + get: jest.fn(), + }; + mockLog.get.mockImplementation((...ctx) => ({ + ctx, + ...mockLog, + })); + + return mockLog; +}; + +export const loggerMock = { + create: createLoggerMock, +}; diff --git a/src/core/server/logging/logger.test.ts b/src/core/server/logging/logger.test.ts index 40c310c4e94c7..026e24fc5df54 100644 --- a/src/core/server/logging/logger.test.ts +++ b/src/core/server/logging/logger.test.ts @@ -25,15 +25,20 @@ import { BaseLogger } from './logger'; const context = LoggingConfig.getLoggerContext(['context', 'parent', 'child']); let appenderMocks: Appender[]; let logger: BaseLogger; +const factory = { + get: jest.fn().mockImplementation(() => logger), +}; + const timestamp = new Date(2012, 1, 1); beforeEach(() => { jest.spyOn(global, 'Date').mockImplementation(() => timestamp); appenderMocks = [{ append: jest.fn() }, { append: jest.fn() }]; - logger = new BaseLogger(context, LogLevel.All, appenderMocks); + logger = new BaseLogger(context, LogLevel.All, appenderMocks, factory); }); afterEach(() => { + jest.resetAllMocks(); jest.restoreAllMocks(); }); @@ -263,8 +268,22 @@ test('`log()` just passes the record to all appenders.', () => { } }); +test('`get()` calls the logger factory with proper context and return the result', () => { + logger.get('sub', 'context'); + expect(factory.get).toHaveBeenCalledTimes(1); + expect(factory.get).toHaveBeenCalledWith(context, 'sub', 'context'); + + factory.get.mockClear(); + factory.get.mockImplementation(() => 'some-logger'); + + const childLogger = logger.get('other', 'sub'); + expect(factory.get).toHaveBeenCalledTimes(1); + expect(factory.get).toHaveBeenCalledWith(context, 'other', 'sub'); + expect(childLogger).toEqual('some-logger'); +}); + test('logger with `Off` level does not pass any records to appenders.', () => { - const turnedOffLogger = new BaseLogger(context, LogLevel.Off, appenderMocks); + const turnedOffLogger = new BaseLogger(context, LogLevel.Off, appenderMocks, factory); turnedOffLogger.trace('trace-message'); turnedOffLogger.debug('debug-message'); turnedOffLogger.info('info-message'); @@ -278,7 +297,7 @@ test('logger with `Off` level does not pass any records to appenders.', () => { }); test('logger with `All` level passes all records to appenders.', () => { - const catchAllLogger = new BaseLogger(context, LogLevel.All, appenderMocks); + const catchAllLogger = new BaseLogger(context, LogLevel.All, appenderMocks, factory); catchAllLogger.trace('trace-message'); for (const appenderMock of appenderMocks) { @@ -348,7 +367,7 @@ test('logger with `All` level passes all records to appenders.', () => { }); test('passes log record to appenders only if log level is supported.', () => { - const warnLogger = new BaseLogger(context, LogLevel.Warn, appenderMocks); + const warnLogger = new BaseLogger(context, LogLevel.Warn, appenderMocks, factory); warnLogger.trace('trace-message'); warnLogger.debug('debug-message'); diff --git a/src/core/server/logging/logger.ts b/src/core/server/logging/logger.ts index e10e79d5cf45b..ac79c1916c07b 100644 --- a/src/core/server/logging/logger.ts +++ b/src/core/server/logging/logger.ts @@ -20,6 +20,7 @@ import { Appender } from './appenders/appenders'; import { LogLevel } from './log_level'; import { LogRecord } from './log_record'; +import { LoggerFactory } from './logger_factory'; /** * Contextual metadata @@ -84,6 +85,17 @@ export interface Logger { /** @internal */ log(record: LogRecord): void; + + /** + * Returns a new {@link Logger} instance extending the current logger context. + * + * @example + * ```typescript + * const logger = loggerFactory.get('plugin', 'service'); // 'plugin.service' context + * const subLogger = logger.get('feature'); // 'plugin.service.feature' context + * ``` + */ + get(...childContextPaths: string[]): Logger; } function isError(x: any): x is Error { @@ -95,7 +107,8 @@ export class BaseLogger implements Logger { constructor( private readonly context: string, private readonly level: LogLevel, - private readonly appenders: Appender[] + private readonly appenders: Appender[], + private readonly factory: LoggerFactory ) {} public trace(message: string, meta?: LogMeta): void { @@ -132,6 +145,10 @@ export class BaseLogger implements Logger { } } + public get(...childContextPaths: string[]): Logger { + return this.factory.get(...[this.context, ...childContextPaths]); + } + private createLogRecord( level: LogLevel, errorOrMessage: string | Error, diff --git a/src/core/server/logging/logger_adapter.test.ts b/src/core/server/logging/logger_adapter.test.ts index 075e8f4d47ffe..c4d7a56e074a7 100644 --- a/src/core/server/logging/logger_adapter.test.ts +++ b/src/core/server/logging/logger_adapter.test.ts @@ -29,6 +29,7 @@ test('proxies all method calls to the internal logger.', () => { log: jest.fn(), trace: jest.fn(), warn: jest.fn(), + get: jest.fn(), }; const adapter = new LoggerAdapter(internalLogger); @@ -56,6 +57,10 @@ test('proxies all method calls to the internal logger.', () => { adapter.fatal('fatal-message'); expect(internalLogger.fatal).toHaveBeenCalledTimes(1); expect(internalLogger.fatal).toHaveBeenCalledWith('fatal-message', undefined); + + adapter.get('context'); + expect(internalLogger.get).toHaveBeenCalledTimes(1); + expect(internalLogger.get).toHaveBeenCalledWith('context'); }); test('forwards all method calls to new internal logger if it is updated.', () => { @@ -67,6 +72,7 @@ test('forwards all method calls to new internal logger if it is updated.', () => log: jest.fn(), trace: jest.fn(), warn: jest.fn(), + get: jest.fn(), }; const newInternalLogger: Logger = { @@ -77,6 +83,7 @@ test('forwards all method calls to new internal logger if it is updated.', () => log: jest.fn(), trace: jest.fn(), warn: jest.fn(), + get: jest.fn(), }; const adapter = new LoggerAdapter(oldInternalLogger); diff --git a/src/core/server/logging/logger_adapter.ts b/src/core/server/logging/logger_adapter.ts index ffc212631e7b4..14e5712e55c58 100644 --- a/src/core/server/logging/logger_adapter.ts +++ b/src/core/server/logging/logger_adapter.ts @@ -63,4 +63,8 @@ export class LoggerAdapter implements Logger { public log(record: LogRecord) { this.logger.log(record); } + + public get(...contextParts: string[]): Logger { + return this.logger.get(...contextParts); + } } diff --git a/src/core/server/logging/logging_service.mock.ts b/src/core/server/logging/logging_service.mock.ts index 50e6edc227bb5..15d66c2e8535c 100644 --- a/src/core/server/logging/logging_service.mock.ts +++ b/src/core/server/logging/logging_service.mock.ts @@ -18,22 +18,17 @@ */ // Test helpers to simplify mocking logs and collecting all their outputs -import { Logger } from './logger'; import { ILoggingService } from './logging_service'; import { LoggerFactory } from './logger_factory'; - -type MockedLogger = jest.Mocked; +import { loggerMock, MockedLogger } from './logger.mock'; const createLoggingServiceMock = () => { - const mockLog: MockedLogger = { - debug: jest.fn(), - error: jest.fn(), - fatal: jest.fn(), - info: jest.fn(), - log: jest.fn(), - trace: jest.fn(), - warn: jest.fn(), - }; + const mockLog = loggerMock.create(); + + mockLog.get.mockImplementation((...context) => ({ + ...mockLog, + context, + })); const mocked: jest.Mocked = { get: jest.fn(), @@ -42,8 +37,8 @@ const createLoggingServiceMock = () => { stop: jest.fn(), }; mocked.get.mockImplementation((...context) => ({ - context, ...mockLog, + context, })); mocked.asLoggerFactory.mockImplementation(() => mocked); mocked.stop.mockResolvedValue(); @@ -84,4 +79,5 @@ export const loggingServiceMock = { create: createLoggingServiceMock, collect: collectLoggingServiceMock, clear: clearLoggingServiceMock, + createLogger: loggerMock.create, }; diff --git a/src/core/server/logging/logging_service.ts b/src/core/server/logging/logging_service.ts index ada02c3b6901a..f9535e6c8283e 100644 --- a/src/core/server/logging/logging_service.ts +++ b/src/core/server/logging/logging_service.ts @@ -37,12 +37,9 @@ export class LoggingService implements LoggerFactory { public get(...contextParts: string[]): Logger { const context = LoggingConfig.getLoggerContext(contextParts); - if (this.loggers.has(context)) { - return this.loggers.get(context)!; + if (!this.loggers.has(context)) { + this.loggers.set(context, new LoggerAdapter(this.createLogger(context, this.config))); } - - this.loggers.set(context, new LoggerAdapter(this.createLogger(context, this.config))); - return this.loggers.get(context)!; } @@ -55,7 +52,7 @@ export class LoggingService implements LoggerFactory { /** * Updates all current active loggers with the new config values. - * @param config New config instance. + * @param rawConfig New config instance. */ public upgrade(rawConfig: LoggingConfigType) { const config = new LoggingConfig(rawConfig); @@ -106,14 +103,14 @@ export class LoggingService implements LoggerFactory { if (config === undefined) { // If we don't have config yet, use `buffered` appender that will store all logged messages in the memory // until the config is ready. - return new BaseLogger(context, LogLevel.All, [this.bufferAppender]); + return new BaseLogger(context, LogLevel.All, [this.bufferAppender], this.asLoggerFactory()); } const { level, appenders } = this.getLoggerConfigByContext(config, context); const loggerLevel = LogLevel.fromId(level); const loggerAppenders = appenders.map(appenderKey => this.appenders.get(appenderKey)!); - return new BaseLogger(context, loggerLevel, loggerAppenders); + return new BaseLogger(context, loggerLevel, loggerAppenders, this.asLoggerFactory()); } private getLoggerConfigByContext(config: LoggingConfig, context: string): LoggerConfigType { diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 18e76324ff309..13ca4b582c98e 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -903,6 +903,7 @@ export interface Logger { debug(message: string, meta?: LogMeta): void; error(errorOrMessage: string | Error, meta?: LogMeta): void; fatal(errorOrMessage: string | Error, meta?: LogMeta): void; + get(...childContextPaths: string[]): Logger; info(message: string, meta?: LogMeta): void; // @internal (undocumented) log(record: LogRecord): void; diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/__mocks__/es_results.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/__mocks__/es_results.ts index 215d9da6eb7ff..47a4dafa2d17f 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/__mocks__/es_results.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/__mocks__/es_results.ts @@ -6,6 +6,7 @@ import { SignalSourceHit, SignalSearchResponse } from '../types'; import { Logger } from 'kibana/server'; +import { loggingServiceMock } from '../../../../../../../../../src/core/server/mocks'; import { RuleTypeParams, OutputRuleAlertRest } from '../../types'; export const sampleRuleAlertParams = ( @@ -279,12 +280,4 @@ export const sampleRule = (): Partial => { }; }; -export const mockLogger: Logger = { - log: jest.fn(), - trace: jest.fn(), - debug: jest.fn(), - info: jest.fn(), - warn: jest.fn(), - error: jest.fn(), - fatal: jest.fn(), -}; +export const mockLogger: Logger = loggingServiceMock.createLogger(); diff --git a/x-pack/plugins/spaces/server/lib/spaces_tutorial_context_factory.test.ts b/x-pack/plugins/spaces/server/lib/spaces_tutorial_context_factory.test.ts index b000c767b53e8..9e1bb5cf9f7d6 100644 --- a/x-pack/plugins/spaces/server/lib/spaces_tutorial_context_factory.test.ts +++ b/x-pack/plugins/spaces/server/lib/spaces_tutorial_context_factory.test.ts @@ -10,21 +10,17 @@ import { createSpacesTutorialContextFactory } from './spaces_tutorial_context_fa import { SpacesService } from '../spaces_service'; import { SavedObjectsLegacyService } from 'src/core/server'; import { SpacesAuditLogger } from './audit_logger'; -import { elasticsearchServiceMock, coreMock } from '../../../../../src/core/server/mocks'; +import { + elasticsearchServiceMock, + coreMock, + loggingServiceMock, +} from '../../../../../src/core/server/mocks'; import { spacesServiceMock } from '../spaces_service/spaces_service.mock'; import { LegacyAPI } from '../plugin'; import { spacesConfig } from './__fixtures__'; import { securityMock } from '../../../security/server/mocks'; -const log = { - log: jest.fn(), - trace: jest.fn(), - debug: jest.fn(), - info: jest.fn(), - warn: jest.fn(), - error: jest.fn(), - fatal: jest.fn(), -}; +const log = loggingServiceMock.createLogger(); const legacyAPI: LegacyAPI = { legacyConfig: {}, diff --git a/x-pack/plugins/spaces/server/spaces_service/spaces_service.test.ts b/x-pack/plugins/spaces/server/spaces_service/spaces_service.test.ts index 73791201185e8..30ad3f399916b 100644 --- a/x-pack/plugins/spaces/server/spaces_service/spaces_service.test.ts +++ b/x-pack/plugins/spaces/server/spaces_service/spaces_service.test.ts @@ -5,7 +5,12 @@ */ import * as Rx from 'rxjs'; import { SpacesService } from './spaces_service'; -import { coreMock, elasticsearchServiceMock, httpServerMock } from 'src/core/server/mocks'; +import { + coreMock, + elasticsearchServiceMock, + httpServerMock, + loggingServiceMock, +} from 'src/core/server/mocks'; import { SpacesAuditLogger } from '../lib/audit_logger'; import { KibanaRequest, @@ -19,15 +24,7 @@ import { LegacyAPI } from '../plugin'; import { spacesConfig } from '../lib/__fixtures__'; import { securityMock } from '../../../security/server/mocks'; -const mockLogger = { - trace: jest.fn(), - debug: jest.fn(), - info: jest.fn(), - warn: jest.fn(), - error: jest.fn(), - fatal: jest.fn(), - log: jest.fn(), -}; +const mockLogger = loggingServiceMock.createLogger(); const createService = async (serverBasePath: string = '') => { const legacyAPI = {