-
Notifications
You must be signed in to change notification settings - Fork 39
feat: unified stack trace configuration from agent, in-code and env vars #2235
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
feat: unified stack trace configuration from agent, in-code and env vars #2235
Conversation
5965b8c to
b30a2d8
Compare
7213e70 to
efa9059
Compare
packages/collector/test/tracing/protocols/http/proxy/expressProxyControls.js
Outdated
Show resolved
Hide resolved
b30a2d8 to
04b7085
Compare
Co-authored-by: kirrg001 <katharina.irrgang@ibm.com>
Co-authored-by: kirrg001 <katharina.irrgang@ibm.com>
Co-authored-by: kirrg001 <katharina.irrgang@ibm.com>
packages/core/src/config/index.js
Outdated
| logger.warn( | ||
| `Normalized the provided value of config.tracing.stackTraceLength (${numericalLength}) to ${normalized}.` | ||
| ); | ||
| if (typeof tracing.stackTraceLength !== 'number') { |
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.
Same here. Why do we have to validate here again that stackTraceLength is a number?
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.
This is a similar case of #2235 (comment)
We can't just check the existence because of the possible 0 case.
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.
This extra check is only needed because we dont set the default value after the failed validation?
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.
Ah, in a recent change I updated the normaliser to return null when no valid data is parsed.
This should have allowed us to move away from type checks and instead use a simple != null check.
However, there are some edge cases to consider.
For example, when a legacy configuration is set with an invalid value:
const config = coreConfig.normalize({ tracing: { stackTraceLength: {} } });
expect(config.tracing.stackTraceLength).to.equal(10);
In this case, validation fails and the normaliser is not called.
Because of that, the value remains defined (but invalid), and the != null check passes.
As a result, we never fall back to the default value, which is incorrect.
To handle edge cases like {}, [], or [18], we still need to perform a type check.
This is specific to stackTraceLength only because legacy support writes directly to the same field we later normalise.
This issue does not apply to stackTrace mode, since that value is derived from tracing.global.stackTrace, and when the user provides an invalid value, we correctly fall back to the default.
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.
Because of that, the value remains defined (but invalid), and the != null check passes.
If validation fails, we have to set defaults!
Co-authored-by: kirrg001 <katharina.irrgang@ibm.com>
packages/core/src/config/index.js
Outdated
| const isLegacyLengthDefined = tracing.stackTraceLength !== undefined; | ||
|
|
||
| if (isGlobalLengthDefined || isLegacyLengthDefined) { | ||
| if (isLegacyLengthDefined && !isGlobalLengthDefined) { |
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.
If you have defined isLegacyLengthDefined -> we show the warning. It doesn't matter if customer already uses the new one.
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.
Okay, updated 👍
|
|
||
| /** | ||
| * Normalizes stack trace length configuration based on precedence. | ||
| * Precedence: global config > config > env var > default |
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.
The features ignore-endpoints and disable used the community config order. This slipped through my review. We should have continued with our Node.js config order. Now we are in a mixed state, which is not right.
As we do not want to change ignore-endpoints and disable features, we decided change the order in our tracer asap (Q1) and release v6. We have that on our board anyway for a while now (https://jsw.ibm.com/browse/INSTA-817).
That means (I am sorry) this PR also has to follow the new community order.
cc @aryamohanan cc @abhilash-sivan
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.
Agreed. Np., Will do
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.
100% make sense.
kirrg001
left a comment
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.
Pre-approving. Just the new order is missing.
aryamohanan
left a comment
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.
Great work 👏
|



refs https://jsw.ibm.com/browse/INSTA-63750
Current PR
agent → tracing.global.stackTraceLength → tracing.stackTraceLength → env var → default
Next PR
Extend the stackTraceLength limit to the getStackTrace function and add the required tests.done in this pr