fix(sdk-node): fail-fast on LoggerProvider creation from config

[trentm]

This PR includes a few changes to declarative config "create()" handling in startNodeSDK().
I've started with just the config handling of logger_provider to have a smaller start and smaller diff for review.

1. This starts a move to "fail fast" semantics: If there is a problem creating SDK components from the parsed config, then create() throws, and startNodeSDK() warns and returns a no-op SDK. This matches OTel Java behaviour and the spec. From https://opentelemetry.io/docs/specs/otel/configuration/sdk/#create

This SHOULD return an error if it encounters an error in configuration (i.e. fail fast) in accordance with initialization error handling principles.

2. I'm proposing a naming pattern:

    create<SDK thing>FromConfig()   # e.g., createLogRecordExporterFromConfig

for the suite of functions that create an SDK thing from configuration data.
"create" because these are all used by the SDK configuration "create()" step (https://opentelemetry.io/docs/specs/otel/configuration/sdk/#create).
"FromConfig" because it is shorter than "FromConfiguration", and these functions don't always take a full ConfigurationModel.

Currently we have these patterns:

    get<SDK thing>                    # getLogRecordExporter
    get<SDK thing>FromConfiguration   # getSpanProcessorsFromConfiguration
    build<SDK thing>FromConfig        # buildSamplerFromConfig

3. This fixes missing handling of logger_provider.limits, which were previously not being passed to the new LoggerProvider(...).

4. I've added a checkConfigUse(...) internal function that can help warn about unhandled config properties in create<SDK thing>FromConfig() functions. It is pretty low tech.
   With this config:

logger_provider:
  logger_configurator/development:
    default_config:
      enabled: true
  processors:
    - simple:
        exporter:
          console:

Before: this would silently ignore "logger_configurator/development".
After: it diag.warn's

Config warning: some specified LoggerProvider configuration properties were not handled by SDK setup: ["logger_configurator/development"]

[codecov]

Codecov Report

❌ Patch coverage is 92.03540% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.93%. Comparing base (940eb20) to head (7458520).

Files with missing lines	Patch %	Lines
...ental/packages/opentelemetry-sdk-node/src/utils.ts	91.78%	6 Missing ⚠️
...ental/packages/opentelemetry-sdk-node/src/start.ts	92.50%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6785      +/-   ##
==========================================
- Coverage   94.97%   94.93%   -0.04%     
==========================================
  Files         383      383              
  Lines       12968    13007      +39     
  Branches     2961     2974      +13     
==========================================
+ Hits        12316    12348      +32     
- Misses        652      659       +7     

Files with missing lines	Coverage Δ
...ental/packages/opentelemetry-sdk-node/src/start.ts	97.05% <92.50%> (-2.95%)	⬇️
...ental/packages/opentelemetry-sdk-node/src/utils.ts	96.50% <91.78%> (-0.52%)	⬇️

🚀 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.

[trentm]

4. I've added a checkConfigUse(...) internal function [...]
it diag.warn's

Arguably this could be made to throw per the "fail fast" policy.

[maryliag]

to confirm (as per your PR description), do you plan on opening following PRs to update here to createMeterProviderFromConfig and below a createTracerProviderFromConfig?
if your plan is to do this right after, then fine, otherwise make sure you open an issue so is not forgotten :)

[trentm]

Yes, definitely. I wanted to start with just one in case discussion/review suggested significant changes.

[maryliag]

on the existing sdk start parameters (NodeSDKConfiguration) the contextManager is required, so I followed that. Any reason why now should be optional?

[trentm]

Making all the SDKComponents properties optional was necessary (without some coding pain) to support this change where components = {} is created outside the try { ... } so it can be used inside the catch { ... }:

  const components: SDKComponents = {};

  try {
    components.contextManager = new AsyncLocalStorageContextManager();
    components.contextManager.enable();
    ...
  } catch (err) {
     // ... use `components` here for cleanup
  }

Interface SDKComponents is only used for the return type of the internal create() function, so there is no breaking change for the user here.

[MikeGoldsmith]

Looking good, thanks @trentm.

[MikeGoldsmith]

We've lost the default of 1000ms here.

[trentm]

Yes, see #6796

[MikeGoldsmith]

Should this also throw like the mustSingleEntry func?

[trentm]

Thanks for the review. I went back and forth on this and settled on not throwing, at least for now.

Say a future v1.2.0 of the configuration spec allows a new property for attribute_limits, and assuming the guard on exact file_format values changes to allow future v1.x versions. Do we want declarative config file handing to completely fail if that new attribute limit isn't handled yet by the current sdk-node package? I realize this is theoretical because the current configuration handling doesn't do semver-based handling of the file_format.

Happy to change to throwing here.