From 2b9832ecb5bbd07217e5199fcd5d7814773ad64d Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Tue, 26 Sep 2023 09:13:58 +0800 Subject: [PATCH] fix(sdk-logs): hide internal methods with internal shared state (#3865) Co-authored-by: Marc Pichler --- experimental/CHANGELOG.md | 1 + .../packages/sdk-logs/src/LogRecord.ts | 23 +-- experimental/packages/sdk-logs/src/Logger.ts | 44 ++---- .../packages/sdk-logs/src/LoggerProvider.ts | 67 +++------ .../sdk-logs/src/MultiLogRecordProcessor.ts | 8 +- experimental/packages/sdk-logs/src/config.ts | 67 +++------ .../src/export/NoopLogRecordProcessor.ts | 3 +- experimental/packages/sdk-logs/src/index.ts | 2 - .../src/internal/LoggerProviderSharedState.ts | 35 +++++ experimental/packages/sdk-logs/src/types.ts | 5 - .../sdk-logs/test/common/LogRecord.test.ts | 26 ++-- .../sdk-logs/test/common/Logger.test.ts | 36 +++-- .../test/common/LoggerProvider.test.ts | 133 +++++++++--------- .../export/BatchLogRecordProcessor.test.ts | 19 ++- .../export/SimpleLogRecordProcessor.test.ts | 51 +++---- 15 files changed, 246 insertions(+), 274 deletions(-) create mode 100644 experimental/packages/sdk-logs/src/internal/LoggerProviderSharedState.ts diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 24f36df117..11e98847da 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -12,6 +12,7 @@ All notable changes to experimental packages in this project will be documented * fix(sdk-node): remove explicit dependency on @opentelemetry/exporter-jaeger * '@opentelemetry/exporter-jaeger' is no longer be a dependency of this package. To continue using '@opentelemetry/exporter-jaeger', please install it manually. * NOTE: `@opentelemetry/exporter-jaeger` is deprecated, consider switching to one of the alternatives described [here](https://www.npmjs.com/package/@opentelemetry/exporter-jaeger) +* fix(sdk-logs): hide internal methods with internal shared state [#3865](https://github.com/open-telemetry/opentelemetry-js/pull/3865) @legendecas ### :rocket: (Enhancement) diff --git a/experimental/packages/sdk-logs/src/LogRecord.ts b/experimental/packages/sdk-logs/src/LogRecord.ts index d184004e6b..ed76d5eb07 100644 --- a/experimental/packages/sdk-logs/src/LogRecord.ts +++ b/experimental/packages/sdk-logs/src/LogRecord.ts @@ -26,8 +26,8 @@ import type { IResource } from '@opentelemetry/resources'; import type { ReadableLogRecord } from './export/ReadableLogRecord'; import type { LogRecordLimits } from './types'; -import { Logger } from './Logger'; import { LogAttributes } from '@opentelemetry/api-logs'; +import { LoggerProviderSharedState } from './internal/LoggerProviderSharedState'; export class LogRecord implements ReadableLogRecord { readonly hrTime: api.HrTime; @@ -41,7 +41,7 @@ export class LogRecord implements ReadableLogRecord { private _body?: string; private _isReadonly: boolean = false; - private readonly _logRecordLimits: LogRecordLimits; + private readonly _logRecordLimits: Required; set severityText(severityText: string | undefined) { if (this._isLogRecordReadonly()) { @@ -73,7 +73,11 @@ export class LogRecord implements ReadableLogRecord { return this._body; } - constructor(logger: Logger, logRecord: logsAPI.LogRecord) { + constructor( + _sharedState: LoggerProviderSharedState, + instrumentationScope: InstrumentationScope, + logRecord: logsAPI.LogRecord + ) { const { timestamp, observedTimestamp, @@ -97,9 +101,9 @@ export class LogRecord implements ReadableLogRecord { this.severityNumber = severityNumber; this.severityText = severityText; this.body = body; - this.resource = logger.resource; - this.instrumentationScope = logger.instrumentationScope; - this._logRecordLimits = logger.getLogRecordLimits(); + this.resource = _sharedState.resource; + this.instrumentationScope = instrumentationScope; + this._logRecordLimits = _sharedState.logRecordLimits; this.setAttributes(attributes); } @@ -127,7 +131,7 @@ export class LogRecord implements ReadableLogRecord { } if ( Object.keys(this.attributes).length >= - this._logRecordLimits.attributeCountLimit! && + this._logRecordLimits.attributeCountLimit && !Object.prototype.hasOwnProperty.call(this.attributes, key) ) { return this; @@ -159,15 +163,16 @@ export class LogRecord implements ReadableLogRecord { } /** + * @internal * A LogRecordProcessor may freely modify logRecord for the duration of the OnEmit call. * If logRecord is needed after OnEmit returns (i.e. for asynchronous processing) only reads are permitted. */ - public makeReadonly() { + _makeReadonly() { this._isReadonly = true; } private _truncateToSize(value: AttributeValue): AttributeValue { - const limit = this._logRecordLimits.attributeValueLengthLimit || 0; + const limit = this._logRecordLimits.attributeValueLengthLimit; // Check limit if (limit <= 0) { // Negative values are invalid, so do not truncate diff --git a/experimental/packages/sdk-logs/src/Logger.ts b/experimental/packages/sdk-logs/src/Logger.ts index 5ea4e8f28f..7694955e62 100644 --- a/experimental/packages/sdk-logs/src/Logger.ts +++ b/experimental/packages/sdk-logs/src/Logger.ts @@ -15,28 +15,17 @@ */ import type * as logsAPI from '@opentelemetry/api-logs'; -import type { IResource } from '@opentelemetry/resources'; import type { InstrumentationScope } from '@opentelemetry/core'; import { context } from '@opentelemetry/api'; -import type { LoggerConfig, LogRecordLimits } from './types'; import { LogRecord } from './LogRecord'; -import { LoggerProvider } from './LoggerProvider'; -import { mergeConfig } from './config'; -import { LogRecordProcessor } from './LogRecordProcessor'; +import { LoggerProviderSharedState } from './internal/LoggerProviderSharedState'; export class Logger implements logsAPI.Logger { - public readonly resource: IResource; - private readonly _loggerConfig: Required; - constructor( public readonly instrumentationScope: InstrumentationScope, - config: LoggerConfig, - private _loggerProvider: LoggerProvider - ) { - this._loggerConfig = mergeConfig(config); - this.resource = _loggerProvider.resource; - } + private _sharedState: LoggerProviderSharedState + ) {} public emit(logRecord: logsAPI.LogRecord): void { const currentContext = logRecord.context || context.active(); @@ -45,30 +34,23 @@ export class Logger implements logsAPI.Logger { * the LogRecords it emits MUST automatically include the Trace Context from the active Context, * if Context has not been explicitly set. */ - const logRecordInstance = new LogRecord(this, { - context: currentContext, - ...logRecord, - }); + const logRecordInstance = new LogRecord( + this._sharedState, + this.instrumentationScope, + { + context: currentContext, + ...logRecord, + } + ); /** * the explicitly passed Context, * the current Context, or an empty Context if the Logger was obtained with include_trace_context=false */ - this.getActiveLogRecordProcessor().onEmit( - logRecordInstance, - currentContext - ); + this._sharedState.activeProcessor.onEmit(logRecordInstance, currentContext); /** * A LogRecordProcessor may freely modify logRecord for the duration of the OnEmit call. * If logRecord is needed after OnEmit returns (i.e. for asynchronous processing) only reads are permitted. */ - logRecordInstance.makeReadonly(); - } - - public getLogRecordLimits(): LogRecordLimits { - return this._loggerConfig.logRecordLimits; - } - - public getActiveLogRecordProcessor(): LogRecordProcessor { - return this._loggerProvider.getActiveLogRecordProcessor(); + logRecordInstance._makeReadonly(); } } diff --git a/experimental/packages/sdk-logs/src/LoggerProvider.ts b/experimental/packages/sdk-logs/src/LoggerProvider.ts index 81515dab86..b8fd9851f8 100644 --- a/experimental/packages/sdk-logs/src/LoggerProvider.ts +++ b/experimental/packages/sdk-logs/src/LoggerProvider.ts @@ -16,7 +16,7 @@ import { diag } from '@opentelemetry/api'; import type * as logsAPI from '@opentelemetry/api-logs'; import { NOOP_LOGGER } from '@opentelemetry/api-logs'; -import { IResource, Resource } from '@opentelemetry/resources'; +import { Resource } from '@opentelemetry/resources'; import { BindOnceFuture, merge } from '@opentelemetry/core'; import type { LoggerProviderConfig } from './types'; @@ -24,39 +24,26 @@ import type { LogRecordProcessor } from './LogRecordProcessor'; import { Logger } from './Logger'; import { loadDefaultConfig, reconfigureLimits } from './config'; import { MultiLogRecordProcessor } from './MultiLogRecordProcessor'; -import { NoopLogRecordProcessor } from './export/NoopLogRecordProcessor'; +import { LoggerProviderSharedState } from './internal/LoggerProviderSharedState'; export const DEFAULT_LOGGER_NAME = 'unknown'; export class LoggerProvider implements logsAPI.LoggerProvider { - public readonly resource: IResource; - - private readonly _loggers: Map = new Map(); - private _activeProcessor: MultiLogRecordProcessor; - private readonly _registeredLogRecordProcessors: LogRecordProcessor[] = []; - private readonly _config: LoggerProviderConfig; private _shutdownOnce: BindOnceFuture; + private readonly _sharedState: LoggerProviderSharedState; constructor(config: LoggerProviderConfig = {}) { const { - resource = Resource.empty(), + resource = Resource.default(), logRecordLimits, forceFlushTimeoutMillis, - } = merge({}, loadDefaultConfig(), reconfigureLimits(config)); - this.resource = Resource.default().merge(resource); - this._config = { - logRecordLimits, - resource: this.resource, + } = merge({}, loadDefaultConfig(), config); + this._sharedState = new LoggerProviderSharedState( + resource, forceFlushTimeoutMillis, - }; - - this._shutdownOnce = new BindOnceFuture(this._shutdown, this); - - // add a default processor: NoopLogRecordProcessor - this._activeProcessor = new MultiLogRecordProcessor( - [new NoopLogRecordProcessor()], - forceFlushTimeoutMillis + reconfigureLimits(logRecordLimits) ); + this._shutdownOnce = new BindOnceFuture(this._shutdown, this); } /** @@ -77,19 +64,17 @@ export class LoggerProvider implements logsAPI.LoggerProvider { } const loggerName = name || DEFAULT_LOGGER_NAME; const key = `${loggerName}@${version || ''}:${options?.schemaUrl || ''}`; - if (!this._loggers.has(key)) { - this._loggers.set( + if (!this._sharedState.loggers.has(key)) { + this._sharedState.loggers.set( key, new Logger( { name: loggerName, version, schemaUrl: options?.schemaUrl }, - { - logRecordLimits: this._config.logRecordLimits, - }, - this + this._sharedState ) ); } - return this._loggers.get(key)!; + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + return this._sharedState.loggers.get(key)!; } /** @@ -97,10 +82,10 @@ export class LoggerProvider implements logsAPI.LoggerProvider { * @param processor the new LogRecordProcessor to be added. */ public addLogRecordProcessor(processor: LogRecordProcessor) { - if (this._registeredLogRecordProcessors.length === 0) { + if (this._sharedState.registeredLogRecordProcessors.length === 0) { // since we might have enabled by default a batchProcessor, we disable it // before adding the new one - this._activeProcessor + this._sharedState.activeProcessor .shutdown() .catch(err => diag.error( @@ -109,10 +94,10 @@ export class LoggerProvider implements logsAPI.LoggerProvider { ) ); } - this._registeredLogRecordProcessors.push(processor); - this._activeProcessor = new MultiLogRecordProcessor( - this._registeredLogRecordProcessors, - this._config.forceFlushTimeoutMillis! + this._sharedState.registeredLogRecordProcessors.push(processor); + this._sharedState.activeProcessor = new MultiLogRecordProcessor( + this._sharedState.registeredLogRecordProcessors, + this._sharedState.forceFlushTimeoutMillis ); } @@ -127,7 +112,7 @@ export class LoggerProvider implements logsAPI.LoggerProvider { diag.warn('invalid attempt to force flush after LoggerProvider shutdown'); return this._shutdownOnce.promise; } - return this._activeProcessor.forceFlush(); + return this._sharedState.activeProcessor.forceFlush(); } /** @@ -144,15 +129,7 @@ export class LoggerProvider implements logsAPI.LoggerProvider { return this._shutdownOnce.call(); } - public getActiveLogRecordProcessor(): MultiLogRecordProcessor { - return this._activeProcessor; - } - - public getActiveLoggers(): Map { - return this._loggers; - } - private _shutdown(): Promise { - return this._activeProcessor.shutdown(); + return this._sharedState.activeProcessor.shutdown(); } } diff --git a/experimental/packages/sdk-logs/src/MultiLogRecordProcessor.ts b/experimental/packages/sdk-logs/src/MultiLogRecordProcessor.ts index c4e5031261..353caefe0f 100644 --- a/experimental/packages/sdk-logs/src/MultiLogRecordProcessor.ts +++ b/experimental/packages/sdk-logs/src/MultiLogRecordProcessor.ts @@ -15,7 +15,7 @@ */ import { callWithTimeout } from '@opentelemetry/core'; - +import type { Context } from '@opentelemetry/api'; import type { LogRecordProcessor } from './LogRecordProcessor'; import type { LogRecord } from './LogRecord'; @@ -38,8 +38,10 @@ export class MultiLogRecordProcessor implements LogRecordProcessor { ); } - public onEmit(logRecord: LogRecord): void { - this.processors.forEach(processors => processors.onEmit(logRecord)); + public onEmit(logRecord: LogRecord, context?: Context): void { + this.processors.forEach(processors => + processors.onEmit(logRecord, context) + ); } public async shutdown(): Promise { diff --git a/experimental/packages/sdk-logs/src/config.ts b/experimental/packages/sdk-logs/src/config.ts index af908f1650..91b2c3e488 100644 --- a/experimental/packages/sdk-logs/src/config.ts +++ b/experimental/packages/sdk-logs/src/config.ts @@ -20,7 +20,7 @@ import { getEnv, getEnvWithoutDefaults, } from '@opentelemetry/core'; -import { LoggerConfig } from './types'; +import { LogRecordLimits } from './types'; export function loadDefaultConfig() { return { @@ -37,50 +37,29 @@ export function loadDefaultConfig() { /** * When general limits are provided and model specific limits are not, * configures the model specific limits by using the values from the general ones. - * @param userConfig User provided tracer configuration + * @param logRecordLimits User provided limits configuration */ -export function reconfigureLimits(userConfig: LoggerConfig): LoggerConfig { - const logRecordLimits = Object.assign({}, userConfig.logRecordLimits); - +export function reconfigureLimits( + logRecordLimits: LogRecordLimits +): Required { const parsedEnvConfig = getEnvWithoutDefaults(); - /** - * Reassign log record attribute count limit to use first non null value defined by user or use default value - */ - logRecordLimits.attributeCountLimit = - userConfig.logRecordLimits?.attributeCountLimit ?? - parsedEnvConfig.OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT ?? - parsedEnvConfig.OTEL_ATTRIBUTE_COUNT_LIMIT ?? - DEFAULT_ATTRIBUTE_COUNT_LIMIT; - - /** - * Reassign log record attribute value length limit to use first non null value defined by user or use default value - */ - logRecordLimits.attributeValueLengthLimit = - userConfig.logRecordLimits?.attributeValueLengthLimit ?? - parsedEnvConfig.OTEL_LOGRECORD_ATTRIBUTE_VALUE_LENGTH_LIMIT ?? - parsedEnvConfig.OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT ?? - DEFAULT_ATTRIBUTE_VALUE_LENGTH_LIMIT; - - return Object.assign({}, userConfig, { logRecordLimits }); -} - -/** - * Function to merge Default configuration (as specified in './config') with - * user provided configurations. - */ -export function mergeConfig(userConfig: LoggerConfig): Required { - const DEFAULT_CONFIG = loadDefaultConfig(); - - const target = Object.assign({}, DEFAULT_CONFIG, userConfig); - - target.logRecordLimits = Object.assign( - {}, - DEFAULT_CONFIG.logRecordLimits, - userConfig.logRecordLimits || {} - ); - - return target; + return { + /** + * Reassign log record attribute count limit to use first non null value defined by user or use default value + */ + attributeCountLimit: + logRecordLimits.attributeCountLimit ?? + parsedEnvConfig.OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT ?? + parsedEnvConfig.OTEL_ATTRIBUTE_COUNT_LIMIT ?? + DEFAULT_ATTRIBUTE_COUNT_LIMIT, + /** + * Reassign log record attribute value length limit to use first non null value defined by user or use default value + */ + attributeValueLengthLimit: + logRecordLimits.attributeValueLengthLimit ?? + parsedEnvConfig.OTEL_LOGRECORD_ATTRIBUTE_VALUE_LENGTH_LIMIT ?? + parsedEnvConfig.OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT ?? + DEFAULT_ATTRIBUTE_VALUE_LENGTH_LIMIT, + }; } - -export const DEFAULT_EVENT_DOMAIN = 'default'; diff --git a/experimental/packages/sdk-logs/src/export/NoopLogRecordProcessor.ts b/experimental/packages/sdk-logs/src/export/NoopLogRecordProcessor.ts index 91f277e8ab..c1f62ed8ca 100644 --- a/experimental/packages/sdk-logs/src/export/NoopLogRecordProcessor.ts +++ b/experimental/packages/sdk-logs/src/export/NoopLogRecordProcessor.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { Context } from '@opentelemetry/api'; import { LogRecordProcessor } from '../LogRecordProcessor'; import { ReadableLogRecord } from './ReadableLogRecord'; @@ -22,7 +23,7 @@ export class NoopLogRecordProcessor implements LogRecordProcessor { return Promise.resolve(); } - onEmit(_logRecord: ReadableLogRecord): void {} + onEmit(_logRecord: ReadableLogRecord, _context: Context): void {} shutdown(): Promise { return Promise.resolve(); diff --git a/experimental/packages/sdk-logs/src/index.ts b/experimental/packages/sdk-logs/src/index.ts index e718ae069e..b7347a2845 100644 --- a/experimental/packages/sdk-logs/src/index.ts +++ b/experimental/packages/sdk-logs/src/index.ts @@ -15,14 +15,12 @@ */ export { - LoggerConfig, LoggerProviderConfig, LogRecordLimits, BufferConfig, BatchLogRecordProcessorBrowserConfig, } from './types'; export { LoggerProvider } from './LoggerProvider'; -export { Logger } from './Logger'; export { LogRecord } from './LogRecord'; export { LogRecordProcessor } from './LogRecordProcessor'; export { ReadableLogRecord } from './export/ReadableLogRecord'; diff --git a/experimental/packages/sdk-logs/src/internal/LoggerProviderSharedState.ts b/experimental/packages/sdk-logs/src/internal/LoggerProviderSharedState.ts new file mode 100644 index 0000000000..16b208f72c --- /dev/null +++ b/experimental/packages/sdk-logs/src/internal/LoggerProviderSharedState.ts @@ -0,0 +1,35 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed 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 + * + * https://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 '@opentelemetry/api-logs'; +import { IResource } from '@opentelemetry/resources'; +import { LogRecordProcessor } from '../LogRecordProcessor'; +import { LogRecordLimits } from '../types'; +import { NoopLogRecordProcessor } from '../export/NoopLogRecordProcessor'; + +export class LoggerProviderSharedState { + readonly loggers: Map = new Map(); + activeProcessor: LogRecordProcessor; + readonly registeredLogRecordProcessors: LogRecordProcessor[] = []; + + constructor( + readonly resource: IResource, + readonly forceFlushTimeoutMillis: number, + readonly logRecordLimits: Required + ) { + this.activeProcessor = new NoopLogRecordProcessor(); + } +} diff --git a/experimental/packages/sdk-logs/src/types.ts b/experimental/packages/sdk-logs/src/types.ts index 026843dc15..27aefa540f 100644 --- a/experimental/packages/sdk-logs/src/types.ts +++ b/experimental/packages/sdk-logs/src/types.ts @@ -30,11 +30,6 @@ export interface LoggerProviderConfig { logRecordLimits?: LogRecordLimits; } -export interface LoggerConfig { - /** Log Record Limits*/ - logRecordLimits?: LogRecordLimits; -} - export interface LogRecordLimits { /** attributeValueLengthLimit is maximum allowed attribute value size */ attributeValueLengthLimit?: number; diff --git a/experimental/packages/sdk-logs/test/common/LogRecord.test.ts b/experimental/packages/sdk-logs/test/common/LogRecord.test.ts index cc7ce8bafd..e32ab39838 100644 --- a/experimental/packages/sdk-logs/test/common/LogRecord.test.ts +++ b/experimental/packages/sdk-logs/test/common/LogRecord.test.ts @@ -33,30 +33,32 @@ import { LogRecordLimits, LogRecordProcessor, LogRecord, - Logger, LoggerProvider, } from './../../src'; import { invalidAttributes, validAttributes } from './utils'; +import { LoggerProviderSharedState } from '../../src/internal/LoggerProviderSharedState'; +import { reconfigureLimits } from '../../src/config'; const performanceTimeOrigin: HrTime = [1, 1]; -const setup = (limits?: LogRecordLimits, data?: logsAPI.LogRecord) => { +const setup = (logRecordLimits?: LogRecordLimits, data?: logsAPI.LogRecord) => { const instrumentationScope = { name: 'test name', version: 'test version', schemaUrl: 'test schema url', }; const resource = Resource.default(); - const loggerProvider = new LoggerProvider({ resource }); - const logger = new Logger( + const sharedState = new LoggerProviderSharedState( + resource, + Infinity, + reconfigureLimits(logRecordLimits ?? {}) + ); + const logRecord = new LogRecord( + sharedState, instrumentationScope, - { - logRecordLimits: limits, - }, - loggerProvider + data ?? {} ); - const logRecord = new LogRecord(logger, data || {}); - return { logger, logRecord, instrumentationScope, resource }; + return { logRecord, instrumentationScope, resource }; }; describe('LogRecord', () => { @@ -320,7 +322,7 @@ describe('LogRecord', () => { it('should not rewrite directly through the property method', () => { const warnStub = sinon.spy(diag, 'warn'); const { logRecord } = setup(undefined, logRecordData); - logRecord.makeReadonly(); + logRecord._makeReadonly(); logRecord.body = newBody; logRecord.severityNumber = newSeverityNumber; @@ -346,7 +348,7 @@ describe('LogRecord', () => { it('should not rewrite using the set method', () => { const warnStub = sinon.spy(diag, 'warn'); const { logRecord } = setup(undefined, logRecordData); - logRecord.makeReadonly(); + logRecord._makeReadonly(); logRecord.setBody(newBody); logRecord.setSeverityNumber(newSeverityNumber); diff --git a/experimental/packages/sdk-logs/test/common/Logger.test.ts b/experimental/packages/sdk-logs/test/common/Logger.test.ts index a5f690a4b7..f78bda2d5e 100644 --- a/experimental/packages/sdk-logs/test/common/Logger.test.ts +++ b/experimental/packages/sdk-logs/test/common/Logger.test.ts @@ -17,21 +17,19 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; -import { LogRecord, Logger, LoggerConfig, LoggerProvider } from '../../src'; +import { LogRecord, LoggerProvider, NoopLogRecordProcessor } from '../../src'; import { ROOT_CONTEXT, TraceFlags, context, trace } from '@opentelemetry/api'; import { LogRecord as ApiLogRecord } from '@opentelemetry/api-logs'; +import { Logger } from '../../src/Logger'; -const setup = (loggerConfig: LoggerConfig = {}) => { - const logger = new Logger( - { - name: 'test name', - version: 'test version', - schemaUrl: 'test schema url', - }, - loggerConfig, - new LoggerProvider() - ); - return { logger }; +const setup = () => { + const loggerProvider = new LoggerProvider(); + const logProcessor = new NoopLogRecordProcessor(); + loggerProvider.addLogRecordProcessor(logProcessor); + const logger = loggerProvider.getLogger('test name', 'test version', { + schemaUrl: 'test schema url', + }) as Logger; + return { logger, logProcessor }; }; describe('Logger', () => { @@ -44,8 +42,8 @@ describe('Logger', () => { describe('emit', () => { it('should emit a logRecord instance', () => { - const { logger } = setup(); - const callSpy = sinon.spy(logger.getActiveLogRecordProcessor(), 'onEmit'); + const { logger, logProcessor } = setup(); + const callSpy = sinon.spy(logProcessor, 'onEmit'); logger.emit({ body: 'test log body', }); @@ -54,7 +52,7 @@ describe('Logger', () => { it('should make log record instance readonly after emit it', () => { const { logger } = setup(); - const makeOnlySpy = sinon.spy(LogRecord.prototype, 'makeReadonly'); + const makeOnlySpy = sinon.spy(LogRecord.prototype, '_makeReadonly'); logger.emit({ body: 'test log body', }); @@ -62,8 +60,8 @@ describe('Logger', () => { }); it('should emit with current Context', () => { - const { logger } = setup({}); - const callSpy = sinon.spy(logger.getActiveLogRecordProcessor(), 'onEmit'); + const { logger, logProcessor } = setup(); + const callSpy = sinon.spy(logProcessor, 'onEmit'); logger.emit({ body: 'test log body', }); @@ -71,7 +69,7 @@ describe('Logger', () => { }); it('should emit with Context specified in LogRecord', () => { - const { logger } = setup({}); + const { logger, logProcessor } = setup(); const spanContext = { traceId: 'd4cda95b652f4a1592b449d5929fda1b', spanId: '6e0c63257de34c92', @@ -82,7 +80,7 @@ describe('Logger', () => { context: activeContext, }; - const callSpy = sinon.spy(logger.getActiveLogRecordProcessor(), 'onEmit'); + const callSpy = sinon.spy(logProcessor, 'onEmit'); logger.emit(logRecordData); assert.ok(callSpy.calledWith(sinon.match.any, activeContext)); }); diff --git a/experimental/packages/sdk-logs/test/common/LoggerProvider.test.ts b/experimental/packages/sdk-logs/test/common/LoggerProvider.test.ts index d6032515a7..e4d80265c6 100644 --- a/experimental/packages/sdk-logs/test/common/LoggerProvider.test.ts +++ b/experimental/packages/sdk-logs/test/common/LoggerProvider.test.ts @@ -19,9 +19,11 @@ import { Resource } from '@opentelemetry/resources'; import * as assert from 'assert'; import * as sinon from 'sinon'; -import { Logger, LoggerProvider, NoopLogRecordProcessor } from '../../src'; +import { LoggerProvider, NoopLogRecordProcessor } from '../../src'; import { loadDefaultConfig } from '../../src/config'; import { DEFAULT_LOGGER_NAME } from './../../src/LoggerProvider'; +import { MultiLogRecordProcessor } from '../../src/MultiLogRecordProcessor'; +import { Logger } from '../../src/Logger'; describe('LoggerProvider', () => { let envSource: Record; @@ -48,45 +50,35 @@ describe('LoggerProvider', () => { assert.ok(provider instanceof LoggerProvider); }); - it('should use noop log record processor by default and no diag error', () => { - const errorStub = sinon.spy(diag, 'error'); + it('should use noop log record processor by default', () => { const provider = new LoggerProvider(); - const processors = provider.getActiveLogRecordProcessor().processors; - assert.ok(processors.length === 1); - assert.ok(processors[0] instanceof NoopLogRecordProcessor); - sinon.assert.notCalled(errorStub); + const sharedState = provider['_sharedState']; + const processor = sharedState.activeProcessor; + assert.ok(processor instanceof NoopLogRecordProcessor); }); it('should have default resource if not pass', () => { const provider = new LoggerProvider(); - const { resource } = provider; + const { resource } = provider['_sharedState']; assert.deepStrictEqual(resource, Resource.default()); }); it('should have default forceFlushTimeoutMillis if not pass', () => { const provider = new LoggerProvider(); - const activeProcessor = provider.getActiveLogRecordProcessor(); + const sharedState = provider['_sharedState']; assert.ok( - activeProcessor.forceFlushTimeoutMillis === + sharedState.forceFlushTimeoutMillis === loadDefaultConfig().forceFlushTimeoutMillis ); }); }); - describe('when user sets unavailable exporter', () => { - it('should use noop log record processor by default', () => { - const provider = new LoggerProvider(); - const processors = provider.getActiveLogRecordProcessor().processors; - assert.ok(processors.length === 1); - assert.ok(processors[0] instanceof NoopLogRecordProcessor); - }); - }); - describe('logRecordLimits', () => { describe('when not defined default values', () => { it('should have logger with default values', () => { - const logger = new LoggerProvider({}).getLogger('default') as Logger; - assert.deepStrictEqual(logger.getLogRecordLimits(), { + const loggerProvider = new LoggerProvider(); + const sharedState = loggerProvider['_sharedState']; + assert.deepStrictEqual(sharedState.logRecordLimits, { attributeValueLengthLimit: Infinity, attributeCountLimit: 128, }); @@ -95,34 +87,37 @@ describe('LoggerProvider', () => { describe('when "attributeCountLimit" is defined', () => { it('should have logger with defined value', () => { - const logger = new LoggerProvider({ + const loggerProvider = new LoggerProvider({ logRecordLimits: { attributeCountLimit: 100, }, - }).getLogger('default') as Logger; - const logRecordLimits = logger.getLogRecordLimits(); + }); + const logRecordLimits = + loggerProvider['_sharedState'].logRecordLimits; assert.strictEqual(logRecordLimits.attributeCountLimit, 100); }); }); describe('when "attributeValueLengthLimit" is defined', () => { it('should have logger with defined value', () => { - const logger = new LoggerProvider({ + const loggerProvider = new LoggerProvider({ logRecordLimits: { attributeValueLengthLimit: 10, }, - }).getLogger('default') as Logger; - const logRecordLimits = logger.getLogRecordLimits(); + }); + const logRecordLimits = + loggerProvider['_sharedState'].logRecordLimits; assert.strictEqual(logRecordLimits.attributeValueLengthLimit, 10); }); it('should have logger with negative "attributeValueLengthLimit" value', () => { - const logger = new LoggerProvider({ + const loggerProvider = new LoggerProvider({ logRecordLimits: { attributeValueLengthLimit: -10, }, - }).getLogger('default') as Logger; - const logRecordLimits = logger.getLogRecordLimits(); + }); + const logRecordLimits = + loggerProvider['_sharedState'].logRecordLimits; assert.strictEqual(logRecordLimits.attributeValueLengthLimit, -10); }); }); @@ -130,8 +125,9 @@ describe('LoggerProvider', () => { describe('when attribute value length limit is defined via env', () => { it('should have attribute value length limit as default of Infinity', () => { envSource.OTEL_LOGRECORD_ATTRIBUTE_VALUE_LENGTH_LIMIT = 'Infinity'; - const logger = new LoggerProvider().getLogger('default') as Logger; - const logRecordLimits = logger.getLogRecordLimits(); + const loggerProvider = new LoggerProvider(); + const logRecordLimits = + loggerProvider['_sharedState'].logRecordLimits; assert.strictEqual( logRecordLimits.attributeValueLengthLimit, Infinity @@ -142,8 +138,9 @@ describe('LoggerProvider', () => { describe('when attribute value length limit is not defined via env', () => { it('should use default value of Infinity', () => { - const logger = new LoggerProvider().getLogger('default') as Logger; - const logRecordLimits = logger.getLogRecordLimits(); + const loggerProvider = new LoggerProvider(); + const logRecordLimits = + loggerProvider['_sharedState'].logRecordLimits; assert.strictEqual( logRecordLimits.attributeValueLengthLimit, Infinity @@ -154,15 +151,17 @@ describe('LoggerProvider', () => { describe('when attribute count limit is defined via env', () => { it('should have attribute count limits as defined in env', () => { envSource.OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT = '35'; - const logger = new LoggerProvider().getLogger('default') as Logger; - const logRecordLimits = logger.getLogRecordLimits(); + const loggerProvider = new LoggerProvider(); + const logRecordLimits = + loggerProvider['_sharedState'].logRecordLimits; assert.strictEqual(logRecordLimits.attributeCountLimit, 35); delete envSource.OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT; }); it('should have attribute count limit as default of 128', () => { envSource.OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT = '128'; - const logger = new LoggerProvider().getLogger('default') as Logger; - const logRecordLimits = logger.getLogRecordLimits(); + const loggerProvider = new LoggerProvider(); + const logRecordLimits = + loggerProvider['_sharedState'].logRecordLimits; assert.strictEqual(logRecordLimits.attributeCountLimit, 128); delete envSource.OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT; }); @@ -170,8 +169,9 @@ describe('LoggerProvider', () => { describe('when attribute count limit is not defined via env', () => { it('should use default value of 128', () => { - const logger = new LoggerProvider().getLogger('default') as Logger; - const logRecordLimits = logger.getLogRecordLimits(); + const loggerProvider = new LoggerProvider(); + const logRecordLimits = + loggerProvider['_sharedState'].logRecordLimits; assert.strictEqual(logRecordLimits.attributeCountLimit, 128); }); }); @@ -186,55 +186,61 @@ describe('LoggerProvider', () => { it('should create a logger instance with default name if the name is invalid ', () => { const provider = new LoggerProvider(); const logger = provider.getLogger('') as Logger; - assert.ok(logger.instrumentationScope.name === DEFAULT_LOGGER_NAME); + assert.strictEqual(logger.instrumentationScope.name, DEFAULT_LOGGER_NAME); }); it("should create a logger instance if the name doesn't exist", () => { const provider = new LoggerProvider(); - assert.ok(provider.getActiveLoggers().size === 0); + const sharedState = provider['_sharedState']; + assert.strictEqual(sharedState.loggers.size, 0); provider.getLogger(testName); - assert.ok(provider.getActiveLoggers().size === 1); + assert.strictEqual(sharedState.loggers.size, 1); }); it('should create A new object if the name & version & schemaUrl are not unique', () => { const provider = new LoggerProvider(); - assert.ok(provider.getActiveLoggers().size === 0); + const sharedState = provider['_sharedState']; + assert.strictEqual(sharedState.loggers.size, 0); provider.getLogger(testName); - assert.ok(provider.getActiveLoggers().size === 1); + assert.strictEqual(sharedState.loggers.size, 1); provider.getLogger(testName, testVersion); - assert.ok(provider.getActiveLoggers().size === 2); + assert.strictEqual(sharedState.loggers.size, 2); provider.getLogger(testName, testVersion, { schemaUrl: testSchemaURL }); - assert.ok(provider.getActiveLoggers().size === 3); + assert.strictEqual(sharedState.loggers.size, 3); }); it('should not create A new object if the name & version & schemaUrl are unique', () => { const provider = new LoggerProvider(); + const sharedState = provider['_sharedState']; - assert.ok(provider.getActiveLoggers().size === 0); + assert.strictEqual(sharedState.loggers.size, 0); provider.getLogger(testName); - assert.ok(provider.getActiveLoggers().size === 1); + assert.strictEqual(sharedState.loggers.size, 1); const logger1 = provider.getLogger(testName, testVersion, { schemaUrl: testSchemaURL, }); - assert.ok(provider.getActiveLoggers().size === 2); + assert.strictEqual(sharedState.loggers.size, 2); const logger2 = provider.getLogger(testName, testVersion, { schemaUrl: testSchemaURL, }); - assert.ok(provider.getActiveLoggers().size === 2); + assert.strictEqual(sharedState.loggers.size, 2); assert.ok(logger2 instanceof Logger); - assert.ok(logger1 === logger2); + assert.strictEqual(logger1, logger2); }); }); describe('addLogRecordProcessor', () => { it('should add logRecord processor', () => { - const logRecordProcessor = new NoopLogRecordProcessor(); const provider = new LoggerProvider(); + const sharedState = provider['_sharedState']; + const logRecordProcessor = new NoopLogRecordProcessor(); provider.addLogRecordProcessor(logRecordProcessor); + assert.ok(sharedState.activeProcessor instanceof MultiLogRecordProcessor); + assert.strictEqual(sharedState.activeProcessor.processors.length, 1); assert.strictEqual( - provider.getActiveLogRecordProcessor().processors.length, - 1 + sharedState.activeProcessor.processors[0], + logRecordProcessor ); }); }); @@ -301,10 +307,9 @@ describe('LoggerProvider', () => { describe('.shutdown()', () => { it('should trigger shutdown when manually invoked', () => { const provider = new LoggerProvider(); - const shutdownStub = sinon.stub( - provider.getActiveLogRecordProcessor(), - 'shutdown' - ); + const processor = new NoopLogRecordProcessor(); + provider.addLogRecordProcessor(processor); + const shutdownStub = sinon.stub(processor, 'shutdown'); provider.shutdown(); sinon.assert.calledOnce(shutdownStub); }); @@ -321,10 +326,7 @@ describe('LoggerProvider', () => { const provider = new LoggerProvider(); const logRecordProcessor = new NoopLogRecordProcessor(); provider.addLogRecordProcessor(logRecordProcessor); - const forceFlushStub = sinon.stub( - provider.getActiveLogRecordProcessor(), - 'forceFlush' - ); + const forceFlushStub = sinon.stub(logRecordProcessor, 'forceFlush'); const warnStub = sinon.spy(diag, 'warn'); provider.shutdown(); provider.forceFlush(); @@ -336,10 +338,7 @@ describe('LoggerProvider', () => { const provider = new LoggerProvider(); const logRecordProcessor = new NoopLogRecordProcessor(); provider.addLogRecordProcessor(logRecordProcessor); - const shutdownStub = sinon.stub( - provider.getActiveLogRecordProcessor(), - 'shutdown' - ); + const shutdownStub = sinon.stub(logRecordProcessor, 'shutdown'); const warnStub = sinon.spy(diag, 'warn'); provider.shutdown(); provider.shutdown(); diff --git a/experimental/packages/sdk-logs/test/common/export/BatchLogRecordProcessor.test.ts b/experimental/packages/sdk-logs/test/common/export/BatchLogRecordProcessor.test.ts index 2d0fab7a2c..70859a314c 100644 --- a/experimental/packages/sdk-logs/test/common/export/BatchLogRecordProcessor.test.ts +++ b/experimental/packages/sdk-logs/test/common/export/BatchLogRecordProcessor.test.ts @@ -28,10 +28,11 @@ import { LogRecordLimits, LogRecord, InMemoryLogRecordExporter, - LoggerProvider, - Logger, } from '../../../src'; import { BatchLogRecordProcessorBase } from '../../../src/export/BatchLogRecordProcessorBase'; +import { reconfigureLimits } from '../../../src/config'; +import { LoggerProviderSharedState } from '../../../src/internal/LoggerProviderSharedState'; +import { Resource } from '@opentelemetry/resources'; class BatchLogRecordProcessor extends BatchLogRecordProcessorBase { onInit() {} @@ -39,18 +40,22 @@ class BatchLogRecordProcessor extends BatchLogRecordProcessorBase } const createLogRecord = (limits?: LogRecordLimits): LogRecord => { - const logger = new Logger( + const sharedState = new LoggerProviderSharedState( + Resource.default(), + Infinity, + reconfigureLimits(limits ?? {}) + ); + const logRecord = new LogRecord( + sharedState, { name: 'test name', version: 'test version', schemaUrl: 'test schema url', }, { - logRecordLimits: limits, - }, - new LoggerProvider() + body: 'body', + } ); - const logRecord = new LogRecord(logger, { body: 'body' }); return logRecord; }; diff --git a/experimental/packages/sdk-logs/test/common/export/SimpleLogRecordProcessor.test.ts b/experimental/packages/sdk-logs/test/common/export/SimpleLogRecordProcessor.test.ts index 202554dde9..27eacc9195 100644 --- a/experimental/packages/sdk-logs/test/common/export/SimpleLogRecordProcessor.test.ts +++ b/experimental/packages/sdk-logs/test/common/export/SimpleLogRecordProcessor.test.ts @@ -27,13 +27,30 @@ import { LogRecordExporter, SimpleLogRecordProcessor, LogRecord, - LoggerProvider, - Logger, } from './../../../src'; +import { LoggerProviderSharedState } from '../../../src/internal/LoggerProviderSharedState'; +import { Resource } from '@opentelemetry/resources'; +import { reconfigureLimits } from '../../../src/config'; const setup = (exporter: LogRecordExporter) => { + const sharedState = new LoggerProviderSharedState( + Resource.default(), + Infinity, + reconfigureLimits({}) + ); + const logRecord = new LogRecord( + sharedState, + { + name: 'test name', + version: 'test version', + schemaUrl: 'test schema url', + }, + { + body: 'body', + } + ); const processor = new SimpleLogRecordProcessor(exporter); - return { exporter, processor }; + return { exporter, processor, logRecord }; }; describe('SimpleLogRecordProcessor', () => { @@ -49,21 +66,9 @@ describe('SimpleLogRecordProcessor', () => { describe('onEmit', () => { it('should handle onEmit', async () => { const exporter = new InMemoryLogRecordExporter(); - const { processor } = setup(exporter); + const { processor, logRecord } = setup(exporter); assert.strictEqual(exporter.getFinishedLogRecords().length, 0); - const logger = new Logger( - { - name: 'test name', - version: 'test version', - schemaUrl: 'test schema url', - }, - {}, - new LoggerProvider() - ); - const logRecord = new LogRecord(logger, { - body: 'body', - }); processor.onEmit(logRecord); assert.strictEqual(exporter.getFinishedLogRecords().length, 1); @@ -82,20 +87,8 @@ describe('SimpleLogRecordProcessor', () => { ), shutdown: () => Promise.resolve(), }; - const { processor } = setup(exporter); + const { processor, logRecord } = setup(exporter); - const logger = new Logger( - { - name: 'test name', - version: 'test version', - schemaUrl: 'test schema url', - }, - {}, - new LoggerProvider() - ); - const logRecord = new LogRecord(logger, { - body: 'body', - }); const errorHandlerSpy = sinon.spy(); setGlobalErrorHandler(errorHandlerSpy); processor.onEmit(logRecord);