-
Notifications
You must be signed in to change notification settings - Fork 98
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: emit an error log on potential memory leak scenario #870
Conversation
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.
lgtm.
no test for it but the warning is pretty soft, and doesn't seem to reference properties that are likely to be undefined. so un likely to crash. 👍
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.
Is there a value in a hard limit also?
// As in the previous case, a root span with a large number of child | ||
// spans suggests a memory leak stemming from context confusion. This | ||
// is likely due to userspace task queues or Promise implementations. | ||
this.logger!.warn(`TraceApi#createChildSpan: [${ |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
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.
Code is looking fine. I left some comments about the UX.
options.name}] will cause the trace with root span [${ | ||
rootSpan.span.name}] to contain more than ${ | ||
this.config! | ||
.spansPerTraceSoftLimit} spans. This is likely a memory leak.`); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
logLevel: 1, | ||
enabled: true, | ||
enhancedDatabaseReporting: false, | ||
rootSpanNameOverride: (name: string) => name, | ||
clsMechanism: 'auto' as CLSMechanism, | ||
spansPerTraceSoftLimit: 200, | ||
spansPerTraceHardLimit: 1000, |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/config.ts
Outdated
/** | ||
* The maximum number of local spans per trace to allow in total. Creating | ||
* more spans will cause the agent to log an error. (This limit does not apply | ||
* when using RootSpan to create child spans.) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Guys the promotion of these logs from "warn" to "error" is causing problems. Specifically line 241 in trace-api.ts. |
@mikelueck this was a mistake. The issue is fixed in #882 and released as the latest version of the module. |
Given that the scenario #838 is likely caused by context confusion on long-running requests, this warning should help us determine whether a memory leak is due to an intrinsic design detail to the agent, or context confusion. Emitting this warning is a clear sign of the latter (the remedy would be to identify the userspace queueing mechanism that breaks context, and patch it/request that it support async resources).