Save built-in JSON object when agent start to allow instrumenting JSON.stringify

[statox]

Is your feature request related to a problem? Please describe.

My team is experimenting with APM and one of the important things we want to instrument is our calls to JSON.stringify. My first try was to override the JSON.stringify() function like this:

const defaultStringify = JSON.stringify;
JSON.stringify = (...args: any): any => {
    const span = startSpan('stringify', 'JSON');
    try {
        const result = defaultStringify.apply(null, args as any);
        span.end('success');
        return result;
    } catch (error) {
        span.end('failure');
        throw error;
    }
};

However when starting the application I get a max call stack exceeded error. My hypothesis is that it comes from the fact that the agent uses JSON.stringify() in the getContextFromRequest() function.

Describe the solution you'd like

I would like to open a PR where I would save the functions of the JSON object when the agent starts so that we can override the object and the agent would keep using the default function.

Describe alternatives you've considered

For now I'm working around this issue by creating a dedicated module which exposes an instrumented function that we use instead of the built-in JSON.stringify

export const stringify = (value: any, replacer?: any, space?: number): string => {
    const span = apm.startSpan('JSON.stringify', 'JSON');
    try {
        const result = JSON.stringify(value, replacer, space);
        span.end('success');
        return result;
    } catch (error) {
        span.end('failure');
        throw error;
    }
};

And I'm coupling that with a custom eslint rule to make sure everyone in the team uses this function and not the default one.

Additional context

I am willing to create the PR on my side but I first wanted to check with you if that would be an acceptable solution.

[trentm]

Hi. Interesting experiment!

My hypothesis is that it comes from the fact that the agent uses JSON.stringify() in the getContextFromRequest() function. ... I am willing to create the PR on my side but I first wanted to check with you if that would be an acceptable solution.

If the change is fairly localized, then yes it would be acceptable. I don't think we can sign up to promise this never gets broken in the future. We could add a test case for the basic usage, but if support the case got baroque, we would consider dropping it.

I worry a little bit about an infinite loop with span creation in other parts of the agent handling. For example in the serialization of APM data done in the client module the agent uses to talk to APM server:

index.js
1253:        setVerUnknownAndNotify(`could not determine APM Server version from information endpoint body: ${JSON.stringify(serverInfo)}`)
1591:  return JSON.parse(JSON.stringify(obj))

lib/ndjson.js
12:    return JSON.stringify(obj)

And then other possible transitive usage of JSON.string, for example, if we moved to using https://github.com/fastify/fast-json-stringify/ for our JSON serialization... that module itself uses JSON.stringify internally.

Definitely tag me if you get this going. I might be able to help battle test it a bit.

export const stringify = (value: any, replacer?: any, space?: number): string => {
    const span = apm.startSpan('JSON.stringify', 'JSON');
    try {
        const result = JSON.stringify(value, replacer, space);
        span.end('success');
        return result;
    } catch (error) {
        span.end('failure');
        throw error;
    }
};

Some small code review on that:

1. apm.startSpan() might return null in several cases (if there is no current transaction, if transactionMaxSpans is hit), so subsequent span.end() calls should guard for that.
2. The argument to span.end() is an endTime. You probably want if (span) { span.setOutcome('success' | 'failure'); span.end() }.