Skip to content

Commit

Permalink
Allow loggers to create child loggers via 'get' method (#52605) (#53106)
Browse files Browse the repository at this point in the history
* add 'Logger.get' method

* updates generated doc

* resolve merge conflicts
  • Loading branch information
pgayvallet authored Dec 16, 2019
1 parent 3e952be commit 9e9fea1
Show file tree
Hide file tree
Showing 12 changed files with 129 additions and 55 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
46 changes: 46 additions & 0 deletions src/core/server/logging/logger.mock.ts
Original file line number Diff line number Diff line change
@@ -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<Logger> & { 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,
};
27 changes: 23 additions & 4 deletions src/core/server/logging/logger.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<any, any>(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();
});

Expand Down Expand Up @@ -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');
Expand All @@ -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) {
Expand Down Expand Up @@ -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');
Expand Down
19 changes: 18 additions & 1 deletion src/core/server/logging/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down
7 changes: 7 additions & 0 deletions src/core/server/logging/logger_adapter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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.', () => {
Expand All @@ -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 = {
Expand All @@ -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);
Expand Down
4 changes: 4 additions & 0 deletions src/core/server/logging/logger_adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
22 changes: 9 additions & 13 deletions src/core/server/logging/logging_service.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<Logger>;
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<ILoggingService> = {
get: jest.fn(),
Expand All @@ -42,8 +37,8 @@ const createLoggingServiceMock = () => {
stop: jest.fn(),
};
mocked.get.mockImplementation((...context) => ({
context,
...mockLog,
context,
}));
mocked.asLoggerFactory.mockImplementation(() => mocked);
mocked.stop.mockResolvedValue();
Expand Down Expand Up @@ -84,4 +79,5 @@ export const loggingServiceMock = {
create: createLoggingServiceMock,
collect: collectLoggingServiceMock,
clear: clearLoggingServiceMock,
createLogger: loggerMock.create,
};
13 changes: 5 additions & 8 deletions src/core/server/logging/logging_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)!;
}

Expand All @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions src/core/server/server.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
Expand Down Expand Up @@ -279,12 +280,4 @@ export const sampleRule = (): Partial<OutputRuleAlertRest> => {
};
};

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();
Original file line number Diff line number Diff line change
Expand Up @@ -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: {},
Expand Down
17 changes: 7 additions & 10 deletions x-pack/plugins/spaces/server/spaces_service/spaces_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 = {
Expand Down

0 comments on commit 9e9fea1

Please sign in to comment.