From db8ef52b18c77dfb98b5caf3ee7a67772c11efa7 Mon Sep 17 00:00:00 2001 From: Siim Kallas Date: Tue, 14 Mar 2023 12:14:22 +0200 Subject: [PATCH] fix: handle empty env vars as undefined (#693) --- src/instrument.ts | 5 +-- src/metrics/index.ts | 10 +++--- src/profiling/index.ts | 7 ++-- src/profiling/utils.ts | 7 ++-- src/start.ts | 9 +++--- src/tracing/index.ts | 8 +++-- src/tracing/options.ts | 13 ++++---- src/types.ts | 34 +++++++++++++++++++ src/utils.ts | 36 +++++++++++++++------ test/examples/collector.config.yml | 2 +- test/utils.test.ts | 52 +++++++++++++++++++++++++++++- 11 files changed, 148 insertions(+), 35 deletions(-) diff --git a/src/instrument.ts b/src/instrument.ts index 1ce05de8..904b7325 100644 --- a/src/instrument.ts +++ b/src/instrument.ts @@ -19,6 +19,7 @@ import { defaultServiceName, getEnvArray, getEnvBoolean, + getNonEmptyEnvVar, parseLogLevel, } from './utils'; @@ -27,13 +28,13 @@ import { startProfiling } from './profiling'; import { startTracing } from './tracing'; function boot() { - const logLevel = parseLogLevel(process.env.OTEL_LOG_LEVEL); + const logLevel = parseLogLevel(getNonEmptyEnvVar('OTEL_LOG_LEVEL')); if (logLevel !== DiagLogLevel.NONE) { diag.setLogger(new DiagConsoleLogger(), logLevel); } - if (process.env.SPLUNK_AUTOINSTRUMENT_PACKAGE_NAMES !== undefined) { + if (getNonEmptyEnvVar('SPLUNK_AUTOINSTRUMENT_PACKAGE_NAMES') !== undefined) { const limitToPackages = getEnvArray( 'SPLUNK_AUTOINSTRUMENT_PACKAGE_NAMES', [] diff --git a/src/metrics/index.ts b/src/metrics/index.ts index f6d1a97e..0db9eca1 100644 --- a/src/metrics/index.ts +++ b/src/metrics/index.ts @@ -32,6 +32,7 @@ import { getEnvBoolean, getEnvNumber, getEnvValueByPrecedence, + getNonEmptyEnvVar, } from '../utils'; import * as util from 'util'; import { detect as detectResource } from '../resource'; @@ -195,7 +196,7 @@ function createExporters(options: MetricsOptions) { if (!areValidExporterTypes(metricExporters)) { throw new Error( `Invalid value for OTEL_METRICS_EXPORTER env variable: ${util.inspect( - process.env.OTEL_METRICS_EXPORTER + getNonEmptyEnvVar('OTEL_METRICS_EXPORTER') )}. Choose from ${util.inspect(SUPPORTED_EXPORTER_TYPES, { compact: true, })} or leave undefined.` @@ -373,11 +374,12 @@ export function _setDefaultOptions( options: StartMetricsOptions = {} ): MetricsOptions { const accessToken = - options.accessToken || process.env.SPLUNK_ACCESS_TOKEN || ''; + options.accessToken || getNonEmptyEnvVar('SPLUNK_ACCESS_TOKEN') || ''; - const endpoint = options.endpoint || process.env.SPLUNK_METRICS_ENDPOINT; + const endpoint = + options.endpoint || getNonEmptyEnvVar('SPLUNK_METRICS_ENDPOINT'); - const realm = options.realm || process.env.SPLUNK_REALM || ''; + const realm = options.realm || getNonEmptyEnvVar('SPLUNK_REALM') || ''; if (realm) { if (!accessToken) { diff --git a/src/profiling/index.ts b/src/profiling/index.ts index ded528c7..c9a3f529 100644 --- a/src/profiling/index.ts +++ b/src/profiling/index.ts @@ -21,6 +21,7 @@ import { defaultServiceName, getEnvBoolean, getEnvNumber, + getNonEmptyEnvVar, } from '../utils'; import { detect as detectResource } from '../resource'; import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions'; @@ -196,15 +197,15 @@ export function _setDefaultOptions( ): ProfilingOptions { const endpoint = options.endpoint || - process.env.SPLUNK_PROFILER_LOGS_ENDPOINT || - process.env.OTEL_EXPORTER_OTLP_ENDPOINT || + getNonEmptyEnvVar('SPLUNK_PROFILER_LOGS_ENDPOINT') || + getNonEmptyEnvVar('OTEL_EXPORTER_OTLP_ENDPOINT') || 'http://localhost:4317'; const combinedResource = detectResource(); const serviceName = String( options.serviceName || - process.env.OTEL_SERVICE_NAME || + getNonEmptyEnvVar('OTEL_SERVICE_NAME') || combinedResource.attributes[SemanticResourceAttributes.SERVICE_NAME] || defaultServiceName() ); diff --git a/src/profiling/utils.ts b/src/profiling/utils.ts index 229b1017..9083c088 100644 --- a/src/profiling/utils.ts +++ b/src/profiling/utils.ts @@ -18,6 +18,7 @@ import { gzip } from 'zlib'; import { promisify } from 'util'; import * as grpc from '@grpc/grpc-js'; import { diag } from '@opentelemetry/api'; +import { getNonEmptyEnvVar } from '../utils'; import { perftools } from './proto/profile'; import type { HeapProfile, RawProfilingData } from './types'; @@ -251,9 +252,9 @@ export function parseEndpoint(endpoint: string): { if (endpoint.startsWith('https://')) { host = endpoint.substr(8); credentials = grpc.credentials.createSsl( - maybeReadPath(process.env.OTEL_EXPORTER_OTLP_CERTIFICATE), - maybeReadPath(process.env.OTEL_EXPORTER_OTLP_CLIENT_KEY), - maybeReadPath(process.env.OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE) + maybeReadPath(getNonEmptyEnvVar('OTEL_EXPORTER_OTLP_CERTIFICATE')), + maybeReadPath(getNonEmptyEnvVar('OTEL_EXPORTER_OTLP_CLIENT_KEY')), + maybeReadPath(getNonEmptyEnvVar('OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE')) ); } else if (endpoint.startsWith('http://')) { host = endpoint.substr(7); diff --git a/src/start.ts b/src/start.ts index deed9d9a..a1ec1d5f 100644 --- a/src/start.ts +++ b/src/start.ts @@ -15,6 +15,7 @@ */ import { assertNoExtraneousProperties, + getNonEmptyEnvVar, parseEnvBooleanString, parseLogLevel, toDiagLogLevel, @@ -25,7 +26,7 @@ import { StartProfilingOptions, _setDefaultOptions as setDefaultProfilingOptions, } from './profiling'; -import type { LogLevel } from './types'; +import type { EnvVarKey, LogLevel } from './types'; import { startTracing, stopTracing, StartTracingOptions } from './tracing'; import { diag, DiagConsoleLogger, DiagLogLevel } from '@opentelemetry/api'; @@ -54,10 +55,10 @@ const running: RunningState = { function isSignalEnabled( option: T | undefined | null, - envVar: string, + envVar: EnvVarKey, def: boolean ) { - return option ?? parseEnvBooleanString(process.env[envVar]) ?? def; + return option ?? parseEnvBooleanString(getNonEmptyEnvVar(envVar)) ?? def; } export const start = (options: Partial = {}) => { @@ -75,7 +76,7 @@ export const start = (options: Partial = {}) => { const logLevel = options.logLevel ? toDiagLogLevel(options.logLevel) - : parseLogLevel(process.env.OTEL_LOG_LEVEL); + : parseLogLevel(getNonEmptyEnvVar('OTEL_LOG_LEVEL')); if (logLevel !== DiagLogLevel.NONE) { diag.setLogger(new DiagConsoleLogger(), logLevel); diff --git a/src/tracing/index.ts b/src/tracing/index.ts index 12446315..178d62dd 100644 --- a/src/tracing/index.ts +++ b/src/tracing/index.ts @@ -36,7 +36,11 @@ import { configureHttpInstrumentation } from '../instrumentations/http'; import { configureLogInjection } from '../instrumentations/logging'; import { allowedTracingOptions, Options, _setDefaultOptions } from './options'; import { configureRedisInstrumentation } from '../instrumentations/redis'; -import { assertNoExtraneousProperties, parseEnvBooleanString } from '../utils'; +import { + assertNoExtraneousProperties, + getNonEmptyEnvVar, + parseEnvBooleanString, +} from '../utils'; import { isProfilingContextManagerSet } from '../profiling'; /** @@ -48,7 +52,7 @@ import { isProfilingContextManagerSet } from '../profiling'; * leaks and is inperfect in terms of the end result. */ const allowDoubleStart = parseEnvBooleanString( - process.env.TEST_ALLOW_DOUBLE_START + getNonEmptyEnvVar('TEST_ALLOW_DOUBLE_START') ); let isStarted = false; let tracingContextManagerEnabled = false; diff --git a/src/tracing/options.ts b/src/tracing/options.ts index df7156b2..e84cc9d4 100644 --- a/src/tracing/options.ts +++ b/src/tracing/options.ts @@ -33,6 +33,7 @@ import { getEnvArray, getEnvBoolean, getEnvValueByPrecedence, + getNonEmptyEnvVar, } from '../utils'; import { NodeTracerConfig } from '@opentelemetry/sdk-trace-node'; import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions'; @@ -89,14 +90,14 @@ export const allowedTracingOptions = [ export function _setDefaultOptions(options: Partial = {}): Options { process.env.OTEL_SPAN_LINK_COUNT_LIMIT = - process.env.OTEL_SPAN_LINK_COUNT_LIMIT ?? '1000'; + getNonEmptyEnvVar('OTEL_SPAN_LINK_COUNT_LIMIT') ?? '1000'; process.env.OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT = - process.env.OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT ?? '12000'; + getNonEmptyEnvVar('OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT') ?? '12000'; options.accessToken = - options.accessToken || process.env.SPLUNK_ACCESS_TOKEN || ''; + options.accessToken || getNonEmptyEnvVar('SPLUNK_ACCESS_TOKEN') || ''; - options.realm = options.realm || process.env.SPLUNK_REALM; + options.realm = options.realm || getNonEmptyEnvVar('SPLUNK_REALM'); if (options.realm) { if (!options.accessToken) { @@ -119,7 +120,7 @@ export function _setDefaultOptions(options: Partial = {}): Options { const serviceName = options.serviceName || - process.env.OTEL_SERVICE_NAME || + getNonEmptyEnvVar('OTEL_SERVICE_NAME') || resource.attributes[SemanticResourceAttributes.SERVICE_NAME]; if (!serviceName) { @@ -313,7 +314,7 @@ export function consoleSpanExporterFactory(): SpanExporter { // Temporary workaround until https://github.com/open-telemetry/opentelemetry-js/issues/3094 is resolved function getBatchSpanProcessorConfig() { // OTel uses its own parsed environment, we can just use the default env if the BSP delay is unset. - if (process.env.OTEL_BSP_SCHEDULE_DELAY !== undefined) { + if (getNonEmptyEnvVar('OTEL_BSP_SCHEDULE_DELAY') !== undefined) { return undefined; } diff --git a/src/types.ts b/src/types.ts index d81dc8fb..5f5e544e 100644 --- a/src/types.ts +++ b/src/types.ts @@ -15,3 +15,37 @@ */ export type LogLevel = 'none' | 'verbose' | 'debug' | 'info' | 'warn' | 'error'; + +export type EnvVarKey = + | 'OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT' + | 'OTEL_BSP_SCHEDULE_DELAY' + | 'OTEL_EXPORTER_OTLP_CERTIFICATE' + | 'OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE' + | 'OTEL_EXPORTER_OTLP_CLIENT_KEY' + | 'OTEL_EXPORTER_OTLP_ENDPOINT' + | 'OTEL_EXPORTER_OTLP_METRICS_PROTOCOL' + | 'OTEL_EXPORTER_OTLP_PROTOCOL' + | 'OTEL_EXPORTER_OTLP_TRACES_ENDPOINT' + | 'OTEL_EXPORTER_OTLP_TRACES_PROTOCOL' + | 'OTEL_LOG_LEVEL' + | 'OTEL_METRIC_EXPORT_INTERVAL' + | 'OTEL_METRICS_EXPORTER' + | 'OTEL_PROPAGATORS' + | 'OTEL_SERVICE_NAME' + | 'OTEL_SPAN_LINK_COUNT_LIMIT' + | 'OTEL_TRACES_EXPORTER' + | 'SPLUNK_ACCESS_TOKEN' + | 'SPLUNK_AUTOINSTRUMENT_PACKAGE_NAMES' + | 'SPLUNK_METRICS_ENABLED' + | 'SPLUNK_METRICS_ENDPOINT' + | 'SPLUNK_PROFILER_CALL_STACK_INTERVAL' + | 'SPLUNK_PROFILER_ENABLED' + | 'SPLUNK_PROFILER_LOGS_ENDPOINT' + | 'SPLUNK_PROFILER_MEMORY_ENABLED' + | 'SPLUNK_REALM' + | 'SPLUNK_REDIS_INCLUDE_COMMAND_ARGS' + | 'SPLUNK_RUNTIME_METRICS_COLLECTION_INTERVAL' + | 'SPLUNK_RUNTIME_METRICS_ENABLED' + | 'SPLUNK_TRACE_RESPONSE_HEADER_ENABLED' + | 'SPLUNK_TRACING_ENABLED' + | 'TEST_ALLOW_DOUBLE_START'; diff --git a/src/utils.ts b/src/utils.ts index bd4cea6f..e7b8bdfc 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -16,7 +16,7 @@ import { strict as assert } from 'assert'; import { diag, DiagLogLevel } from '@opentelemetry/api'; -import type { LogLevel } from './types'; +import type { EnvVarKey, LogLevel } from './types'; import { resolve } from 'path'; import * as fs from 'fs'; @@ -54,6 +54,24 @@ export function defaultServiceName(cache: ConfigCache = configCache): string { return findServiceName(cache) || 'unnamed-node-service'; } +export function getNonEmptyEnvVar(key: EnvVarKey): string | undefined { + const value = process.env[key]; + + if (value !== undefined) { + const trimmed = value.trim(); + if (trimmed.length === 0) { + diag.warn( + `Defined, but empty environment variable: '${key}'. The value will be considered as undefined.` + ); + return undefined; + } + + return trimmed; + } + + return value; +} + export function parseEnvBooleanString(value?: string) { if (typeof value !== 'string') { return value; @@ -72,8 +90,8 @@ export function parseEnvBooleanString(value?: string) { throw new Error(`Invalid string representing boolean: ${value}`); } -export function getEnvBoolean(key: string, defaultValue = true) { - const value = process.env[key]; +export function getEnvBoolean(key: EnvVarKey, defaultValue = true) { + const value = getNonEmptyEnvVar(key); if (value === undefined) { return defaultValue; @@ -86,8 +104,8 @@ export function getEnvBoolean(key: string, defaultValue = true) { return true; } -export function getEnvNumber(key: string, defaultValue: number): number { - const value = process.env[key]; +export function getEnvNumber(key: EnvVarKey, defaultValue: number): number { + const value = getNonEmptyEnvVar(key); if (value === undefined) { return defaultValue; @@ -106,8 +124,8 @@ export function deduplicate(arr: string[]) { return [...new Set(arr)]; } -export function getEnvArray(key: string, defaultValue: string[]): string[] { - const value = process.env[key]; +export function getEnvArray(key: EnvVarKey, defaultValue: string[]): string[] { + const value = getNonEmptyEnvVar(key); if (value === undefined) { return defaultValue; @@ -117,11 +135,11 @@ export function getEnvArray(key: string, defaultValue: string[]): string[] { } export function getEnvValueByPrecedence( - keys: string[], + keys: EnvVarKey[], defaultValue?: string ): string | undefined { for (const key of keys) { - const value = process.env[key]; + const value = getNonEmptyEnvVar(key); if (value !== undefined) { return value; diff --git a/test/examples/collector.config.yml b/test/examples/collector.config.yml index d9ddd9ba..aee6c640 100644 --- a/test/examples/collector.config.yml +++ b/test/examples/collector.config.yml @@ -20,7 +20,7 @@ service: pipelines: traces: receivers: [otlp] - exporters: [httpsink] + exporters: [httpsink, logging/debug] logs/profiling: receivers: [otlp] exporters: [logging/debug] diff --git a/test/utils.test.ts b/test/utils.test.ts index a28d95fa..7ea649e4 100644 --- a/test/utils.test.ts +++ b/test/utils.test.ts @@ -14,8 +14,10 @@ * limitations under the License. */ import * as assert from 'assert'; +import * as sinon from 'sinon'; +import * as api from '@opentelemetry/api'; -import { parseLogLevel } from '../src/utils'; +import { getNonEmptyEnvVar, parseLogLevel } from '../src/utils'; import { cleanEnvironment } from './utils'; import { DiagLogLevel } from '@opentelemetry/api'; @@ -33,4 +35,52 @@ describe('utils', () => { assert.deepStrictEqual(parseLogLevel('ERROR'), DiagLogLevel.ERROR); }); }); + + describe('getNonEmptyEnvVar', () => { + const sandbox = sinon.createSandbox(); + let logger; + + beforeEach(() => { + cleanEnvironment(); + logger = { + warn: sandbox.spy(), + }; + api.diag.setLogger(logger, api.DiagLogLevel.ALL); + logger.warn.resetHistory(); + }); + + afterEach(() => { + sandbox.restore(); + }); + + it('returns an empty environment variable as undefined', () => { + process.env.OTEL_SERVICE_NAME = ''; + assert.deepStrictEqual(getNonEmptyEnvVar('OTEL_SERVICE_NAME'), undefined); + }); + + it('returns a whitespace environment variable as undefined', () => { + process.env.OTEL_SERVICE_NAME = ' '; + assert.deepStrictEqual(getNonEmptyEnvVar('OTEL_SERVICE_NAME'), undefined); + }); + + it('returns a defined environment variable', () => { + process.env.OTEL_SERVICE_NAME = 'TEST'; + assert.deepStrictEqual(getNonEmptyEnvVar('OTEL_SERVICE_NAME'), 'TEST'); + }); + + it('trims whitespace', () => { + process.env.OTEL_SERVICE_NAME = ' TEST '; + assert.deepStrictEqual(getNonEmptyEnvVar('OTEL_SERVICE_NAME'), 'TEST'); + }); + + it('warns when reading an empty environment variable', () => { + process.env.OTEL_SERVICE_NAME = ''; + assert.deepStrictEqual(getNonEmptyEnvVar('OTEL_SERVICE_NAME'), undefined); + + sinon.assert.calledWith( + logger.warn, + `Defined, but empty environment variable: 'OTEL_SERVICE_NAME'. The value will be considered as undefined.` + ); + }); + }); });