feat(sdk-node, configuration)!: refactor of *env*-based code path for startNodeSDK - #6999
feat(sdk-node, configuration)!: refactor of *env*-based code path for startNodeSDK#6999trentm wants to merge 43 commits into
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)
… full effort is done on startNodeSDK() usage with env-based config
… a parseConfigFile now
… path I believe this is basically working, but I haven't yet added tests.
…EnvDetector`, mark `serviceInstanceIdDetector` stable Deprecate `envDetector` in favor of separate `resourceAttributesEnvDetector` and `serviceNameEnvDetector`. Also mark `serviceInstanceIdDetector` as stable (the `service.instance.id` semconv attribute is now stable).
…IBUTES, when 'service' detector is being used
…default set; note serviceInstanceIdDetector as stable
…ase'; consistent with opts.resourceDetectors
…rks that way. No need for out-of-band null to signal this. Also consistent with opts.propagators and opts.resourceDetectors
Pull request dashboard statusWaiting on reviewers · refreshed 2026-09-04 11:22 UTC Review the latest changes. Also blocked by: Merge conflicts. Status above doesn't look right?
|
|
The CodeQL check failure is because of a current GH incident. |
…guration packages
|
big change, thank you for working on it, but I'm not sure I agree 😅 I created the configuration package so that one would be responsible for the setup of the config that any other package would need, could be from a file or could be env. Other repos have created something similar to what you have here, with the config package looking at only file, but then when using, you need to also have an env config path, but this makes hard to find all around the code the places where something should be updated. If you have a single package that handles config (file and env), it's all contained and it's easier to find all existing config, tests and so on. With the way I created whoever is calling the config object, doesn't have to care about the setup itself, because it was already done, it just calls the factory and that's it. But now you had to create So you're making a big change that would be the opposite of my initial intention, so I would like to understand more what is the advantage of those changes. |
|
I hear the impedance mismatch case for OTEL_NODE_RESOURCE_DETECTORS=all etc., but I'd rather solve those with an explicit env-overrides mechanism inside configuration than by moving all env parsing into sdk-node. Some options in between the current version and the new could be: Keep unified for the env → model layer, but let SDKs augment. configuration handles the declarative-config-expressible env vars (which is most of them), and each SDK adds its own layer for Node-specific env vars only. Keep the split you have has now, but make the common shape reusable. Instead of full duplication between create-from-config and create-from-env, each helper function takes a common "resolved config" shape and both entry points build it their own way. Cuts the "two places to update per new feature" cost. |
|
(Sorry, this got long.) It seems appealing to have a design where all sources of config can be read and validated into one common model ( However, we don't have that many of either. There are two sources of config: env and YAML config file. There is currently one user of the config model: the
Creating a ConfigurationModel from envvars, to then be used to create SDK components, is a leaky abstraction. One leaky case is
Sure, we could try that. Perhaps something like this function startNodeSdk() {
const factory = createConfigFactory({
customizeConfigForEnv: (config) => {
// Read OTEL_NODE_RESOURCE_DETECTORS and add `config['detection/development'].detectors`...
}
});
const config = factory.getConfig();
// ...
}Another leak is error handling. The spec says that (a) creating the SDK from env should warn and gracefully ignore invalid env vars (https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#type-specific-guidance) and (b) creating the SDK from config should fail-fast (https://opentelemetry.io/docs/specs/otel/configuration/sdk/#create). If we have one common As well, good error messages are harder to make when the configuration source is abstracted away. In the env-specific The following spec discussion suggests a direction where env-based configuration should not evolve with config-based. open-telemetry/opentelemetry-specification#3967 (comment)
This is only an issue discussion and not spec, but it implies an intent that when/if we add support for PluginComponentProvider, then the env-based configuration code path should not use that component provider. If we have shared Q1: What options should As a replacement for Q2: Assuming we offer similar options, should those options be used when file-based config is used? I think no. If using a config file, then the config should only come from Assuming you agree with that call, then a single Regarding maintenance.
I disagree. Or, at the least, I think it is a wash deciding ahead of time which way is less maintenance. Above I've argued that error handling mode, good error messages, PluginComponentProvider usage, and handling The current PR adds full support for |
|
|
||
| const instrumentations = opts?.instrumentations?.flat(); | ||
| if (instrumentations) { | ||
| registerInstrumentations({ instrumentations }); |
There was a problem hiding this comment.
[question]: any reason to register the instrumentations before providers are resolved? AFAIK the method sets the different providers (logs, traces, metrics) for each instrumentation so if providers are set 1st the instrumentations will get the right tracer, logger and meter then the if in L156 is not necessary
There was a problem hiding this comment.
David,
A similar point was brought up recently here: #6989 (comment)
See my response on that thread.
tl;dr: Yes, you are right. I do want to change to calling registerInstrumentations(...) after the providers have been registered. However, as with the PR above, I don't want to add this change to this PR because:
- It keeps this (already controversial) PR a little less busy. Both
startNodeSDK()andnew NodeSDK()before this change are doingregisterInstrumentations(...)before creating and registering the providers. - My proposed change to the ConfigProvider PR -- feat(sdk-node,instrumentation,instrumentation-http,api-config,configuration): add declarative config support for
instrumentation/development#6868 (comment) -- is doing this move ofregisterInstrumentations()to be after creating/registering the providers. See here: 45e9a42...trentm:opentelemetry-js:inst-config-provider#diff-44b3cba79d8ee04c3efd823142cf926e5effe45f31c1199ff8eae607c3500885R86-R116
(For historical interest: This PR is where registerInstrumentations() was moved to be before creating/registering the providers. This was because, at the time, sdk.start() was async to deal with async resource detectors. That's no longer relevant. #3502)
Closes: #5945
Closes: #6107
Closes: #6488
Obsoletes: #6988
overview
This is a significant refactor (especially a breaking change to what the
configurationpackage is about).startNodeSdk()code path handling env-based config -- i.e. the thing that is intended to replaceconst sdk = new NodeSDK(); sdk.start()-- is fully implemented.startNodeSdk()now supports a full set of options (e.g.opts.views,opts.resourceDetectors, et al). Those options are not supported when using a config file, the point of declarative config is that the full config is defined by the YAML file. The options (including detailed diffs fromnew NodeSDK()options) are here: https://github.com/open-telemetry/opentelemetry-js/pull/6999/changes#diff-cf9f37462550254b82df2157b0ddaf0ba4c0a12782d46b803a076117740aac46R55-R206configurationpackage has been reduced to just having the parsing, validation, and TypeScript types for theConfigurationModel. It no longer handles envvars at all. Those have moved tosdk-node.new NodeSDK()andstartNodeSdk(). See section below for this.Resource detector changes
We've been discussing how the SDK should create the Resource lately. (
serviceInstanceDetectorwithserviceDetector#6488)
The main question is how to handle the user configuring resource detectors given that:
OTEL_NODE_RESOURCE_DETECTORSwith nameshost, os, process, serviceinstance, env,hostandservicenames that conflict.This PR implements the following changes:
new NodeSDK(): Theosdetector has been added to the default set. Some history:osDetectorwas created in 2022 in feat: implement OSDetector #2927,hostDetectorwas added to the default detectors in Mar 2024 in feat(sdk-node): addHostDetectoras default resource detector #4566. I see no reason not to addosDetectorto the default set.new NodeSDK(): Theserviceinstancedetector has been added to the default set, becauseservice.instance.idis now stable in semconv.Other that those two things,
new NodeSDK()behaviour is unchanged.When a user switches to using
startNodeSdk()(and is not using a config file), thenOTEL_NODE_RESOURCE_DETECTORSnames will change to the spec names.OTEL_RESOURCE_ATTRIBUTESis now always read,OTEL_SERVICE_NAMEis handled by the 'service' detectorOTEL_SERVICE_NAMEandservice.instance.id.os.*attributes are now provided with the 'host' detector name.host.*andos.*attributesstartNodeSdk()will diag.warn appropriately when the old detector names are used, e.g.:By default, i.e. when
OTEL_NODE_RESOURCE_DETECTORSis not specified, the detector behaviour betweennew NodeSDK()andstartNodeSdk()will be the same.No breaking changes to the
resourcespackage were needed. Instead newresourceAttributesEnvDetectorandserviceNameEnvDetectordetectors were added andenvDetectordeprecated: 8b5b494#diff-4b2e7f2d8a9a7c6779860d70c98b1c3b0d11151296d044f1f261ae69b53ec98cbreaking changes
configurationpackage API has changed (mostly to just aparseConfigFile()method and types).Why
startNodeSDK()->startNodeSdk()name change?I changed to
startNodeSdk(). Why?In my changes, I named the type for the thing that
startNodeSdk()returns (the object with ashutdown()method). I named itNodeSdk, which seems natural. It is handy that this doesn't collide with the existingNodeSDKclass -- although fair that they are close so there is some potential for confusion there. I'm thinking forward to when we deprecateNodeSDKsoonish.The fairly recent Browser SDK package has a
const sdk: WebSdk = startBrowserSdk(). See https://github.com/open-telemetry/opentelemetry-browser/blob/main/packages/sdk/src/index.ts. This matches that change, for what its worth.There aren't a lot of current examples of exported function names in opentelemetry-js.git where all-caps-or-not-for-acronyms is a consideration. Some of Marc's recentish work on exporters bias towards not all-caps, e.g.: