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: support child spans with tail latencies #913
Conversation
52196d7
to
30d5ab8
Compare
30d5ab8
to
9f1b448
Compare
9f1b448
to
ecc1eda
Compare
👋 @googleapis/node-team @kjin can someone take a look at this one? |
src/span-data.ts
Outdated
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use an identifier other than this
?
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 type annotation only, it'll be erased on compilation.
src/span-data.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a commit that changes the way this is being done, LMK what you think.
afa0642
to
ef32ae3
Compare
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.
So much better. Let a few more comments.
src/trace-writer.ts
Outdated
/** | ||
* Clears the buffer, returning its original contents. | ||
*/ | ||
flush(): Trace[] { |
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 name doesn't match the implementation.
child.shouldSelfPublish = true; | ||
} | ||
}); | ||
this.children = []; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
test/test-trace-writer.ts
Outdated
@@ -63,18 +63,20 @@ function mockNoMetadata() { | |||
}; | |||
} | |||
|
|||
function createDummyTrace(): Trace { | |||
let traceIdHighWater = 0; |
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.
nit: complete the name to traceIdHighWatermark
aa16adb
to
fab9f66
Compare
Fixes #794