feat: support child spans with tail latencies#913
Conversation
googlebot
added
the
cla: yes
kjin
changed the title
kjin
force-pushed
the
child-span-tails
branch
from
52196d7 to
30d5ab8
Compare
kjin
force-pushed
the
child-span-tails
branch
from
30d5ab8 to
9f1b448
Compare
kjin
changed the title
kjin
force-pushed
the
child-span-tails
branch
from
9f1b448 to
ecc1eda
Compare
|
👋 @googleapis/node-team @kjin can someone take a look at this one? |
| // A function that replaces the ChildSpanData#endSpan prototype method on | ||
| // a child span instance. | ||
| private static overrideChildEndSpanHandler = function( | ||
| this: ChildSpanData, timestamp?: Date) { |
There was a problem hiding this comment.
Can you use an identifier other than this?
There was a problem hiding this comment.
this is a type annotation only, it'll be erased on compilation.
| if (!child.span.endTime) { | ||
| // Child hasn't ended yet. | ||
| // Override the endSpan function to make it re-publish only itself. | ||
| child.endSpan = RootSpanData.overrideChildEndSpanHandler; |
There was a problem hiding this comment.
Is there another way to do this instead of monkey patching? Could you have a test in the endSpan function? It is best to avoid monkey patching functions unless absolutely necessary. A reader will have to know that the endSpan method is part of the dynamic state rather than just the data.
There was a problem hiding this comment.
I've added a commit that changes the way this is being done, LMK what you think.
kjin
force-pushed
the
child-span-tails
branch
from
afa0642 to
ef32ae3
Compare
ofrobots
left a comment
ofrobots
left a comment
There was a problem hiding this comment.
So much better. Let a few more comments.
| /** | ||
| * Clears the buffer, returning its original contents. | ||
| */ | ||
| flush(): Trace[] { |
There was a problem hiding this comment.
The name doesn't match the implementation.
| child.shouldSelfPublish = true; | ||
| } | ||
| }); | ||
| this.children = []; |
There was a problem hiding this comment.
We need a comment on why we are setting the children array to empty. My guess is that the reason is that we don't want to keep the children array to stay alive because of the unterminated spans (since they continue to keep a reference to trace).
Alternatively, you could remove the reference from span to the trace, and remove this line and the needed comment.
There was a problem hiding this comment.
It's really more just that the this.children served its one-time purpose, so its value no longer holds any real meaning. I don't think there is significant memory impact either way, though there is the miniscule added benefit that in unforeseen circumstances where the root span is being retained in memory for whatever reason, that is has a slightly smaller footprint.
| } | ||
|
|
||
| function createDummyTrace(): Trace { | ||
| let traceIdHighWater = 0; |
There was a problem hiding this comment.
nit: complete the name to traceIdHighWatermark
kjin
force-pushed
the
child-span-tails
branch
from
aa16adb to
fab9f66
Compare
4 participants
Fixes #794