chore(configuration)!: change parseConfigFile() to *not* add default values, nor merge *_list fields#6765
Conversation
…values for unspecified fields - Handling of defaults (nullBehavior and defaultBehavior) belongs in the `create()` step. https://opentelemetry.io/docs/specs/otel/configuration/sdk/#create - Config parsing (and validation) is much simpler and clearer. The returned `type ConfigurationModel` object has exactly the data that is specified in the file. It still matches the generated TypeScript types, which the `parse()` handling (currently in the sdk-node package) needs to handle. This is a continuation of open-telemetry#6679 which removed some post-processing of a parsed and validated config file. See longer discussion here: open-telemetry#6679 (comment)
There was a problem hiding this comment.
Reviewer note: These formatting changes in tsconfig.* files are from the top-level "postinstall" npm script that updates and re-writes tsconfig files after npm install. This was missed in #6758.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6765 +/- ##
==========================================
- Coverage 95.57% 95.57% -0.01%
==========================================
Files 387 387
Lines 13162 13146 -16
Branches 3016 3001 -15
==========================================
- Hits 12580 12564 -16
Misses 582 582
🚀 New features to boost your workflow:
|
That helper for merging resource.attributes and .attribute_list is now exported as `mergeResourceAttributesConfig(...)`. Responsibility for merging moves to the "create()" step -- in sdk-node.
The helper for merging propagator.composite and .composite_list is now exported as `mergePropagatorCompositeConfig(...)`. Responsibility for merging moves to the "create()" step, currently in sdk-node.
MikeGoldsmith
left a comment
There was a problem hiding this comment.
Thanks for putting this together @trentm. Is this ready for feedback?
@MikeGoldsmith Almost. See the "Checklist" in the description for some work that this PR is waiting for. The big chunk of work was #6775 which just merged. Next is #6817. Then I can finish this off. |
maryliag
left a comment
There was a problem hiding this comment.
Once the last nit is addressed and tests are passing, LGTM
*_list fields
With this change, the
parseConfigFile()will no longer add default values (e.g. addinglog_level: 'info'if the config file YAML did not specifylog_level) and will no longer merge some*_listfields (e.g. mergingresource.attributes_listintoresource.attributes).create()step.https://opentelemetry.io/docs/specs/otel/configuration/sdk/#create
returned
type ConfigurationModelobject has exactly the data that isspecified in the file. It still matches the generated TypeScript
types, which the
create()handling (currently in the sdk-nodepackage) needs to handle.
configurationpackage now exports utility functions for doing some of the*_listfield merging, that is used by the "create" handling in the sdk-node package.This is a continuation of #6679 which removed some post-processing of a
parsed and validated config file. See longer discussion here:
#6679 (comment)