Skip to content

Commit 991d15c

Browse files
authored
refactor(config): route env reads through the config singleton (#8241)
* refactor(config): route env reads through the config singleton Replace `getValueFromEnvSources(...)` with config singleton reads across CI visibility, runtime metrics, Azure metadata, AWS SDK, the git-cache helper, and a few smaller helpers. The singleton already applies parsers, alias precedence, and origin tracking; the helper bypasses all three and re-walks the source order on every call. A few sites need more than a drop-in replacement: 1. `serverless.js` lazily requires `./config` inside `enableGCPPubSubPushSubscription` to break the `config -> serverless -> config` import cycle. 2. `aws-sdk/base.js` needs to check `config.getOrigin(key) === 'default'`. As a side effect, programmatic `batchPropagationEnabled` now wins over env vars in every permutation; master's `??` chain happened to let the service-specific env var beat a top-level programmatic value, which was the one rung that disagreed with how every other plugin option resolves. 3. `git-cache.js` defers the env read into an idempotent `ensureCacheDir()` first called from `cachedExec()`. The integration test also invalidates `packages/dd-trace/src/config` from the require cache so the next `require(...)` rebuilds the singleton from the just-set env vars. 4. `getJobIDFromDiagFile` reads `RUNNER_TEMP`, which is not a DD env var, so it goes through `getEnvironmentVariable` rather than the singleton. Unit tests use a proxyquire-injected `() => proxyquire.noPreserveCache()(.../config, {})()` so per-test `process.env` mutations still drive a fresh singleton. `runtime_metrics.spec.js`'s synthetic config object gains `DD_RUNTIME_METRICS_FLUSH_INTERVAL`. The aws-sdk plugin spec hoists `process.env.DD_TRACE_AWS_SDK_*_BATCH_PROPAGATION_ENABLED` into the outer `describe('Plugin')` so the env vars are set before the singleton is first built. A new `serverless.spec.js` covers the cycle-broken branch directly. Two call sites stay on the helper: 1. `llmobs/sdk.js`'s `enable()` reads `DD_LLMOBS_ENABLED` at call time so user code (and the existing test) can mutate the env var after `tracer.init()` and have it honored. The singleton snapshot can't reflect a post-init mutation. 2. `test-optimization-cache.js`'s `setupSettingsCachePath` writes `process.env` so NYC subprocesses inherit the path; the singleton is a one-shot snapshot of the parent's env. Drive-by fix: * The `DD_TRACE_AWS_SDK_*_BATCH_PROPAGATION_ENABLED` entries in `supported-configurations.json` flip from `default: "true"` / `implementation: "A"` to `default: "false"` / `implementation: "B"`. The shipped chain has always defaulted to `false` via `isTrue(undefined)`. * fix(serverless): keep Pub/Sub push check on the env helper `enableGCPPubSubPushSubscription` lazy-required `./config` to break the `config -> serverless -> config` import cycle. `HttpPlugin.plugins` calls it on every `tracer.use('http')`, and each `proxyquire.noPreserveCache( .../config)` in the test harness clears `config/index.js` from `require.cache`. The next call then re-evaluates the module and builds a fresh `Config()` (parsing `supported-configurations.json`, walking `process.env`, reading git files). AppSec / express CI timed out three RASP-SSRF `before all` hooks at 5 s. Read `DD_TRACE_GCP_PUBSUB_PUSH_ENABLED` via `getValueFromEnvSources` and inline it so `K_SERVICE !== undefined` short-circuits the env lookup. Drive-by fix: * `.github/CODEOWNERS`: route the new `test/serverless.spec.js` to `@DataDog/apm-serverless`, matching `azure_metadata.spec.js` so `lint:codeowners:ci` stops failing on it.
1 parent 423ac93 commit 991d15c

20 files changed

Lines changed: 231 additions & 118 deletions

File tree

.github/CODEOWNERS

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
/packages/dd-trace/src/lambda/ @DataDog/serverless-aws @DataDog/apm-serverless
3333
/packages/dd-trace/test/lambda/ @DataDog/serverless-aws @DataDog/apm-serverless
3434
/packages/dd-trace/test/azure_metadata.spec.js @DataDog/apm-serverless
35+
/packages/dd-trace/test/serverless.spec.js @DataDog/apm-serverless
3536
/packages/dd-trace/test/plugins/util/inferred_proxy.spec.js @DataDog/serverless-aws @DataDog/apm-serverless
3637

3738
# IDM

integration-tests/ci-visibility/git-cache.spec.js

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,11 @@ describe('git-cache integration tests', () => {
4545
beforeEach(() => {
4646
process.env.DD_EXPERIMENTAL_TEST_OPT_GIT_CACHE_ENABLED = 'true'
4747
process.env.DD_EXPERIMENTAL_TEST_OPT_GIT_CACHE_DIR = cacheDir
48-
// We need this, otherwise the file is already loaded and the cache is disabled
48+
// git-cache reads its config from the dd-trace config singleton on first
49+
// use; clearing both modules from the require cache makes the next
50+
// `require(...)` rebuild the singleton from the env we just set.
4951
delete require.cache[require.resolve('../../packages/dd-trace/src/plugins/util/git-cache')]
52+
delete require.cache[require.resolve('../../packages/dd-trace/src/config')]
5053
gitCache = require('../../packages/dd-trace/src/plugins/util/git-cache')
5154

5255
originalPath = process.env.PATH
@@ -148,6 +151,7 @@ describe('git-cache integration tests', () => {
148151
delete process.env.DD_EXPERIMENTAL_TEST_OPT_GIT_CACHE_ENABLED
149152

150153
delete require.cache[require.resolve('../../packages/dd-trace/src/plugins/util/git-cache')]
154+
delete require.cache[require.resolve('../../packages/dd-trace/src/config')]
151155
const disabledGitCache = require('../../packages/dd-trace/src/plugins/util/git-cache')
152156

153157
const firstResult = disabledGitCache.cachedExec('git', GET_COMMIT_MESSAGE_COMMAND_ARGS)
@@ -172,11 +176,39 @@ describe('git-cache integration tests', () => {
172176
assert.match(secondError.message, /git/)
173177
})
174178

179+
it('should cache when DD_EXPERIMENTAL_TEST_OPT_GIT_CACHE_ENABLED is true and cache dir is unset', function () {
180+
delete process.env.DD_EXPERIMENTAL_TEST_OPT_GIT_CACHE_DIR
181+
182+
delete require.cache[require.resolve('../../packages/dd-trace/src/plugins/util/git-cache')]
183+
const defaultDirGitCache = require('../../packages/dd-trace/src/plugins/util/git-cache')
184+
185+
const firstResultStr = String(defaultDirGitCache.cachedExec('git', GET_COMMIT_MESSAGE_COMMAND_ARGS)).trim()
186+
assert.strictEqual(firstResultStr, FIXED_COMMIT_MESSAGE)
187+
188+
const cacheKey = defaultDirGitCache.getCacheKey('git', GET_COMMIT_MESSAGE_COMMAND_ARGS)
189+
const cacheFilePath = defaultDirGitCache.getCacheFilePath(cacheKey)
190+
assert.ok(cacheFilePath.includes('dd-trace-git-cache'))
191+
assert.strictEqual(fs.existsSync(cacheFilePath), true)
192+
193+
removeGitFromPath()
194+
195+
const secondResult = String(defaultDirGitCache.cachedExec('git', GET_COMMIT_MESSAGE_COMMAND_ARGS)).trim()
196+
assert.strictEqual(secondResult, firstResultStr)
197+
198+
fs.rmSync(cacheFilePath, { force: true })
199+
try {
200+
fs.rmdirSync(path.dirname(cacheFilePath))
201+
} catch {
202+
// Ignore, if the directory is not empty
203+
}
204+
})
205+
175206
context('invalid DD_EXPERIMENTAL_TEST_OPT_GIT_CACHE_DIR', () => {
176207
function runInvalidCacheTest (invalidCacheDir) {
177208
process.env.DD_EXPERIMENTAL_TEST_OPT_GIT_CACHE_DIR = invalidCacheDir
178209

179210
delete require.cache[require.resolve('../../packages/dd-trace/src/plugins/util/git-cache')]
211+
delete require.cache[require.resolve('../../packages/dd-trace/src/config')]
180212
const invalidDirGitCache = require('../../packages/dd-trace/src/plugins/util/git-cache')
181213

182214
const firstResult = invalidDirGitCache.cachedExec('git', GET_COMMIT_MESSAGE_COMMAND_ARGS)

packages/datadog-plugin-aws-sdk/src/base.js

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,8 @@
33
const analyticsSampler = require('../../dd-trace/src/analytics_sampler')
44
const ClientPlugin = require('../../dd-trace/src/plugins/client')
55
const { storage } = require('../../datadog-core')
6-
const { isTrue } = require('../../dd-trace/src/util')
76
const { tagsFromRequest, tagsFromResponse } = require('../../dd-trace/src/payload-tagging')
8-
const { getValueFromEnvSources } = require('../../dd-trace/src/config/helper')
7+
const getConfig = require('../../dd-trace/src/config')
98
const { IS_SERVERLESS } = require('../../dd-trace/src/serverless')
109

1110
const RESPONSE_SKIP_KEYS = new Set(['request', 'requestId', 'error', '$metadata'])
@@ -313,29 +312,25 @@ function normalizeConfig (config, serviceIdentifier) {
313312
const hooks = getHooks(config)
314313

315314
let specificConfig = config[serviceIdentifier]
316-
switch (typeof specificConfig) {
317-
case 'undefined':
318-
specificConfig = {}
319-
break
320-
case 'boolean':
321-
specificConfig = { enabled: specificConfig }
322-
break
315+
if (typeof specificConfig === 'boolean') {
316+
specificConfig = { enabled: specificConfig }
323317
}
324318

325-
// check if AWS batch propagation or AWS_[SERVICE] batch propagation is enabled via env variable
319+
// Check if AWS batch propagation or AWS_[SERVICE] batch propagation is enabled via env variable
320+
const tracerConfig = getConfig()
326321
const serviceId = serviceIdentifier.toUpperCase()
327-
const batchPropagationEnabled = isTrue(
328-
specificConfig.batchPropagationEnabled ??
329-
getValueFromEnvSources(`DD_TRACE_AWS_SDK_${serviceId}_BATCH_PROPAGATION_ENABLED`) ??
330-
config.batchPropagationEnabled ??
331-
getValueFromEnvSources('DD_TRACE_AWS_SDK_BATCH_PROPAGATION_ENABLED')
322+
const serviceBatchKey = /** @type {import('../../dd-trace/src/config/config-types').ConfigPath} */(
323+
`DD_TRACE_AWS_SDK_${serviceId}_BATCH_PROPAGATION_ENABLED`
332324
)
325+
const batchPropagationEnabled = tracerConfig.getOrigin(serviceBatchKey) === 'default'
326+
? tracerConfig.DD_TRACE_AWS_SDK_BATCH_PROPAGATION_ENABLED
327+
: tracerConfig[serviceBatchKey]
333328

334329
// Merge the specific config back into the main config
335330
return {
331+
batchPropagationEnabled,
336332
...config,
337333
...specificConfig,
338-
batchPropagationEnabled,
339334
hooks,
340335
}
341336
}

packages/datadog-plugin-aws-sdk/test/aws-sdk.spec.js

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,31 @@ const { assertObjectContains } = require('../../../integration-tests/helpers')
1111
const { setup, sort, withAwsSdkV2Versions, withAwsSdkVersions } = require('./spec_helpers')
1212

1313
describe('Plugin', () => {
14+
// The config singleton is built lazily on the first `agent.load(...)` and is
15+
// not rebuilt across describes, so any env var the singleton needs to see
16+
// must be set before then. The 'with env variable _BATCH_PROPAGATION_ENABLED'
17+
// describe asserts on these specific values; they're harmless to the other
18+
// describes (which assert on programmatic config that wins in the `??` chain).
19+
const ORIGINAL_BATCH_PROPAGATION_ENV = {
20+
GLOBAL: process.env.DD_TRACE_AWS_SDK_BATCH_PROPAGATION_ENABLED,
21+
KINESIS: process.env.DD_TRACE_AWS_SDK_KINESIS_BATCH_PROPAGATION_ENABLED,
22+
SQS: process.env.DD_TRACE_AWS_SDK_SQS_BATCH_PROPAGATION_ENABLED,
23+
}
24+
before(() => {
25+
process.env.DD_TRACE_AWS_SDK_BATCH_PROPAGATION_ENABLED = 'true'
26+
process.env.DD_TRACE_AWS_SDK_KINESIS_BATCH_PROPAGATION_ENABLED = 'false'
27+
process.env.DD_TRACE_AWS_SDK_SQS_BATCH_PROPAGATION_ENABLED = 'true'
28+
})
29+
after(() => {
30+
function restore (name, original) {
31+
if (original === undefined) delete process.env[name]
32+
else process.env[name] = original
33+
}
34+
restore('DD_TRACE_AWS_SDK_BATCH_PROPAGATION_ENABLED', ORIGINAL_BATCH_PROPAGATION_ENV.GLOBAL)
35+
restore('DD_TRACE_AWS_SDK_KINESIS_BATCH_PROPAGATION_ENABLED', ORIGINAL_BATCH_PROPAGATION_ENV.KINESIS)
36+
restore('DD_TRACE_AWS_SDK_SQS_BATCH_PROPAGATION_ENABLED', ORIGINAL_BATCH_PROPAGATION_ENV.SQS)
37+
})
38+
1439
// TODO: use the Request class directly for generic tests
1540
// TODO: add test files for every service
1641
describe('aws-sdk direct import', function () {

packages/datadog-plugin-cypress/src/cypress-plugin.js

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ const {
7373
} = require('../../dd-trace/src/plugins/util/test')
7474
const { isMarkedAsUnskippable } = require('../../datadog-plugin-jest/src/util')
7575
const { ORIGIN_KEY, COMPONENT } = require('../../dd-trace/src/constants')
76-
const { getValueFromEnvSources } = require('../../dd-trace/src/config/helper')
76+
const getConfig = require('../../dd-trace/src/config')
7777
const { appClosing: appClosingTelemetry } = require('../../dd-trace/src/telemetry')
7878
const log = require('../../dd-trace/src/log')
7979

@@ -505,8 +505,7 @@ class CypressPlugin {
505505

506506
this.isTestIsolationEnabled = getIsTestIsolationEnabled(cypressConfig)
507507

508-
const envFlushWait = Number(getValueFromEnvSources('DD_CIVISIBILITY_RUM_FLUSH_WAIT_MILLIS'))
509-
this.rumFlushWaitMillis = Number.isFinite(envFlushWait) ? envFlushWait : undefined
508+
this.rumFlushWaitMillis = getConfig().DD_CIVISIBILITY_RUM_FLUSH_WAIT_MILLIS
510509

511510
if (!this.isTestIsolationEnabled) {
512511
log.warn('Test isolation is disabled, retries will not be enabled')
@@ -995,7 +994,7 @@ class CypressPlugin {
995994
this.ciVisEvent(TELEMETRY_EVENT_FINISHED, 'session')
996995
incrementCountMetric(TELEMETRY_TEST_SESSION, {
997996
provider: this.ciProviderName,
998-
autoInjected: !!getValueFromEnvSources('DD_CIVISIBILITY_AUTO_INSTRUMENTATION_PROVIDER'),
997+
autoInjected: !!getConfig().DD_CIVISIBILITY_AUTO_INSTRUMENTATION_PROVIDER,
999998
})
1000999

10011000
finishAllTraceSpans(this.testSessionSpan)

packages/dd-trace/src/azure_metadata.js

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,8 @@
33
// Modeled after https://github.com/DataDog/libdatadog/blob/f3994857a59bb5679a65967138c5a3aec418a65f/ddcommon/src/azure_app_services.rs
44

55
const os = require('os')
6-
const {
7-
getEnvironmentVariable,
8-
getValueFromEnvSources,
9-
} = require('./config/helper')
6+
const getConfig = require('./config')
7+
const { getEnvironmentVariable } = require('./config/helper')
108
const { getIsAzureFunction } = require('./serverless')
119

1210
function extractSubscriptionID (ownerName) {
@@ -69,7 +67,7 @@ function buildMetadata () {
6967
const WEBSITE_SITE_NAME = getEnvironmentVariable('WEBSITE_SITE_NAME')
7068
const WEBSITE_SKU = getEnvironmentVariable('WEBSITE_SKU')
7169

72-
const DD_AZURE_RESOURCE_GROUP = getValueFromEnvSources('DD_AZURE_RESOURCE_GROUP')
70+
const { DD_AZURE_RESOURCE_GROUP } = getConfig()
7371
const isAzureFunction = FUNCTIONS_EXTENSION_VERSION !== undefined && FUNCTIONS_WORKER_RUNTIME !== undefined
7472
const isFlexConsumptionAzureFunction = isAzureFunction && WEBSITE_SKU === 'FlexConsumption'
7573

packages/dd-trace/src/ci-visibility/exporters/test-worker/index.js

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,12 @@ const {
1111
VITEST_WORKER_TRACE_PAYLOAD_CODE,
1212
VITEST_WORKER_LOGS_PAYLOAD_CODE,
1313
} = require('../../../plugins/util/test')
14-
const { getEnvironmentVariable, getValueFromEnvSources } = require('../../../config/helper')
14+
const getConfig = require('../../../config')
15+
const { getEnvironmentVariable } = require('../../../config/helper')
1516
const Writer = require('./writer')
1617

1718
function getInterprocessTraceCode () {
19+
const { DD_PLAYWRIGHT_WORKER, DD_VITEST_WORKER } = getConfig()
1820
if (getEnvironmentVariable('JEST_WORKER_ID')) {
1921
return JEST_WORKER_TRACE_PAYLOAD_CODE
2022
}
@@ -24,13 +26,13 @@ function getInterprocessTraceCode () {
2426
if (getEnvironmentVariable('MOCHA_WORKER_ID')) {
2527
return MOCHA_WORKER_TRACE_PAYLOAD_CODE
2628
}
27-
if (getValueFromEnvSources('DD_PLAYWRIGHT_WORKER')) {
29+
if (DD_PLAYWRIGHT_WORKER) {
2830
return PLAYWRIGHT_WORKER_TRACE_PAYLOAD_CODE
2931
}
3032
if (getEnvironmentVariable('TINYPOOL_WORKER_ID')) {
3133
return VITEST_WORKER_TRACE_PAYLOAD_CODE
3234
}
33-
if (getValueFromEnvSources('DD_VITEST_WORKER')) {
35+
if (DD_VITEST_WORKER) {
3436
return VITEST_WORKER_TRACE_PAYLOAD_CODE
3537
}
3638
return null
@@ -51,7 +53,7 @@ function getInterprocessLogsCode () {
5153
if (getEnvironmentVariable('TINYPOOL_WORKER_ID')) {
5254
return VITEST_WORKER_LOGS_PAYLOAD_CODE
5355
}
54-
if (getValueFromEnvSources('DD_VITEST_WORKER')) {
56+
if (getConfig().DD_VITEST_WORKER) {
5557
return VITEST_WORKER_LOGS_PAYLOAD_CODE
5658
}
5759
return null

packages/dd-trace/src/config/generated-config-types.d.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ export interface GeneratedConfig {
9090
DD_ENABLE_LAGE_PACKAGE_NAME: boolean;
9191
DD_ENABLE_NX_SERVICE_NAME: boolean;
9292
DD_EXPERIMENTAL_PROPAGATE_PROCESS_TAGS_ENABLED: boolean;
93-
DD_EXPERIMENTAL_TEST_OPT_GIT_CACHE_DIR: string;
93+
DD_EXPERIMENTAL_TEST_OPT_GIT_CACHE_DIR: string | undefined;
9494
DD_EXPERIMENTAL_TEST_OPT_GIT_CACHE_ENABLED: boolean;
9595
DD_EXPERIMENTAL_TEST_OPT_SETTINGS_CACHE: string;
9696
DD_EXPERIMENTAL_TEST_REQUESTS_FS_CACHE: boolean;

0 commit comments

Comments
 (0)