feat: Add callback option to config to capture context when starting transactions and spans

[cjr125]

No description provided.

[cla-checker-service]

💚 CLA has been signed

[vigneshshanmugam]

Not sure whats the use-case we are solving with this. Why not use Global Labels for this functionality - https://www.elastic.co/guide/en/apm/agent/rum-js/current/agent-api.html#apm-add-labels

[cjr125]

Not sure whats the use-case we are solving with this. Why not use Global Labels for this functionality - https://www.elastic.co/guide/en/apm/agent/rum-js/current/agent-api.html#apm-add-labels

The specific use case for the callback is to capture context specific data which is different between the call stacks when firing / handling the transaction / span events from the auto-instrumentation. For example, when an application uses a combination of javascript files in which some import the RUM agent and others do not, there will be different stack traces when comparing the stack frame where the event was fired as opposed to handled. The ultimate goal would still be to assign labels, the gap being the context when the event is fired in cases where that context does not import the RUM agent library

[cjr125]

@vigneshshanmugam any update on this? do you understand the use case?

[malek-andrew]

@vigneshshanmugam Customer filed a support case today regarding this PR. Would you mind give it another review?

[david-luna]

Hi @cjr125

While I run some tests locally to understand better the intent of this new config I'm going to do a 1st review on this. Thanks for the contribution :)

[david-luna]

Hi @cjr125

I did finish the testing on this change-set. Thanks for being patient and for your contribution. Here are some questions and comments regarding this.

use case

The specific use case for the callback is to capture context specific data which is different between the call stacks when firing / handling the transaction / span events from the auto-instrumentation. For example, when an application uses a combination of javascript files in which some import the RUM agent and others do not, there will be different stack traces when comparing the stack frame where the event was fired as opposed to handled. The ultimate goal would still be to assign labels, the gap being the context when the event is fired in cases where that context does not import the RUM agent library

Is the call stack the only difference?

2 options instead of 1

The same option is used to extract context whenever a new transaction or span is created by the RUM agent. One potential problem I see is that there is no way to differentiate is the function is being called for a transaction or for a span. Also I think to leave the user to deal with this detection may be cumbersome. I propose to have a dedicated option per type (transaction or span) so we have more control on what info we add into transactions and spans and.

be defensive about user input

In your implementation the callback function is called if the configuration option is defined but it does not check that the value passed is a callable function. A bad configuration option will raise an error when the agent tries to call an expression which is not a function and it may break the user's app (it certainly breaks the instrumentation).

Also the RUM agent needs to make sure the function call does not raise an error (try/catch) and check if the returned value from the callback is of the type we expect to add it to the labels/tags

make use of the API

similar to the above section setting the return object as tags property may lead to issues when processing the data and displaying it in kibana. The public API addLabels from both transaction and span performs some checks and makes sure the tag names have valid chars in it (ref)

I think these 3 points should be addressed. I'm expecting to have bandwidth in the following days so if you prefer I can jump in the implementation :)

[cjr125]

Hi @cjr125

I did finish the testing on this change-set. Thanks for being patient and for your contribution. Here are some questions and comments regarding this.

use case

The specific use case for the callback is to capture context specific data which is different between the call stacks when firing / handling the transaction / span events from the auto-instrumentation. For example, when an application uses a combination of javascript files in which some import the RUM agent and others do not, there will be different stack traces when comparing the stack frame where the event was fired as opposed to handled. The ultimate goal would still be to assign labels, the gap being the context when the event is fired in cases where that context does not import the RUM agent library

Is the call stack the only difference?

2 options instead of 1

The same option is used to extract context whenever a new transaction or span is created by the RUM agent. One potential problem I see is that there is no way to differentiate is the function is being called for a transaction or for a span. Also I think to leave the user to deal with this detection may be cumbersome. I propose to have a dedicated option per type (transaction or span) so we have more control on what info we add into transactions and spans and.

be defensive about user input

In your implementation the callback function is called if the configuration option is defined but it does not check that the value passed is a callable function. A bad configuration option will raise an error when the agent tries to call an expression which is not a function and it may break the user's app (it certainly breaks the instrumentation).

Also the RUM agent needs to make sure the function call does not raise an error (try/catch) and check if the returned value from the callback is of the type we expect to add it to the labels/tags

make use of the API

similar to the above section setting the return object as tags property may lead to issues when processing the data and displaying it in kibana. The public API addLabels from both transaction and span performs some checks and makes sure the tag names have valid chars in it (ref)

I think these 3 points should be addressed. I'm expecting to have bandwidth in the following days so if you prefer I can jump in the implementation :)

@david-luna I believe I have addressed each of your comments. Can you please review the changes again when you have a chance?

[david-luna]

run docs-build

[david-luna]

LGTM. Thanks for your contribution :)

[david-luna]

@vigneshshanmugam do you mind to do a quick check. Thanks

[david-luna]

a couple of formatting changes to make the build pass

[david-luna]

run docs-build

[vigneshshanmugam]

I am not a big fan of this approach and exposing configurations feels unncessary. Why not adding custom properites to transaction context via

apm.SetCustomContext

You can basically call this function apm.SetCustomContext anywhere and get the context added to the transaction/error events

apm.Observe

You can rely on the events when any transaction gets started/ended and add the relevant tags/context specific information.

apm.observe('transaction:start', function (transaction) {
    stack = "" // error stack
    transaction.addLabels({ stack })
})

Let me know what you think.

[cjr125]

@vigneshshanmugam the callback approach makes an important difference when context needs to be captured from an auto-instrumented transaction which starts in a file which is loaded by the application but does not import the RUM agent library. Using the built-in setCustomContext approach will not show the URL of the file where such a transaction started in a predictable order in the stack trace if we are to capture it as you have described. The callback approach will guarantee it will occur at the top of the stack trace. The use case we are interested in is quickly identifying the developer responsible for maintaining the code where a problematic transaction starts, so the callback approach is the only way to guarantee that we can immediately filter out the other files (when the original file does not import the library). Please let me know if you have other questions.