fix(sdk-node)!: fixes for and fail-fast on Resource creation from config file - #6989
Conversation
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)
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.
…h handles negative, and fails-fast
…d unused utils.ts code and tests
…-from-config-TracerProvider
…fig file **Note: This depends on the following PRs being merged first: open-telemetry#6954, open-telemetry#6962, open-telemetry#6987. ** This refactors and fixes creation of the Resource for the "startNodeSDK()" code path. `setupResource(...)` -> `createResourceFromConfig(...)` fix: The `defaultResource()` is now always included. It isn't super clear from the declarative config spec that this is intended, but if it helps this is the OTel Java behaviour: https://github.com/open-telemetry/opentelemetry-java/blob/d948e130ebf31fd41136ee2d72e28729047f8b9d/sdk-extensions/declarative-config/src/main/java/io/opentelemetry/sdk/autoconfigure/declarativeconfig/ResourceFactory.java#L36 This means that a declarative config like this will still get the default `service.name` and `telemetry.sdk.*` attributes. ```yaml resource: attributes: - name: foo value: bar ``` fix: Attributes specified in `resource.attributes` properly override values from resource detectors. fix: Fail-fast (i.e. throw, resulting in a no-op SDK) when there are unknown resource detectors specified. fix: Change the behaviour of the "well-known" resource detectors to match that described in the spec (and the OTel Java impl): https://opentelemetry.io/docs/specs/otel/resource/sdk/#resource-detector-name This is a **breaking change** for `startNodeSDK()` usage, and there is some confusion on resource detector names to clarify. - The spec'd behaviour for the "host" and "service" detector names are **different** to the behaviour of the "host" and "service" detector names used in `OTEL_NODE_RESOURCE_DETECTORS`. - The spec says "host": "Populates `host.*` and `os.*` attributes." This is equivalent to "host" **and "os"** in `OTEL_NODE_RESOURCE_DETECTORS`. - The spec says "service": "Populates `service.name` based on `OTEL_SERVICE_NAME` environment variable; populates `service.instance.id` [...]". This is close, **but not quite**, to "service" and "env" in `OTEL_NODE_RESOURCE_DETECTORS`. - "Not quite" the same, because if we used the existing `envDetector` for handling a file-based config, we would **incorrectly** read resource attributes from `OTEL_RESOURCE_ATTRIBUTES`. - I struggled with this for a while, figuring Java would have the same confusion. OTel Java *does* have an option similar to `OTEL_NODE_RESOURCE_DETECTORS`. It is `OTEL_JAVA_ENABLED_RESOURCE_PROVIDERS` (or the `otel.java.enabled.resource.providers` system property). However, the strings passed to this setting are **not** convenient short strings, they are instead fully qualified class names, e.g.: ```bash export OTEL_JAVA_ENABLED_RESOURCE_PROVIDERS=io.opentelemetry.sdk.autoconfigure.EnvironmentResourceProvider,io.opentelemetry.instrumentation.resources.HostResourceProvider ``` There is no confusion in Java-land between those settings and "host" in a declarative config file. - Options: 1. We treat the names used in `OTEL_NODE_RESOURCE_DETECTORS` and those in `detectors: ...` in a declarative config file as **not in the same namespace**. They are unrelated names that cannot be mapped to each other. 2. We *break* the meaning of the current "host", "service" and "env" detectors used in `OTEL_NODE_RESOURCE_DETECTORS` to match what the spec now uses. In this PR I have opted for Option 1. (Option 2 means breaking current de facto stable behaviour.) There are implications to this discussed below. Implications of `OTEL_NODE_RESOURCE_DETECTORS names !== declarative config detectors names`. The main implication is that `FileConfigFactory` in the "configuration" package **cannot express `OTEL_NODE_RESOURCE_DETECTORS` in a `ConfigurationModel`.** This was somewhat already true, because there is no way to express `OTEL_NODE_RESOURCE_DETECTORS=all` in `ConfigurationModel`. I have ideas for changes in the sdk-node and configuration packages around this, but I propose to handle them in a separate issue/PR. (I'll link back to this one.) This PR also tweaks the dev tool "configuration/scripts/parse-config.mjs" to read config from the environment if an argument is not provided. This is useful for inspecting what the "config from environment" code path is generating as a ConfigurationModel. Refs: open-telemetry#6107 (comment)
Pull request dashboard statusMerged · refreshed 2026-08-21 16:57 UTC Status above doesn't look right?
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6989 +/- ##
==========================================
- Coverage 95.02% 94.98% -0.05%
==========================================
Files 409 409
Lines 14298 14335 +37
Branches 3277 3276 -1
==========================================
+ Hits 13587 13616 +29
- Misses 711 719 +8
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
This change in this file is independent. This file is a dev script. The change is to allows viewing the ConfigurationModel that the configuration package generates from envvars, which is convenient for comparing to the model it generates for a YAML config file.
JacksonWeber
left a comment
There was a problem hiding this comment.
Overall, LGTM apart from one nit.
…of env-based config
…ailures
In CI I saw this failure:
> 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 55 links in 0.239 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
[403] https://cloud-native.slack.com/archives/C01NL1GRPQR
doc/upgrade-to-2.x.md
[403] https://cloud-native.slack.com/archives/C01NL1GRPQR
| components.contextManager.enable(); | ||
|
|
||
| const resource = setupResource(config, sdkOptions); | ||
| const resource = createResourceFromConfig(config.resource); |
There was a problem hiding this comment.
Could we create the resource before registering instrumentations? If resource creation fails here, the instrumentations have already patched modules and returning NOOP_SDK won’t undo that setup.
There was a problem hiding this comment.
I added a suggestion on location for the register that hopefully would address. You can create a test to confirm, something like
it('should not register instrumentations if SDK creation fails', async () => {
process.env.OTEL_NODE_RESOURCE_DETECTORS = 'container'; // triggers throw
const instrumentation = { enable: Sinon.fake(), disable: Sinon.fake() /* ... */ };
const sdk = startNodeSDK({ instrumentations: [instrumentation as any] });
// NOOP_SDK returned, instrumentation.enable() was never called.
Sinon.assert.notCalled(instrumentation.enable);
await sdk.shutdown();
});
There was a problem hiding this comment.
So... Instrumentation .enable() ordering is a rabbit hole.
Putting this registerInstrumentations() after the create(...) doesn't avoid enabling instrumentations in the common case. This is because the class InstrumentationBase for both node and browser enable the instrumentation in the constructor. That means in most cases, by the time instrumentations are passed to new NodeSDK() or startNodeSDK() the instrumentations are already enabled, i.e. have already done their monkey patching.
Ultimately I'd like to have registerInstrumentations() later in SDK setup.
And for other reasons: I'd like instrumentations to setup after the providers have been globally registered (trace.setGlobalTracerProvider(...) et al).
- Doing so would possibly eliminate the current need that we have for proxying of providers in the
apipackage. E.g. currently when youapi.getTracerProvider()before one is registered, you don't get a NOOP_TRACER_PROVIDER. Instead you get a ProxyTracerProvider that will handle the case of a real TracerProvider being registered later. Once the real provider is registered, calls you make on the earlier returned ProxyTracerProvider will pass through to the real provider. We don't currently have a proxy for MeterProvider and Meter, so we currently have a hackprotected _updateMetricInstruments(): voidon the baseclass InstrumentationAbstractclass. - Doing so would also handle a difficulty with the coming ConfigProvider (see feat(sdk-node,instrumentation,instrumentation-http,api-config,configuration): add declarative config support for
instrumentation/development#6868). If an instrumentation is to read some config from the declarative config YAML file, it must read that config after the SDK has registered the ConfigProvider.
This is a complex-enough topic that I'd prefer to deal with it in a separate issue/PR.
| components.contextManager.enable(); | ||
|
|
||
| const resource = setupResource(config, sdkOptions); | ||
| const resource = createResourceFromConfig(config.resource); |
There was a problem hiding this comment.
I added a suggestion on location for the register that hopefully would address. You can create a test to confirm, something like
it('should not register instrumentations if SDK creation fails', async () => {
process.env.OTEL_NODE_RESOURCE_DETECTORS = 'container'; // triggers throw
const instrumentation = { enable: Sinon.fake(), disable: Sinon.fake() /* ... */ };
const sdk = startNodeSDK({ instrumentations: [instrumentation as any] });
// NOOP_SDK returned, instrumentation.enable() was never called.
Sinon.assert.notCalled(instrumentation.enable);
await sdk.shutdown();
});
Co-authored-by: Marylia Gutierrez <maryliag@gmail.com>
…or codein the resources package Co-authored-by: Marylia Gutierrez <maryliag@gmail.com>
Co-authored-by: Marylia Gutierrez <maryliag@gmail.com>
tl;dr: Refactor
setupResource(...)->createResourceFromConfig(...)for thestartNodeSDK()code path. It changes to "fail-fast" semantics: i.e. any failure to handle the given declarative config throws and results in a no-op SDK. It fixes some edge cases. It breaks the usage ofstartNodeSDK()when envvar-based config for now -- with a plan to follow-up on this. This code path was already incomplete anyway.This refactors and fixes creation of the Resource for the "startNodeSDK()" code path.
setupResource(...)->createResourceFromConfig(...)fix: The
defaultResource()is now always included.It isn't super clear from the declarative config spec that this is intended, but if it helps this is the OTel Java behaviour:
https://github.com/open-telemetry/opentelemetry-java/blob/d948e130ebf31fd41136ee2d72e28729047f8b9d/sdk-extensions/declarative-config/src/main/java/io/opentelemetry/sdk/autoconfigure/declarativeconfig/ResourceFactory.java#L36
This means that a declarative config like the following will still get the default
service.nameandtelemetry.sdk.*attributes.fix: Attributes specified in
resource.attributesproperly override values from resource detectors.fix: Fail-fast (i.e. throw, resulting in a no-op SDK) when there are unknown resource detectors specified.
fix: Change the behaviour of the "well-known" resource detectors to match that described in the spec (and the OTel Java impl):
https://opentelemetry.io/docs/specs/otel/resource/sdk/#resource-detector-name
This is a breaking change for
startNodeSDK()usage, and there is some confusion on resource detector names to clarify.OTEL_NODE_RESOURCE_DETECTORS(and thehostDetectorandserviceDetectorimplementations in the "resources" package).host.*andos.*attributes."This is equivalent to "host" and "os" in
OTEL_NODE_RESOURCE_DETECTORS.service.namebased onOTEL_SERVICE_NAMEenvironment variable; populatesservice.instance.id[...]".This is close, but not quite, to "service" and "env" in
OTEL_NODE_RESOURCE_DETECTORS.envDetectorfor handling a file-based config, we would incorrectly read resource attributes fromOTEL_RESOURCE_ATTRIBUTES.OTel Java does have an option similar to
OTEL_NODE_RESOURCE_DETECTORS.It is
OTEL_JAVA_ENABLED_RESOURCE_PROVIDERS(or theotel.java.enabled.resource.providerssystem property).However, the strings passed to this setting are not convenient short strings, they are instead fully qualified class names, e.g.:
export OTEL_JAVA_ENABLED_RESOURCE_PROVIDERS=io.opentelemetry.sdk.autoconfigure.EnvironmentResourceProvider,io.opentelemetry.instrumentation.resources.HostResourceProviderOTEL_NODE_RESOURCE_DETECTORSand those indetectors: ...in a declarative config file as not in the same namespace. They are unrelated names that cannot be mapped to each other.OTEL_NODE_RESOURCE_DETECTORSto match what the spec now uses.(Option 2 means breaking current de facto stable behaviour.)
There are implications to this discussed below.
Implications of
OTEL_NODE_RESOURCE_DETECTORS names !== declarative config detectors names.The main implication is that
FileConfigFactoryin the "configuration" package cannot expressOTEL_NODE_RESOURCE_DETECTORSin aConfigurationModel.This was somewhat already true, because there is no way to express
OTEL_NODE_RESOURCE_DETECTORS=allinConfigurationModel.I have ideas for changes in the sdk-node and configuration packages around this, but I propose to handle them in a separate issue/PR. (See #6999.)
breaking change: I've dropped the
resourceDetectorsoption tostartNodeSDK()for now.There is no need for this option for file-based config. For env-based config there is an argument for it. However, no one is using env-based config with
startNodeSDK(): we don't advertisestartNodeSDK()at all, and the env-based creation of aConfigurationModelin the "configuration" package already has some basic issues that limit its usefulness (e.g. no tracer provider is created unlessOTEL_TRACES_EXPORTERis given a value, it should default to "otlp").As well, for
startNodeSDK()to be a reasonable replacement fornew NodeSDK()for env-based config, it needs to grow back some of the options thatNodeSDKsupports (like passing in span processors, log record processors, metric views, et al). I propose to come back to all this in follow-up PRs. (#6999 re-addsresourceDetectorsand a full set of options to replacenew NodeSDK(...)usage.)This PR also tweaks the dev tool "configuration/scripts/parse-config.mjs" to read config from the environment if an argument is not provided. This is useful for inspecting what the "config from environment" code path is generating as a ConfigurationModel.
Refs: #6107 (comment)