[sdk-node] a possible design for *extension* support

[trentm]

This issue proposes a possible design for supporting OTel Node.js extensions,
somewhat similar to OTel Java's https://opentelemetry.io/docs/zero-code/java/agent/extensions/ which allow something like:

java -javaagent:path/to/opentelemetry-javaagent.jar \
     -Dotel.javaagent.extensions=/path/to/extension1.jar,/path/to/extension2.jar \
     -jar myapp.jar

Motivation:

• Having extensions support can solve the issue of "I have this one extra instrumentation I want to use with auto-instrumentations-node."
• With the work going on in the sdk-node package (declarative config, new startNodeSDK() to replace class NodeSDK, ConfigProvider API, PluginComponentProvider for extending declarative config), it has helped me feel out how all these pieces can fit together.

Extension support in OTel Node.js

OTEL_NODE_EXTENSIONS=@trentm/my-otel-stuff,@opentelemetry/instrumentation-http \
  node --import @opentelemetry/sdk-node/start app.js

(This is the "configure the SDK from env vars" code path. No declarative config, yet.)

@opentelemetry/sdk-node/start will:

• read OTEL_NODE_EXTENSIONS
• await import() each of the given entry points
• call their loadOpenTelemetryExtension() which returns interface OpenTelemetryExtensionV1
• this interface has an optional instrumentations array of instrumentation info (name and class).
• These instrumentations are added to the default set of instrumentations.
  For sdk-node the default set starts empty,
  for auto-instrumentations-node that is a large set of included instrumentations.
• Then it calls startNodeSdkFromEnv({instrumentations}).
• (This entrypoint intentionally doesn't work with --require because it uses top-level await.)

Support for OTEL_NODE_DISABLED_INSTRUMENTATIONS / OTEL_NODE_ENABLED_INSTRUMENTATIONS could move from auto-instrumentations-node to sdk-node.
sdk-node would export a selectInstrumentationsFromEnv() that handles these envvars to filter a default set.
sdk-node's default set is empty, but adds new ones from OTEL_NODE_EXTENSIONS.

(Aside: We could revive @opentelemetry/node to shorten this to node --import @opentelemetry/node app.js. :)

Extension support in declarative config

Using declarative config:

OTEL_CONFIG_FILE=./otel-sdk-config.yaml \
  node --import @opentelemetry/sdk-node/start app.js

To continue with the idea of "no outside envvars when using OTEL_CONFIG_FILE", how about this "otel-sdk-config.yaml":

# ... the usual tracer_provider et al

distribution:
  nodejs:   # 1.
    extensions: # 2.
      # 3.
      - @opentelemetry/instrumentation-http
      - @trentm/my-otel-stuff
    extensions_list: ${OTEL_NODE_EXTENSIONS}  # Similar idea to attributes_list, headers_list pattern.
    instrumentations: # 4.
      # included:
      #   - ...
      excluded:
        - bar  # Say we don't want the "bar" instrumentation provided from the "@trentm/my-otel-stuff" extension.

instrumentation/development: # 5.
  js:
    pino:
      disable_log_sending: true

Notes:

1. This proposes claiming a well-known "nodejs" distribution name.
   For comparison, the OTel Java javaagent supports a "javaagent" name here, I believe.
2. This requires the startNodeSDK() function to be async to support extensions.
   That's a bit of a pain.
   It means that users (and our test files) wanting to use startNodeSDK() need top-level await.
   I propose having separate function startNodeSdkFromConfigSync() and async function startNodeSdkFromConfig().
   The latter supports extensions. The "Sync" version throws if the loaded config has extensions.
   Long term, the impact here is that there is a bias away from using startNodeSdk() at the top of one's app.js,
   in favour of using node --import ./telemetry.mjs app.js.
   I think that isn't a bad thing.
   It helps with the "imported package before instrumenting it" problem.
   (Some day https://github.com/tc39/proposal-import-sync could make this sync again.)
   (FWIW, I have an earlier sketch of a plan that would not add distribution.nodejs.extensions to the config file; instead it uses the OTEL_NODE_EXTENSIONS envvar, has top-level code in @opentelemtry/sdk-node/start load extensions, and then pass extension data to startNodeSDK() which now no longer needs to be async.)
3. Similar to the previous section, each extension is await import(...) loaded,
   and its loadOpenTelemetryExtension() is called, which returns an interface OpenTelemetryExtensionV1.
   This interface includes an optional componentProviders array,
   which is passed into startNodeSdkFromConfig({componentProviders, instrumentations}).
   See Marc's PoC for PluginComponentProvider support: refactor(sdk-node): model built-in exporter resolution on top of PluginComponentProvider spec #6730
4. This uses the included/excluded pattern in declarative config to select from available instrumentations.
   I.e. this replaces OTEL_NODE_{DIS,EN}ABLED_INSTRUMENTATIONS.
5. instrumentation/development.js.* allows for configuring instrumentations.
   This isn't new.
   mwear has a PR to add support for this: feat(sdk-node,instrumentation,instrumentation-http,api-config,configuration): add declarative config support for instrumentation/development #6868

Auto-instrumentations-node and extensions

Currently auto-instrs-node provides this convenience to get a bunch of instrs and cloud-y resource detectors:

node --import @opentelemetry/auto-instrumentations-node/register app.js

It could be updated (using new utilities from sdk-node) to support OTEL_NODE_EXTENSIONS.
This could mostly solve the issue where a 3rd party instrumentation is at a disadvantage if it doesn't get included in the opentelemetry-js-contrib.git repo and the auto-instrs-node package:

OTEL_NODE_EXTENSIONS=@ariseai/openinference-instrumentation-langchain \
  node --import @opentelemetry/auto-instrumentations-node/register app.js

Or auto-instrs-node could itself support being used as extension, then one could use:

# ... the usual tracer_provider et al

distribution:
  nodejs:
    extensions:
      - @opentelemetry/auto-instrumentations-node
    ## Optionally select a subset of instrs from auto-instrumentations-node.
    # instrumentations:
    #   included:
    #     - pino
    #     - http
    #     - ioredis
    #     - pg

Security

Q: Is it a security concern to be able to list dynamically-loaded extensions in the config file?

Previously if the config file was hacked, then what was the exposure?
Less than "arbitrary code execution".

Mind you, this can only execute code from an already installed package.
That might still allow shenanigans, but perhaps not arbitrary code execution unless have a naughty package installed.

Any mitigations worthwhile here?
E.g. could we limit the extensions value to something?
Just a absolute package entry point? No local paths?
No '..' path segments?

Similar to how "attacker controls env vars" is not in our (draft) threat model (#6676),
I think it likely that "attacker controls declarative config file" is also not in the threat model.