-
Notifications
You must be signed in to change notification settings - Fork 360
Add class ConfigEnvSources to merge stable config and environment variables #6982
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Overall package sizeSelf size: 4.43 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 2.0.0 | 68.46 kB | 797.03 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6982 +/- ##
==========================================
+ Coverage 85.14% 85.17% +0.03%
==========================================
Files 532 532
Lines 22897 22922 +25
==========================================
+ Hits 19495 19524 +29
+ Misses 3402 3398 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2026-01-20 01:44:41 Comparing candidate commit af99299 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 230 metrics, 30 unstable metrics. |
eacb99d to
a532c6a
Compare
packages/dd-trace/src/ci-visibility/exporters/test-worker/index.js
Outdated
Show resolved
Hide resolved
282f094 to
948ddde
Compare
| localStableConfig: localStableConfig ?? {}, | ||
| fleetStableConfig: fleetStableConfig ?? {}, | ||
| stableConfigWarnings: stableConfigWarnings ?? [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see these 3 variables will never be undefined, so this could be written as:
| localStableConfig: localStableConfig ?? {}, | |
| fleetStableConfig: fleetStableConfig ?? {}, | |
| stableConfigWarnings: stableConfigWarnings ?? [] | |
| localStableConfig, | |
| fleetStableConfig, | |
| stableConfigWarnings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you said in some of your other comments, this is not strictly true. In case of a serverless environment, these will be undefined. However, in the code we never actually call this function in that case, so it should work without the undefined check. However, the JSDoc @returns is now technically wrong I guess. But it's all a mess because if you guard the call getStableConfigSources with a check for if we're not in serverless, then they are guaranteed to be objects, so 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I just checked the overall calls and we guard against calling this in config.js by first checking if the environment is not serverless.
I guess it is a minimal risk to not have the fallback in case a refactoring would ever change that in a weird way. Otherwise this should be good as is. That also applies to the JSDoc as such 😅
I am fine either way. What is your preferred way of handling this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to keep it as is
21955bc to
6f3422a
Compare
| localStableConfig: localStableConfig ?? {}, | ||
| fleetStableConfig: fleetStableConfig ?? {}, | ||
| stableConfigWarnings: stableConfigWarnings ?? [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you said in some of your other comments, this is not strictly true. In case of a serverless environment, these will be undefined. However, in the code we never actually call this function in that case, so it should work without the undefined check. However, the JSDoc @returns is now technically wrong I guess. But it's all a mess because if you guard the call getStableConfigSources with a check for if we're not in serverless, then they are guaranteed to be objects, so 🤷
| 'Datadog-Meta-Lang': 'nodejs', | ||
| 'Datadog-Meta-Lang-Version': process.version, | ||
| 'Datadog-Meta-Lang-Interpreter': process.jsEngine || 'v8' | ||
| 'Datadog-Meta-Lang-Interpreter': process.versions.bun ? 'JavaScriptCore' : 'V8' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I believe this is just a value we read, while it could theoretically have an impact on dashboards that look for the lowercase version (most cases would be case insensitive anyway though).
| makeRequest(_protocolVersion, data, count, _url, _headers, _lookup, true, (err, res, status) => { | ||
| makeRequest(_protocolVersion, data, count, _url, _headers, _lookup, (err, res, status) => { | ||
| // Note that logging will only happen once, regardless of how many times this is called. | ||
| startupLog(status !== 404 && status !== 200 ? { status, message: err?.message ?? inspect(err) } : undefined) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I am uncertain about the fallback (it should never happen that an error has no message, but just in theory). We would receive the full stacktrace in case it's still a regular error, which would be likely.
| errors.agentError = { | ||
| code: agentError.code ?? '', | ||
| message: `Agent Error:${agentError.message}` | ||
| code: agentError.status, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am actually uncertain, if this is definitely correct. I have to verify that again.
| if (errors.agentError) { | ||
| app.error = errors.agentError | ||
| errors.agentError = undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure why this was never activated before. I am going to try to verify that before landing.
| architecture: os.arch(), | ||
| os_version: os.version() | ||
| } | ||
| if (os.platform() === 'win32') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This is actually a bug fix.
c745a69 to
b1308bc
Compare
| fleetConfigPath = '/etc/datadog-agent/managed/datadog-agent/stable/application_monitoring.yaml' | ||
| break | ||
| #getStableConfigPaths () { | ||
| // TODO(BridgeAR): Remove these environment variables once we have a proper way to test the stable config. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As much as we'd like to avoid mocking .... would a filesystem mock suffice here? I think it's sufficiently low level and not mocking dd-trace functionality that it might be appropriate here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would work. I just did not want to make even more changes 😅
| function getInterprocessTraceCode () { | ||
| if (getEnvironmentVariable('JEST_WORKER_ID')) { | ||
| return JEST_WORKER_TRACE_PAYLOAD_CODE | ||
| } | ||
| if (getEnvironmentVariable('CUCUMBER_WORKER_ID')) { | ||
| return CUCUMBER_WORKER_TRACE_PAYLOAD_CODE | ||
| } | ||
| if (getEnvironmentVariable('MOCHA_WORKER_ID')) { | ||
| return MOCHA_WORKER_TRACE_PAYLOAD_CODE | ||
| } | ||
| if (getEnvironmentVariable('DD_PLAYWRIGHT_WORKER')) { | ||
| if (getValueFromEnvSources('DD_PLAYWRIGHT_WORKER')) { | ||
| return PLAYWRIGHT_WORKER_TRACE_PAYLOAD_CODE | ||
| } | ||
| if (getEnvironmentVariable('TINYPOOL_WORKER_ID')) { | ||
| return VITEST_WORKER_TRACE_PAYLOAD_CODE | ||
| } | ||
| if (getEnvironmentVariable('DD_VITEST_WORKER')) { | ||
| if (getValueFromEnvSources('DD_VITEST_WORKER')) { | ||
| return VITEST_WORKER_TRACE_PAYLOAD_CODE | ||
| } | ||
| return null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like eyes of @juan-fernandez on the test-viz changes, some of the env vars here are DD_ and some are not. Should we really change it to geTValueFromEnvSources ? Is having a mix of getEnvironementVariable() and getValueFromEnvSources() the behavior we want ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe having this mix of behaviors depending on the env var name in this function will cause unexpected behavior to customers or even to our own devs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to check, what our RFCs say about stable config and non DD_ envs. It would only depend on that.
fix circular dependency Migrate civis call to use getEnvironmnetVariableSources instead run linter cleanup comments on config-env-sources complete ci/init.js migration migrate otel-sdk-trace.js assert getResolvedEnv behaves just like getEnvironmentVariable when only envs exist migrate register.js migrate DD_TRACE_AWS_SDK_BATCH_PROPAGATION_ENABLED migrate DD_TRACE_AZURE_EVENTHUBS_BATCH_LINKS_ENABLED and DD_TRACE_AZURE_SERVICEBUS_BATCH_LINKS_ENABLED migrate cypress and cucumber DD_CIVISIBILITY_AUTO_INSTRUMENTATION_PROVIDER migrate dd-trace-api, mocha and jest migrate mongodb, playwright and vitest plugins migrate index.js -- specifically for disablement env vars -- and update tests to call resetEnvConfigSources migrate plugin_manager migrate DD_API_KEY in proxy.js Finish bulk migration fix debugger test: reset configenvsources Fix failing tests in ci fix failing lambda tets and aws-sdk tests whoops - include all changes to lambda tests migrate DD_TRACE_ENCODING_DEBUG and DD_TRACE_EXPERIMENTAL_STATE_TRACKING Try out not cachine env vars update config-env-sources tests accordingly Remove calls to resetConfigEnvSources in legacy tests nits restore exporter.spec.js have Config re-use the stable config entries from ConfigEnvSources Add doc for ConfigEnvSources drop config-env-sources; just use config-helper delete config env sources try again to delete config-env-sources Apply getValueFromEnvSources to rest of DD calls; fix serverless.js fix ritm fix more referecnes to config env sources fix another import fix calls to getEnvironmentVariables that need getValueFromEnvSources pass 'config' all the way down into profiler; remove duplicate config resolution logic use getValueFromEnvSources in profiler for DD_ configs leave TODO
Potentially split this into multiple PRs
b1308bc to
af99299
Compare
Reviewer tips
The majority of code changes are just replacing
getEnvironmentVariablewithgetValueFromEnvSources. Feel free to gloss over those changes once you get the idea from a few, and instead focus on:Background
Currently, stable config files and environment variables are only processed when the Config class is instantiated, which happens in tracer.init(). This limits us from earlier access: code that runs before tracer.init() can't access stable config values.
This PR allows us to access stable config values before tracer.init(), when we peek at DD_TRACE env vars.
It also deduplicates configuration sourcing in profiler.
What does this PR do?
Introduces
getValueFromEnvSourcesAPI in config-helper.js that resolves configuration from local stable config, environment variables, and fleet stable config in ascending priority order, before theConfigsingleton is initialized. This method can be used instead ofgetEnvironmentVariablewhen we want to see whether particular DD_TRACE configs have been set.Key additions:
config-helper:
profiler
Motivation
All environment variables must be set-able via stable config files: RFC
Note
Not all calls to getEnvironmentVariable were replaced by getResolvedEnv. In some cases, we are really looking for an environment variable that is a heuristic for a particular type of environment. Stable config is not relevant in these cases.
Plugin Checklist
Additional Notes