`apm.startTransaction` crash if agent not yet started

[trentm]

const apm = require('elastic-apm-node');

const transaction = apm.startTransaction('test', 'any');

setTimeout(() => {
  transaction.end();
}, 1000);

After the changes in #2410 to not configure (and set agent._conf) until agent.start(...) is called, the above snippet crashes:

.../kibana/node_modules/traceparent/index.js:127
      : start(Math.random() <= conf.transactionSampleRate)
                                    ^

TypeError: Cannot read properties of null (reading 'transactionSampleRate')
    at Function.startOrResume (.../kibana/node_modules/traceparent/index.js:127:37)
    at Function.startOrResume (.../kibana/node_modules/elastic-apm-node/lib/tracecontext/index.js:15:37)
    at Transaction.GenericSpan work/.../kibana/node_modules/elastic-apm-node/lib/instrumentation/generic-span.js:18:32)
    at new Transaction (.../kibana/node_modules/elastic-apm-node/lib/instrumentation/transaction.js:20:15)
    at Instrumentation.startTransaction (.../kibana/node_modules/elastic-apm-node/lib/instrumentation/index.js:371:17)
    at Agent.startTransaction (.../kibana/node_modules/elastic-apm-node/lib/agent.js:145:49)

Before #2410 the apm.startTransaction was returning a transaction based on incomplete configuration. I think the right fix here is that apm.startTransaction() should return null in this case (perhaps with a log.warning that a transaction cannot be created before the agent is started). Thankfully the interface (in index.d.ts) is already that apm.startTransaction() can return null.

Part of this fix should update the changelog entry for #2410 to notify that here is a case (the first one) where apm.startTransaction() might actually return null. That could affect user code. /cc @mshustov

[astorm]

We did flag this as a hypothetical crasher during review: https://github.com/elastic/apm-agent-nodejs/pull/2410/files#r742251729

Given that we now have evidence that this is no longer hypothetical (we introduced a real crash) what do you think about restoring the empty _conf object between module loading and agent start? It may be unintentional but the agent has a unstated (but deep rooted) assumption that there's always a configuration object there.

Put another way, while the _conf object isn't an explicit public API, our change resulted in methods that are a part of our explicit API changing their behavior, which included crashing the agent and crashing the end user's application/service.

My immediate (maybe naive?) view is that we should restore the empty configuration object to prevent these crashes, and any further refactoring to change this implicit dependency between agent and configuration object should be part of a major version bump.

[trentm]

I would like to avoid re-instating a subtly-wrong _conf object if we can.

The correct behaviour (again, IMHO) of the agent is for apm.startTransaction(...) to return null if the agent isn't yet started and the existing API definition in index.d.ts allows that:

    startTransaction(
      name?: string | null,
      options?: TransactionOptions
    ): Transaction | null;
    // ...

From the point of view of a TypeScript user of the APM agent, all is likely well and good (assuming the TypeScript user doesn't ignore the | null in type checking). Quoting Mikhail -- my sample TypeScript user -- from recent DM'ing:

[Trent]

Yes, it can in general return null, so my hope is that Kibana usage will be fine if that happens.
I'm asking here, because I think this is a new semantic case where apm.startTransaction() would return null -- after I fix this crash bug.

[Mikhail]
Kibana should not make assumptions about the APM internal logic, so if it starts failing, we need to fix it on the Kibana side

For a JS user, it is less clear. All the getting started docs show usage starting the agent before any other JS code runs. However, the apm.startTransaction() API docs don't explicitly say that null can be returned in certain cases as the apm.startSpan() docs do.

So the question isn't so much whether there should always be an apm._conf object -- the crash in apm.startTransaction() without apm._conf is a bug to fix in the agent. The question is whether changing apm.startTransaction() to have its first actual case of being able to return null is a "breaking change" for users. This only applies to uses that have manual instrumentations. I agree this is a breaking change, we should push to a major ver bump. I'll open a separate issue for that.

Options to fix this issue:

1. Revert the zero-conf change and go back to doing two agent._config() calls. Ew, not gonna go there.
2. Set agent._conf = {} (empty config) in the Agent constructor. This would fix this one issue, but only because of JS implicit type conversions.
3. Set agent._conf = /* a copy of config.DEFAULTS */ (default config) to start in a known state that we hope is stable/reasonable. This should fix most issues. I say "most" because DEFAULTS doesn't actually have values for all config vars.
4. Change apm.startTransaction() to return a no-op Transaction object when not started.

I think Option 4 is the "cleanest" and most explicit fix, but I'm not sure it is worth the extra effort. I propose to go with option 3. I'll also look through all the other Agent methods that just call into this._instrumentation.$method() to see if there are similar faulty assumptions that the Agent is started. I was wrong in my zero-config PR description when I said:

... All Agent methods have been updated as necessary to guard
against usage before the agent is started. ...