Skip to content

fix(sdk-node)!: fail-fast on MeterProvider creation from config file - #6954

Open
trentm wants to merge 13 commits into
open-telemetry:mainfrom
trentm:trentm-sdk-create-from-config-4
Open

fix(sdk-node)!: fail-fast on MeterProvider creation from config file#6954
trentm wants to merge 13 commits into
open-telemetry:mainfrom
trentm:trentm-sdk-create-from-config-4

Conversation

@trentm

@trentm trentm commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

This refactors the MeterProvider creation from declarative config
in the startNodeSDK() code path:

  • fail-fast (and fallback to a no-op SDK) if the config has unknown
    values or values that aren't yet supported by the SDK
  • fixes a number of missed cases
  • adds a number of TODO for meter_provider config cases to follow-up on
    (Some of these are to keep this PR smaller, some are because
    sdk-metrics doesn't yet support everything the declarative config
    schema supports configuring.)

Refs: #6785
Refs: #6107 (comment)

This refactors the MeterProvider creation from declarative config
in the `startNodeSDK()` code path:
- fail-fast (and fallback to a no-op SDK) if the config has unknown
  values or values that aren't yet supported by the SDK
- fixes a number of missed cases
- adds a number of TODO for meter_provider config cases to follow-up on
  (Some of these are to keep this PR smaller, some are because
  sdk-metrics doesn't yet support everything the declarative config
  schema supports configuring.)

Refs: open-telemetry#6785
Refs: open-telemetry#6107 (comment)
@trentm trentm self-assigned this Jul 28, 2026
@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Jul 28, 2026

Copy link
Copy Markdown

Pull request dashboard status

Waiting on reviewers · refreshed 2026-07-30 17:29 UTC

Review the latest changes.

Status above doesn't look right?
  • Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
  • Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.91339% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.97%. Comparing base (7b06368) to head (addf7cc).

Files with missing lines Patch % Lines
...s/opentelemetry-sdk-node/src/create-from-config.ts 92.80% 18 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6954      +/-   ##
==========================================
- Coverage   95.07%   94.97%   -0.11%     
==========================================
  Files         409      409              
  Lines       14256    14328      +72     
  Branches     3267     3292      +25     
==========================================
+ Hits        13554    13608      +54     
- Misses        702      720      +18     
Files with missing lines Coverage Δ
...ental/packages/opentelemetry-sdk-node/src/start.ts 96.87% <100.00%> (-0.16%) ⬇️
...ental/packages/opentelemetry-sdk-node/src/utils.ts 95.90% <100.00%> (-1.31%) ⬇️
...s/opentelemetry-sdk-node/src/create-from-config.ts 93.76% <92.80%> (-1.66%) ⬇️
🚀 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 added 5 commits July 28, 2026 10:17
Also limit reading of TLS files to *absolute paths* as required
by the declarative config.

Note: that doesn't work properly for the 'build-config-from-env-vars'
code path.

Update to have real TLS files with the usual maint:... npm script.
Real TLS files are required by createSslCredentials(), which was
being masked before by the just-warn semantics.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The files and regenerate.sh script in this dir are the exact same setup we use for testable TLS files in the otlp-exporter-base and otlp-grpc-exporter-base packages.

@trentm

trentm commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Before/after comparison of test coverage for the 'sdk-node' package.

Before:

  252 passing (2s)

-----------------------|---------|----------|---------|---------|-----------------------------------------
File                   | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
-----------------------|---------|----------|---------|---------|-----------------------------------------
All files              |   97.36 |    93.67 |   98.92 |   97.45 |
 create-from-config.ts |   95.37 |    90.76 |   94.44 |   96.22 | 164,183,223,284
 create-from-env.ts    |     100 |      100 |     100 |     100 |
 diag.ts               |     100 |      100 |     100 |     100 |
 sdk.ts                |    97.7 |    97.75 |     100 |    97.7 | 123,304,430
 semconv.ts            |     100 |      100 |     100 |     100 |
 start.ts              |    96.8 |    90.32 |     100 |    96.8 | 192,195,198
 utils.ts              |   97.65 |    92.97 |     100 |    97.6 | 160,166,205,209-212,392,624-625,719,744
-----------------------|---------|----------|---------|---------|-----------------------------------------

After:

  253 passing (2s)

-----------------------|---------|----------|---------|---------|---------------------------------------------
File                   | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
-----------------------|---------|----------|---------|---------|---------------------------------------------
All files              |   96.63 |     91.7 |   98.96 |   96.71 |
 create-from-config.ts |    95.6 |    88.88 |   97.22 |   95.94 | 248,308,327,367,432,579,609,836,857,882,943
 create-from-env.ts    |     100 |      100 |     100 |     100 |
 diag.ts               |     100 |      100 |     100 |     100 |
 sdk.ts                |    97.7 |    97.75 |     100 |    97.7 | 123,304,430
 semconv.ts            |     100 |      100 |     100 |     100 |
 start.ts              |   96.66 |    89.65 |     100 |   96.66 | 188,191,194
 utils.ts              |   96.67 |    91.02 |     100 |   96.57 | 140,146,185,189-192,372,542,567,576-577
-----------------------|---------|----------|---------|---------|---------------------------------------------
  • Still good coverage numbers.
  • No changes in utils.ts (the same lines are uncovered, the % Stmts dropped because there are fewer lines in utils.ts now)
  • create-from-config.ts: % Stmts slightly better. More "Uncovered Lines", but every one added is a default: ... block of a case statement. I can add tests for those, but didn't seem necessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer note: Sorry, I know reviewing this file will be tedious. I both moved and improved functionality that was in "utils.ts". For example getMeterViewsFromConfiguration() became createViewOptionsFromConfig() in this file.

cert_file: /app/cert.pem
ca_file: ${TEST_DIR}/certs/ca.crt
key_file: ${TEST_DIR}/certs/client.key
cert_file: ${TEST_DIR}/certs/client.crt

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer note: We are now strictly requiring that TLS files exist.

@trentm
trentm marked this pull request as ready for review July 29, 2026 20:56
@trentm
trentm requested a review from a team as a code owner July 29, 2026 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

1 participant