MongoDB first working version

[tom-pytel]

still needs test

[tom-pytel]

%( ?!?

[tom-pytel]

I have now officially spent more time trying to get this garbage test to run that I did on the actual plugin 😠

EDIT: Never mind, the garbage test fails because there is an actual bug with the plugin...

[tom-pytel]

Ok, that bug in the plugin is not so much of a bug but a design issue. It is caused by MongoDB calling back into itself for internal operation using the same user-level API that starts an exit span. This causes a new exit span but inherits from the old and screws up some internal counts. I can easily fix it for this case, but the more general question is as follows:

The original behavior in this agent was (and still) is that if an Entry / Exit span gets created on top of another of the same kind then that span is inherited instead of a new span being created as a child. This is great for something like express sitting on top of http, but there is a case where it becomes a problem. Like for example when I consider the user callback processing an incoming stream as part of the span, and that code creates another unrelated outgoing span. Like a message or database access during the processing of an incoming HTTP stream. Obviously that new span should not inherit from the old one but should rather be a child of it, but that gives us Exit span as a child of an Exit span.

So what is the desired behavior here? Should I exclude user processing (like the callback or .then()) of an incoming stream from the span and just end the span before the user part? This would give incorrect end times for long streams. Or allow at least Exit spans as children of Exit spans and allow inheritance only in specifically recognized cases like for example express on top of http? Or the other option of making the sub-Exit a sibling of the parent Exit?

[tom-pytel]

Ok, finally, good to merge...

[kezhenxu94]

Should I exclude user processing (like the callback or .then()) of an incoming stream from the span and just end the span before the user part?

For such kind of operations (user local processing, callback or .then()), we can use Local Span (given proper operation name) to represent that local operations and it's acceptable to incur another Exit span in that local span.

Ok, that bug in the plugin is not so much of a bug but a design issue. It is caused by MongoDB calling back into itself for internal operation using the same user-level API that starts an exit span.

For this particular case, I'm 👍 to your solution in this PR

[tom-pytel]

For such kind of operations (user local processing, callback or .then()), we can use Local Span (given proper operation name) to represent that local operations and it's acceptable to incur another Exit span in that local span.

The thing is we do not know ahead of time what the user will do in the callback, so do we always pre-emptively create a local span on all Exit spans which call user code? This is essentially duplicating the Exit span everywhere and most of those Local spans will be unnecessary.

[wu-sheng]

so do we always pre-emptively create a local span on all Exit spans which call user code?

I don't think we have to. The reason other agents did this, is for propagating context. Their tracing context have to be injected in the callback thread to avoid trace broken. If the nodejs agent doesn't have to deal with this case, feel free to not create this local span.

[tom-pytel]

I don't think we have to. The reason other agents did this, is for propagating context. Their tracing context have to be injected in the callback thread to avoid trace broken. If the nodejs agent doesn't have to deal with this case, feel free to not create this local span.

That doesn't solve the original problem I described.

[wu-sheng]

I don't think we have to. The reason other agents did this, is for propagating context. Their tracing context have to be injected in the callback thread to avoid trace broken. If the nodejs agent doesn't have to deal with this case, feel free to not create this local span.

That doesn't solve the original problem I described.

I am answering the question about the exit + local(callback) + exit.
About your design question, in general design perspective, if one RPC framework on the top of another, then only the first exit should be created, in your question, express span only.
One exit span means one RPC interaction between client and server, if the 2nd internal call is causing the real RPC, then it is correct to create an exit span as the child of the first exit span. The basic principle is, the exit span should be accurate mapping to the RPC behavior. Same as the entry span.

[tom-pytel]

I am answering the question about the exit + local(callback) + exit.
About your design question, in general design perspective, if one RPC framework on the top of another, then only the first exit should be created, in your question, express span only.
One exit span means one RPC interaction between client and server, if the 2nd internal call is causing the real RPC, then it is correct to create an exit span as the child of the first exit span. The basic principle is, the exit span should be accurate mapping to the RPC behavior. Same as the entry span.

Ok, so by that I understand that if inside an open HTTP Exit span the user code queries a database which needs its own Exit span then that span will be a CHILD of the HTTP Exit span, and the current architecture has no problem with Exit spans as children of other Exit spans.

[wu-sheng]

I am answering the question about the exit + local(callback) + exit.
About your design question, in general design perspective, if one RPC framework on the top of another, then only the first exit should be created, in your question, express span only.
One exit span means one RPC interaction between client and server, if the 2nd internal call is causing the real RPC, then it is correct to create an exit span as the child of the first exit span. The basic principle is, the exit span should be accurate mapping to the RPC behavior. Same as the entry span.

Ok, so by that I understand that if inside an open HTTP Exit span the user code queries a database which needs its own Exit span then that span will be a CHILD of the HTTP Exit span, and the current architecture has no problem with Exit spans as children of other Exit spans.

Yes, correct. The OAP would consider all exit spans accurate. If things like express on the top of http, it is agent's responsibility. Java agent did this kind of merge in a thread when facing nested exit spans(no local span in the middle)