From 5c90aec9afb2f2c86dea3ab649feeb1b5564d14d Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Mon, 17 Feb 2025 10:57:22 +0100 Subject: [PATCH] feat(sdk-logs)!: do not read environment variables from window (#5472) --- experimental/CHANGELOG.md | 3 + experimental/packages/sdk-logs/karma.conf.js | 6 +- experimental/packages/sdk-logs/src/config.ts | 27 +++----- .../src/export/BatchLogRecordProcessorBase.ts | 21 ++++-- .../test/{ => browser}/index-webpack.ts | 11 +-- .../{ => browser}/index-webpack.worker.ts | 11 +-- .../test/common/LoggerProvider.test.ts | 45 +----------- .../export/BatchLogRecordProcessor.test.ts | 24 ++----- .../sdk-logs/test/node/LoggerProvider.test.ts | 68 +++++++++++++++++++ 9 files changed, 120 insertions(+), 96 deletions(-) rename experimental/packages/sdk-logs/test/{ => browser}/index-webpack.ts (64%) rename experimental/packages/sdk-logs/test/{ => browser}/index-webpack.worker.ts (64%) create mode 100644 experimental/packages/sdk-logs/test/node/LoggerProvider.test.ts diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 46e5bfeb8a5..2911c55cd5f 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -16,6 +16,9 @@ All notable changes to experimental packages in this project will be documented * chore!: Raise the minimum supported Node.js version to `^18.19.0 || >=20.6.0`. Support for Node.js 14, 16, and early minor versions of 18 and 20 have been dropped. This applies to all packages except the 'api' and 'semantic-conventions' packages. [#5395](https://github.com/open-telemetry/opentelemetry-js/issues/5395) @trentm * feat(sdk-node)!: use `IMetricReader` over `MetricReader` [#5311](https://github.com/open-telemetry/opentelemetry-js/pull/5311) * (user-facing): `NodeSDKConfiguration` now provides the more general `IMetricReader` type over `MetricReader` +* feat(sdk-logs)!: do not read environment variables from window in browsers [#5472](https://github.com/open-telemetry/opentelemetry-js/pull/5472) @pichlermarc + * (user-facing): all configuration previously possible via `window.OTEL_*` is now not supported anymore, please pass configuration options to constructors instead. + * Note: Node.js environment variable configuration continues to work as-is. ### :rocket: (Enhancement) diff --git a/experimental/packages/sdk-logs/karma.conf.js b/experimental/packages/sdk-logs/karma.conf.js index d1c8fbaac18..50fac98bd80 100644 --- a/experimental/packages/sdk-logs/karma.conf.js +++ b/experimental/packages/sdk-logs/karma.conf.js @@ -1,11 +1,11 @@ -/*! +/* * 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 * - * http://www.apache.org/licenses/LICENSE-2.0 + * 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, @@ -21,6 +21,8 @@ module.exports = config => { config.set( Object.assign({}, karmaBaseConfig, { webpack: karmaWebpackConfig, + files: ['test/browser/index-webpack.ts'], + preprocessors: { 'test/browser/index-webpack.ts': ['webpack'] }, }) ); }; diff --git a/experimental/packages/sdk-logs/src/config.ts b/experimental/packages/sdk-logs/src/config.ts index 91b2c3e4884..cb6b77d69fc 100644 --- a/experimental/packages/sdk-logs/src/config.ts +++ b/experimental/packages/sdk-logs/src/config.ts @@ -14,12 +14,7 @@ * limitations under the License. */ -import { - DEFAULT_ATTRIBUTE_COUNT_LIMIT, - DEFAULT_ATTRIBUTE_VALUE_LENGTH_LIMIT, - getEnv, - getEnvWithoutDefaults, -} from '@opentelemetry/core'; +import { getNumberFromEnv } from '@opentelemetry/core'; import { LogRecordLimits } from './types'; export function loadDefaultConfig() { @@ -27,8 +22,10 @@ export function loadDefaultConfig() { forceFlushTimeoutMillis: 30000, logRecordLimits: { attributeValueLengthLimit: - getEnv().OTEL_LOGRECORD_ATTRIBUTE_VALUE_LENGTH_LIMIT, - attributeCountLimit: getEnv().OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT, + getNumberFromEnv('OTEL_LOGRECORD_ATTRIBUTE_VALUE_LENGTH_LIMIT') ?? + Infinity, + attributeCountLimit: + getNumberFromEnv('OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT') ?? 128, }, includeTraceContext: true, }; @@ -42,24 +39,22 @@ export function loadDefaultConfig() { export function reconfigureLimits( logRecordLimits: LogRecordLimits ): Required { - const parsedEnvConfig = getEnvWithoutDefaults(); - 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, + getNumberFromEnv('OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT') ?? + getNumberFromEnv('OTEL_ATTRIBUTE_COUNT_LIMIT') ?? + 128, /** * 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, + getNumberFromEnv('OTEL_LOGRECORD_ATTRIBUTE_VALUE_LENGTH_LIMIT') ?? + getNumberFromEnv('OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT') ?? + Infinity, }; } diff --git a/experimental/packages/sdk-logs/src/export/BatchLogRecordProcessorBase.ts b/experimental/packages/sdk-logs/src/export/BatchLogRecordProcessorBase.ts index bfe6367cf25..e68f6a1fe0c 100644 --- a/experimental/packages/sdk-logs/src/export/BatchLogRecordProcessorBase.ts +++ b/experimental/packages/sdk-logs/src/export/BatchLogRecordProcessorBase.ts @@ -14,11 +14,10 @@ * limitations under the License. */ -import type { ExportResult } from '@opentelemetry/core'; +import { ExportResult, getNumberFromEnv } from '@opentelemetry/core'; import { diag } from '@opentelemetry/api'; import { ExportResultCode, - getEnv, globalErrorHandler, unrefTimer, BindOnceFuture, @@ -47,14 +46,22 @@ export abstract class BatchLogRecordProcessorBase private readonly _exporter: LogRecordExporter, config?: T ) { - const env = getEnv(); this._maxExportBatchSize = - config?.maxExportBatchSize ?? env.OTEL_BLRP_MAX_EXPORT_BATCH_SIZE; - this._maxQueueSize = config?.maxQueueSize ?? env.OTEL_BLRP_MAX_QUEUE_SIZE; + config?.maxExportBatchSize ?? + getNumberFromEnv('OTEL_BLRP_MAX_EXPORT_BATCH_SIZE') ?? + 512; + this._maxQueueSize = + config?.maxQueueSize ?? + getNumberFromEnv('OTEL_BLRP_MAX_QUEUE_SIZE') ?? + 2048; this._scheduledDelayMillis = - config?.scheduledDelayMillis ?? env.OTEL_BLRP_SCHEDULE_DELAY; + config?.scheduledDelayMillis ?? + getNumberFromEnv('OTEL_BLRP_SCHEDULE_DELAY') ?? + 5000; this._exportTimeoutMillis = - config?.exportTimeoutMillis ?? env.OTEL_BLRP_EXPORT_TIMEOUT; + config?.exportTimeoutMillis ?? + getNumberFromEnv('OTEL_BLRP_EXPORT_TIMEOUT') ?? + 30000; this._shutdownOnce = new BindOnceFuture(this._shutdown, this); diff --git a/experimental/packages/sdk-logs/test/index-webpack.ts b/experimental/packages/sdk-logs/test/browser/index-webpack.ts similarity index 64% rename from experimental/packages/sdk-logs/test/index-webpack.ts rename to experimental/packages/sdk-logs/test/browser/index-webpack.ts index 802d6c4053e..99100a0f6ee 100644 --- a/experimental/packages/sdk-logs/test/index-webpack.ts +++ b/experimental/packages/sdk-logs/test/browser/index-webpack.ts @@ -13,8 +13,11 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +const testsContext = require.context('../browser', true, /test$/); +testsContext.keys().forEach(testsContext); -{ - const testsContext = require.context('./', true, /test$/); - testsContext.keys().forEach(testsContext); -} +const testsContextCommon = require.context('../common', true, /test$/); +testsContextCommon.keys().forEach(testsContextCommon); + +const srcContext = require.context('.', true, /src$/); +srcContext.keys().forEach(srcContext); diff --git a/experimental/packages/sdk-logs/test/index-webpack.worker.ts b/experimental/packages/sdk-logs/test/browser/index-webpack.worker.ts similarity index 64% rename from experimental/packages/sdk-logs/test/index-webpack.worker.ts rename to experimental/packages/sdk-logs/test/browser/index-webpack.worker.ts index 802d6c4053e..99100a0f6ee 100644 --- a/experimental/packages/sdk-logs/test/index-webpack.worker.ts +++ b/experimental/packages/sdk-logs/test/browser/index-webpack.worker.ts @@ -13,8 +13,11 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +const testsContext = require.context('../browser', true, /test$/); +testsContext.keys().forEach(testsContext); -{ - const testsContext = require.context('./', true, /test$/); - testsContext.keys().forEach(testsContext); -} +const testsContextCommon = require.context('../common', true, /test$/); +testsContextCommon.keys().forEach(testsContextCommon); + +const srcContext = require.context('.', true, /src$/); +srcContext.keys().forEach(srcContext); diff --git a/experimental/packages/sdk-logs/test/common/LoggerProvider.test.ts b/experimental/packages/sdk-logs/test/common/LoggerProvider.test.ts index 9dcdc18dd44..391aa9716aa 100644 --- a/experimental/packages/sdk-logs/test/common/LoggerProvider.test.ts +++ b/experimental/packages/sdk-logs/test/common/LoggerProvider.test.ts @@ -29,14 +29,6 @@ import { MultiLogRecordProcessor } from '../../src/MultiLogRecordProcessor'; import { Logger } from '../../src/Logger'; describe('LoggerProvider', () => { - let envSource: Record; - - if (global.process?.versions?.node === undefined) { - envSource = globalThis as unknown as Record; - } else { - envSource = process.env as Record; - } - beforeEach(() => { // to avoid actually registering the LoggerProvider and leaking env to other tests sinon.stub(logs, 'setGlobalLoggerProvider'); @@ -134,21 +126,7 @@ 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 loggerProvider = new LoggerProvider(); - const logRecordLimits = - loggerProvider['_sharedState'].logRecordLimits; - assert.strictEqual( - logRecordLimits.attributeValueLengthLimit, - Infinity - ); - delete envSource.OTEL_LOGRECORD_ATTRIBUTE_VALUE_LENGTH_LIMIT; - }); - }); - - describe('when attribute value length limit is not defined via env', () => { + describe('when attribute value length limit is not defined', () => { it('should use default value of Infinity', () => { const loggerProvider = new LoggerProvider(); const logRecordLimits = @@ -160,26 +138,7 @@ 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 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 loggerProvider = new LoggerProvider(); - const logRecordLimits = - loggerProvider['_sharedState'].logRecordLimits; - assert.strictEqual(logRecordLimits.attributeCountLimit, 128); - delete envSource.OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT; - }); - }); - - describe('when attribute count limit is not defined via env', () => { + describe('when attribute count limit is not defined', () => { it('should use default value of 128', () => { const loggerProvider = new LoggerProvider(); const logRecordLimits = 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 6348ab5e72a..527843d92b0 100644 --- a/experimental/packages/sdk-logs/test/common/export/BatchLogRecordProcessor.test.ts +++ b/experimental/packages/sdk-logs/test/common/export/BatchLogRecordProcessor.test.ts @@ -18,7 +18,6 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import { ExportResultCode, - getEnv, loggingErrorHandler, setGlobalErrorHandler, } from '@opentelemetry/core'; @@ -117,26 +116,11 @@ describe('BatchLogRecordProcessorBase', () => { it('should create a BatchLogRecordProcessor instance with empty config', () => { const processor = new BatchLogRecordProcessor(exporter); - const { - OTEL_BSP_MAX_EXPORT_BATCH_SIZE, - OTEL_BSP_MAX_QUEUE_SIZE, - OTEL_BSP_SCHEDULE_DELAY, - OTEL_BSP_EXPORT_TIMEOUT, - } = getEnv(); assert.ok(processor instanceof BatchLogRecordProcessor); - assert.strictEqual( - processor['_maxExportBatchSize'], - OTEL_BSP_MAX_EXPORT_BATCH_SIZE - ); - assert.strictEqual(processor['_maxQueueSize'], OTEL_BSP_MAX_QUEUE_SIZE); - assert.strictEqual( - processor['_scheduledDelayMillis'], - OTEL_BSP_SCHEDULE_DELAY - ); - assert.strictEqual( - processor['_exportTimeoutMillis'], - OTEL_BSP_EXPORT_TIMEOUT - ); + assert.strictEqual(processor['_maxExportBatchSize'], 512); + assert.strictEqual(processor['_maxQueueSize'], 2048); + assert.strictEqual(processor['_scheduledDelayMillis'], 5000); + assert.strictEqual(processor['_exportTimeoutMillis'], 30000); processor.shutdown(); }); diff --git a/experimental/packages/sdk-logs/test/node/LoggerProvider.test.ts b/experimental/packages/sdk-logs/test/node/LoggerProvider.test.ts new file mode 100644 index 00000000000..94f4acb8f1b --- /dev/null +++ b/experimental/packages/sdk-logs/test/node/LoggerProvider.test.ts @@ -0,0 +1,68 @@ +/* + * 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 { logs } from '@opentelemetry/api-logs'; +import * as assert from 'assert'; +import * as sinon from 'sinon'; + +import { LoggerProvider } from '../../src'; + +describe('LoggerProvider - node', () => { + beforeEach(() => { + // to avoid actually registering the LoggerProvider and leaking env to other tests + sinon.stub(logs, 'setGlobalLoggerProvider'); + }); + + afterEach(() => { + sinon.restore(); + }); + + describe('constructor', () => { + describe('logRecordLimits', () => { + describe('when attribute value length limit is defined via env', () => { + it('should have attribute value length limit as default of Infinity', () => { + process.env.OTEL_LOGRECORD_ATTRIBUTE_VALUE_LENGTH_LIMIT = 'Infinity'; + const loggerProvider = new LoggerProvider(); + const logRecordLimits = + loggerProvider['_sharedState'].logRecordLimits; + assert.strictEqual( + logRecordLimits.attributeValueLengthLimit, + Infinity + ); + delete process.env.OTEL_LOGRECORD_ATTRIBUTE_VALUE_LENGTH_LIMIT; + }); + }); + + describe('when attribute count limit is defined via env', () => { + it('should have attribute count limits as defined in env', () => { + process.env.OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT = '35'; + const loggerProvider = new LoggerProvider(); + const logRecordLimits = + loggerProvider['_sharedState'].logRecordLimits; + assert.strictEqual(logRecordLimits.attributeCountLimit, 35); + delete process.env.OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT; + }); + it('should have attribute count limit as default of 128', () => { + process.env.OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT = '128'; + const loggerProvider = new LoggerProvider(); + const logRecordLimits = + loggerProvider['_sharedState'].logRecordLimits; + assert.strictEqual(logRecordLimits.attributeCountLimit, 128); + delete process.env.OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT; + }); + }); + }); + }); +});