fix(configuration)!: backtrack on removing "| null"s from generated types

[trentm]

Closes: #6576

tl;dr

This backtracks on the configuration package design of attempting to avoid any null values in a parsed JS config. This PR changes configuration parsing to leave the nulls in and move responsibility for handling null values to the consumers of the parsed config object. This solves the general issue partially mentioned by #6576 and results in the TypeScript types more closely matching the Configuration JSON Schema shape.

Background

The OpenTelemetry configuration JSON schema intentionally adds an optional "null" type to many fields for two purposes (https://github.com/open-telemetry/opentelemetry-configuration/blob/main/CONTRIBUTING.md#required-and-null-properties):

1. Allowing envvar substitution, so that a config is valid if a particular envvar is not set, e.g. endpoint below.
2. When modeling objects that do not require any properties, e.g. OtlpHttpExporter below.

For example, let's look at this part of the JSON schema:

    "SpanExporter": {
      "type": "object",
      "properties": {
        "otlp_http": {
          "$ref": "#/$defs/OtlpHttpExporter",
          "description": "Configure exporter to be OTLP with HTTP transport.\nIf omitted, ignore.\n"
        },
    // ...
    "OtlpHttpExporter": {
      "type": [ "object", "null" ],
      "additionalProperties": false,
      "properties": {
        "endpoint": {
          "type": [ "string", "null" ],
          "description": "Configure endpoint, including the signal specific path.\nIf omitted or null, the http://localhost:4318/v1/{signal} (where signal is 'traces', 'logs', or 'metrics') is used.\n"
        },
    // ...

Without manipulation, json-schema-to-typescript translates this to these TypeScript types:

export interface SpanExporter {
  otlp_http?: OtlpHttpExporter;
  // ...
}
export type OtlpHttpExporter = {
  /**
   * Configure endpoint, including the signal specific path.
   * If omitted or null, the http://localhost:4318/v1/{signal} (where signal is 'traces', 'logs', or 'metrics') is used.
   */
  endpoint?: string | null;
  // ...
} | null;

Going back to the two purposes of null. The first is for envvar substitution, so that this YAML works:

tracer_provider:
  processors:
    - batch:
        exporter:
          otlp_http:
            endpoint: ${MY_OTLP_ENDPOINT}

If MY_OTLP_ENDPOINT is not set, then parsing this YAML and doing envvar substitution results in this JS object, with endpoint: null.

{
  tracer_provider: {
    processors: [
      { batch: { exporter: { otlp_http: { endpoint: null } } } }
    ]
  }
}

The second purpose is for objects that do not require any properties, either because they don't have any or because all properties have default values. This allows a YAML config like this:

tracer_provider:
  processors:
    - batch:
        exporter:
          otlp_http:   # means use `otlp_http` with its default values

which yields this parsed config:

{
  tracer_provider: {
    processors: [
      { batch: { exporter: { otlp_http: null } } }
    ]
  }
}

How to handle those null values?

There are two options for dealing with these null values:

1. Have any code that uses the parsed config know to handle null values as described in the "description" field for each type. E.g. the endpoint property above has a description with "If omitted or null, use ...".
2. Get rid of the | nulls so that SDK code using the parsed config can do truthy checks, if (exporter.otlp_http) { ... }, and pass endpoint (now of type string | undefined) directly to an SDK component constructor, new OtlpHttpTraceExporter({ endpoint: exporter.otlp_http.endpoint }).

Currently the configuration package attempts option 2, because it seems nicer for SDK code using the resulting config object.

However there are problems.

The problems

When the | nulls have been removed from the TypeScript types, the parsed config file needs to be post-processed to not have those null values. This involves:

1. Converting null values to {} for objects that don't require any properties.

   Before: { batch: { exporter: { otlp_http: null } } }
   After:  { batch: { exporter: { otlp_http: {}   } } }
   

   so that if (exporter.otlp_http) { ... } works.
   This should only be done for some nulls.

2. Then remove the remaining properties that have a null value.

   Before: { batch: { exporter: { otlp_http: { endpoint: null } } } }
   After:  { batch: { exporter: { otlp_http: {}                 } } }
   

A problem is knowing which properties should have null converted to {} and which should be removed. Currently the code, normalizeYamlNulls(), has a hardcoded allowlist of well-known property names and a heuristic for arrays.

First, this is brittle. For example, here is a current case where that heuristic gets it wrong:

% cat empty-attr.yml
file_format: '1.0'
resource:
  attributes:
    - name: my.attr
      value: ${NO_SUCH_ENV_VAR_DEFINED}

% ./scripts/parse-config.mjs empty-attr.yml
.../experimental/packages/configuration/build/src/FileConfigFactory.js:48
        throw new Error(`Invalid OpenTelemetry config file: ${detail.trim()}`);
              ^

Error: Invalid OpenTelemetry config file: /resource/attributes/0/value must be string
    at parseConfigFile (/Users/trentm/semconv/opentelemetry-js3/experimental/packages/configuration/build/src/FileConfigFactory.js:48:15)

The crash here is because that normalizeYamlNulls() is converting this raw config object:

{
  file_format: '1.0',
  resource: { attributes: [ { name: 'my.attr', value: null } ] }
}

to this:

{
  file_format: '1.0',
  resource: { attributes: [ { name: 'my.attr', value: {} } ] }
}

And then schema validation fails because resource attributes may not be an object.

Second, the hardcoded allowlist cannot handle custom values (like custom detectors, custom processors, etc.). See #6576 that describes the issue for custom propagators.

One considered fix

One option for the brittle case shown above would be to make the normalizeYamlNulls() and stripNulls() heuristics smarter -- by adding more hardcoded allowlisting of known names in the current schema. That could work, but is an ongoing maintenance burden: as the declarative config schema evolves the @opentelemetry/configuration package might have to update its post-processing. The fix would remain brittle.

As well, this fix option cannot handle custom YAML sections of the schema: custom processors, detectors, etc.; anything under instrumentation/development.{language}. So, the result for consumers of the parsed config would always be two flavours of parsed config:

1. if it is a well-known section of the schema, then there (mostly) won't be any null values,
2. but if it is a custom section of the schame, then you need to handle null values

Proposed fix

The fix I'm proposing is to backtrack on the design and have configuration parsing not remove nulls. It then becomes the responsibility of consumers of the parsed config to handled nulls.

(I'm moving the configuration package design away from having smarts on config parse, and moving those smarts to the create step.)

Currently "consumers of the parsed config" is just the sdk-node package. However, eventually that will also include: (a) instrumentations using ConfigProvider#getInstrumentationConfig() and (b) any PluginComponentProvider, once these things are implemented in OTel JS.

Impact of this change

• Using the parsed config JS object needs to know about nulls.

  1. For properties like exporter.otlp_http.endpoint, that means handling string | null | undefined. It turns out this isn't so bad, because all optional properties have a default value, so nullish-coalescing works nicely:

     return new OTLPProtoTraceExporter({
       url: exporter.otlp_http?.endpoint ?? 'http://localhost:4318/v1/traces',
                                     //  ^^--- use `?? defaultValue`
       // ...

  2. For objects that can be null, like exporter.otlp_http, that means one must not use truthiness. This does require some more care by SDK developers.

     if (exporter.otlp_http) {                  // BAD!
       // ... create exporter
     }
     
     if (exporter.otlp_http !== undefined) {    // GOOD!
       // ... create exporter
     }

One the plus side:

• Config parsing (and validation) is much simpler and clearer. The parsed config JS object is close(r) to the YAML structure.
• The generated TypeScript types are closer to what the Configuration JSON Schema actually describes. We aren't creating an abstraction layer that is subtley different.
• There won't be an issue with "two flavours of parsed config", described above.

[codecov]

Codecov Report

❌ Patch coverage is 98.27586% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.94%. Comparing base (c990aff) to head (91b99de).

Files with missing lines	Patch %	Lines
...ental/packages/opentelemetry-sdk-node/src/utils.ts	98.21%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6679      +/-   ##
==========================================
+ Coverage   94.85%   94.94%   +0.09%     
==========================================
  Files         377      377              
  Lines       12751    12730      -21     
  Branches     2887     2893       +6     
==========================================
- Hits        12095    12087       -8     
+ Misses        656      643      -13     

Files with missing lines	Coverage Δ
.../packages/configuration/scripts/generate-config.js	0.00% <ø> (ø)
...al/packages/configuration/src/FileConfigFactory.ts	98.42% <ø> (-0.31%)	⬇️
...ental/packages/opentelemetry-sdk-node/src/start.ts	100.00% <100.00%> (ø)
...ental/packages/opentelemetry-sdk-node/src/utils.ts	97.12% <98.21%> (-0.07%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[maryliag]

same as above

https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/configuration/src/FileConfigFactory.ts#L188-L204

https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/configuration/src/EnvironmentConfigFactory.ts#L335-L342

[trentm]

Continuing from #6679 (comment), this is my attempt to defend why I think the handling of an unspecified encoding for OtlpHttpExporter belongs in the create() step (as described by https://opentelemetry.io/docs/specs/otel/configuration/sdk/#create).

• The create() function should be able to handle any valid interface ConfigurationModel object.

This config is a valid ConfigurationModel:

const config: ConfigurationModel = {
  tracer_provider: {
    processors: [
      {
        batch: {
          exporter: { otlp_http: null },
        }
      }
    ]
  }
}

or, at least it is with the |null-related changes in this PR.
On the current main this one is valid:

...
          exporter: { otlp_http: {} },
...

Because the type is:

export type OtlpHttpExporter = {
  // ...
  encoding?: OtlpHttpEncoding;
};

The create() function shouldn't depend on some defaults having been added to the (valid) parsed config file.

• I agree with you that many of the default values should not be hardcoded in the create() step.

Using AttributeLimits.attribute_count_limit for example:

    "AttributeLimits": {
      "type": "object",
      "additionalProperties": false,
      "properties": {
        "attribute_count_limit": {
          "type": [
            "integer",
            "null"
          ],
          "minimum": 0,
          "description": "Configure max attribute count. \nValue must be non-negative.\nIf omitted or null, 128 is used.\n"
        }
      }
    },

There is no need for the 128 value to be in the create() code in sdk-node.
If config doesn't specify a value, an undefined can be passed to the
SDK component implementation (here BasicTracerProvider) to apply the default:

    const tracerProvider = new BasicTracerProvider({
      // ...
      generalLimits: {
        attributeCountLimit:
          config.attribute_limits?.attribute_count_limit ?? undefined,
      },
    });

There is a choice here: should a span limit default value live in the declarative configuration handling or in the implementation of the SDK component?

I think the latter, when possible. The Configuration spec says there must be a programmatic interface (https://opentelemetry.io/docs/specs/otel/configuration/#programmatic). For example, creating a BatchSpanProcessor programmatically:

const { BatchSpanProcessor } = require('@opentelemetry/sdk-trace-base');
const exporter = ...;
const proc = new BatchSpanProcessor(exporter);

This does not currently require the user to specify all options, like scheduleDelayMillis or maxQueueSize.
Assuming we do not want to require all options to be specified, this means that SDK components will include default values.

We can then avoid the work of adding these default values to the parsed config. So this config file:

file_format: "1.0"
tracer_provider:
  processors:
    - batch:
        exporter:
          otlp_http:

Could parse to this:

{
  tracer_provider: {
    processors: [
      {
        batch: {
          exporter: { otlp_http: ... },
        }
      }
    ]
  }
}

Rather than this:

{
  tracer_provider: {
    processors: [
      {
        batch: {
          exporter: { otlp_http: ... },
          schedule_delay: 5000,
          export_timeout: 30000,
          max_queue_size: 2048,
          max_export_batch_size: 512
        }
      }
    ]
  }
}

Back to the encoding: 'protobuf' | 'json' case.

• OtlpHttpExporter.encoding is an abberation in the Declarative Config schema.

I might be wrong, but I think that this part of the Configuration schema will be the only place where a sub-property (e.g. encoding) of a node (e.g. otlp_http) maps to a different SDK component in OTel JS. "otlp_http" with "encoding=protobuf" points to OTLPTraceExporter in @opentelemetry/exporter-trace-otlp-proto, but with "encoding=json" points to the one in @opentelemetry/exporter-trace-otlp-http.

My (naive, possibly unfair) guess is that this is a side-effect of encoding=json being an afterthought when Java-focused folks were designing the Declarative Config schema. Before Declarative Config the OTEL_EXPORTER_OTLP_PROTOCOL envvar was an enum of three values: grpc, http/protobuf, http/json (https://opentelemetry.io/docs/specs/otel/protocol/exporter/). And OTel Java currently doesn't support a first class OTLP json encoding: open-telemetry/opentelemetry-java#5833

My point is that while it may feel like a design wart to have create() code
handling a fallback value for otlp_http?.encoding, it fits the schema wart,
so I'm not too concerned about not having some level of purity in the create() implementation.

Much more important is that the parse() result encodes what's actually in
the config file.

What I don't want is each package calling the configuration one having to handle the default cases, because values can be easily missed and inconsistent.

The sdk-node package should not be handling this.

Now that I've removed all the defaults in this PR, except the otlp_http.encoding, from sdk-node I'm hoping this concern is satisfied.

If encoding is still an issue: What packages do you expect to be calling the configuration package? I don't see there being any callers beyond wherever the create() step lives -- currently in sdk-node. It is doubtful that a theoretical sdk-browser will use declarative config.

Possibly a future sdk-bun or sdk-deno or sdk-cloudflare-workers?
If we (happily) got to those, I think we could consider moving create() functionality out to a shared @opentelemetry/sdk-base (or whatever name).

[maryliag]

Sorry for the delay on replying here and thanks for the details of your thinking.
The reason why I prefer the default being set in the config package instead of the sdk-node package, was preparing for future cases (e.g. like browser, possible bun, deno, etc), and it would be hard to keep track if the default value change in just one of them.
Since right now this is not an issue, this is not a hill I'm willing to die on 😄

LGTM (once merge conflicts are resolved)

[trentm]

That lint failure was unrelated:

> opentelemetry@0.1.0 docs:test
> linkinator docs --silent --retry && linkinator doc/*.md --skip http://localhost:3000 --skip http://localhost:9464 --skip https://github.com/ --skip https://www.npmjs.com/ --silent --retry --directory-listing

→ crawling docs
✓ Successfully scanned 53 links in 0.327 seconds.
→ crawling doc/context.md doc/esm-support.md doc/exporter-guide.md doc/frequently-asked-questions.md doc/instrumentation-guide.md doc/metrics.md doc/propagation.md doc/sdk-registration.md doc/semconv-stable-http-and-database.md doc/tracing.md doc/upgrade-to-2.x.md
[500] https://opentelemetry.io/docs/concepts/signals/traces/
doc/semconv-stable-http-and-database.md
  [500] https://opentelemetry.io/docs/concepts/signals/traces/
ERROR: Detected 1 broken links. Scanned 38 links in 300.945 seconds.

Aside: our docs:test seems to me to effectively be a DoS engine. I'd love to have something that can (typically) only check doc links that have changed in a given PR.