Conversation
googlebot
added
the
cla: yes
ghost
assigned
soldair
left a comment
soldair
left a comment
There was a problem hiding this comment.
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. 👍
ofrobots
left a comment
ofrobots
left a comment
There was a problem hiding this comment.
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.
kjin
changed the title
ofrobots
left a comment
ofrobots
left a comment
There was a problem hiding this comment.
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.
| 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.
| /** | ||
| * 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. |
6 participants
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).