refactor(sdk-node): move all existing create<SdkThing>FromConfig() to create-from-config.ts#6924
Conversation
…o create-from-config.js Also add tests for each of them, adding some tests to what was there before. The functions were *moved* from utils.ts to create-from-config.ts The only change was adding a few 'checkConfigUse()' internal calls.
trentm
changed the title
trentm
changed the title
| logRecordLimits, | ||
| // TODO: loggerConfigurator | ||
| // TODO: meterProvider | ||
| // Note: forceFlushTimeoutMillis not configurable via decl conf. |
There was a problem hiding this comment.
I know this was not added by your PR, but if is not via decla conf, how user can config it?
do we have anything else to use in the meantine, or the plan is to add this to declarative config?
not something for this pR, but wanted to check what is the plan
There was a problem hiding this comment.
I think this the way LoggerProvider and TracerProvider implement this unfortunately ended up being incompatible with declarative config.
We should allow providing options to forceFlush(), similar to how this is done in MeterProvider. That'll allow users to specify it when they call it. The spec does not have any opinion on how to make the timeout configurable, even leaves it open to the implementation to not allow configuring it.
ForceFlush SHOULD complete or abort within some timeout. ForceFlush MAY be implemented as a blocking API or an asynchronous API which notifies the caller via a callback or an event. OpenTelemetry SDK authors MAY decide if they want to make the flush timeout configurable.
My suggestion: let's do this like in sdk-metrics and deprecate/remove the option in the constructor of TracerProvider and LoggerProvider.
There was a problem hiding this comment.
I don't have a current plan, beyond calling out cases like this with a note when I see them.
I don't yet know if there are other cases where OTel JS SDK components have configuration options not defined by the current decl conf schema. Relatedly, configuration of instrumentations from YAML will likely never cover all instr config options: specifically the hooks that accept a JS function. (Unless, of course, we get crazy and allow JS hook code in YAML and eval it. That almost certainly won't happen.)
For this case:
- a
forceFlushTimeoutMilliswas, I think, first added to TracerProvider here: feat: addforceFlush-function to theBasicTracerProvider-class #2191 - I didn't see any discussion on that PR about where and whether to make the timeout configurable. An alternative option would have been for
TracerProvider#forceFlush()to accept and optional timeout parameter. - The spec says: "OpenTelemetry client authors can decide if they want to make the flush timeout configurable." So, I guess, per the spec we could "decide" to not make it configurable via decl conf.
There was a problem hiding this comment.
I guess I was writing my response here when Marc was writing and posting his. Marc wins both in time and actually creating issues and PRs.
Pull request dashboard statusStatus last refreshed: 2026-07-21 19:19:06 UTC.
This automated status or its linked feedback items may be incorrect. If something looks wrong, report it with the result you expected. |
4e8c9fd
github-project-automation
Bot
moved this from In Progress
to Done
in Declarative Configuration: JavaScript
3 participants
Also add tests for each of them, adding some tests to what was
there before.
The functions were moved from utils.ts to create-from-config.ts
The only change was adding a few 'checkConfigUse()' internal calls.
This continues on work from #6785
In that PR I added these methods to "utils.ts" because at the same time I'd had a PR starting "create-from-config.ts".
This is a refactor, mostly just moving existing functionality, before I continue work on "fail fast" semantics mentioned at #6785
See this comment from that PR: #6785 (comment)