fix avoids a possible 'RangeError: Maximum call stack size exceeded' in maybeTime(...)

[trentm]

This avoids a possible 'RangeError: Maximum call stack size exceeded' as
described in #2929.

The case the user hit is in maybeTime(...) used in Span timer handling.
Fixed by dropping the recursion. A Timer instance's _timer is always the
same object as its parent's _timer if it has a parent, so there is no
need to walk the chain.

Refs: #2929

---

See the discussion at #2929 for why this doesn't include tests. We don't have a perfect understanding of how this is happening so don't have a good test to add. If this passes the current breakdown metrics tests ("test/metrics/breakdown.test.js"), then I think this should be good as a workaround for the user's issue.

[ghost]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-10-13T22:20:33.295+0000

• Duration: 23 min 30 sec

Test stats 🧪

Test	Results
Failed	0
Passed	269908
Skipped	0
Total	269908

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules

• run benchmark tests : Run the benchmark test only.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[astorm]

Approving, but just to say this out loud, that approval hinges on this being true

A Timer instance's _timer is always the same object as its parent's _timer if it has a parent

I only dug into this shallowly, but looking at the place a span-or-transaction's Timer object gets instantiated

https://github.com/elastic/apm-agent-nodejs/blob/main/lib/instrumentation/generic-span.js#L25

  this._timer = new Timer(opts.timer, opts.startTime)

The timer passed to Timer's constructor is an object that could be set by a user passing in options to startSpan or startTransaction, which means making assumptions about its structure is trusting that end users aren't getting too wild with what they're doing.

This isn't a documented part of the user facing API so not a blocker, but if we start getting reports of weird timings from our most power of power users we'll want to remember this change has been made.

[astorm]

Only seeing the (more accurate) parameter name change and comment change here -- no functional changes.

[trentm]

Approving, but just to say this out loud, that approval hinges on this being true

A Timer instance's _timer is always the same object as its parent's _timer if it has a parent
...

@astorm Good point, thanks. What do you think about adding a warning if a user passing in opts.timer to new Span or new Transaction? Using process.emitWarning like this 9c4e22f looks like the following on the CLI:

(node:65143) [ELASTIC_APM_SPAN_TIMER_OPTION] DeprecationWarning: specifying the `timer` option to `new Span()` was never a public API and will be removed

[astorm]

The log makes sense 👍 approving.

[trentm]

#2990 added to drop the option in a future major rev.