fix(core): ensured env vars take precedence over in-code config#2409
fix(core): ensured env vars take precedence over in-code config#2409aryamohanan wants to merge 14 commits intofix-config-precedencefrom
Conversation
aryamohanan
changed the title
aryamohanan
force-pushed
the
fix-config-order
branch
from
2fc5f73 to
15e7bc5
Compare
aryamohanan
added
feature-branch
aryamohanan
changed the title
|
I'd suggest to rename the commit to:
And add a TODO for the other packages e.g. collector needs an update too. See normalizeConfig. |
aryamohanan
changed the title
| return; | ||
| } | ||
|
|
||
| if (process.env['INSTANA_TRACING_DISABLE_EOL_EVENTS'] === 'false') { |
There was a problem hiding this comment.
This config conditions pattern repeat everywhere in the config file. I am sure we can write a helper FN for it. I'd suggest to add a TODO as lower prio.
| * @param {InstanaConfig} config | ||
| */ | ||
| function normalizeIgnoreEndpoints(config) { | ||
| if (!config.tracing.ignoreEndpoints) { |
There was a problem hiding this comment.
This can be removed IMO - not part of this PR.
It would be good also to normalize these checks as a TODO for later.
Usually we do
if (config.tracing.ignoreEndpointsDisableSuppression === true) {
if (typeof ignoreEndpointsConfig !== 'object' || Array.isArray(ignoreEndpointsConfig)) {
To see if the value is set and right.
| logger.warn( | ||
| `The value of ${envVarKey} ("${envVarVal}") is not numerical or cannot be parsed to a numerical value. Assuming the default value ${defaultValue}.` | ||
| ); | ||
| return defaultValue; |
There was a problem hiding this comment.
If env is set and invalid, should we not check if in-code is set? 🤔
And if in-code is not set, we fallback to default.
There was a problem hiding this comment.
Hmm, that’s an interesting thought. We might need a validator for all configurations. Not part of this PR maybe; we can discuss it.
However, repeatedly validating – both at the environment level once and, if it fails, then in-code – feels like overkill.
There was a problem hiding this comment.
We might need a validator for all configurations
Yeah validation of all incoming values would be the best before checking its precense.
| ); | ||
| config.serviceName = process.env['INSTANA_SERVICE_NAME']; | ||
| } else if (config.serviceName != null && typeof config.serviceName === 'string') { | ||
| logger.debug(`Service name has been configured via config: ${config.serviceName}`); |
There was a problem hiding this comment.
We need a refactoring card.
We merge the user config in line 164
if (userConfig !== null) {
targetConfig = Object.assign({}, userConfig);
}
And in the normalize functions we do nothing because we already know that
function normalizeServiceName(config) {
is the user config. This is really hard to read.
And then we just do a debug log here.
There is different refactoring approaches to this.
|
3 participants
This is the first PR addressing configuration precedence.
The goal of this change is to ensure the correct order between environment variables and in-code configuration:
env > in-code
Currently, the precedence between these two sources is mixed. This PR corrects that behavior.
Added missing logging in the configuration file when a config is set.
Scope of This PR
This PR only fixes the precedence between:
Agent configuration precedence will be addressed in separate follow-up PRs.
Future Work
Additional PRs will follow to correct the agent configuration order.
Migration Plan
This change will be released in the next major version (v6), planned for Q2 2026.
ref https://jsw.ibm.com/browse/INSTA-80965
Epic : https://jsw.ibm.com/browse/INSTA-817