fix(configuration): add missing config fallback defaults for file/env consistency#6717
Draft
MikeGoldsmith wants to merge 3 commits into
Draft
fix(configuration): add missing config fallback defaults for file/env consistency#6717MikeGoldsmith wants to merge 3 commits into
MikeGoldsmith wants to merge 3 commits into
Conversation
- Fix BLRP schedule_delay default: was 5000 (same as BSP) but spec says 1000 for batch log record processors - Add log_level: 'info' to initializeDefaultConfiguration so env-based config matches file-based config default - Add tracer provider limits defaults (event_count_limit, link_count_limit, event_attribute_count_limit, link_attribute_count_limit) in FileConfigFactory - Add logger provider attribute_count_limit default in FileConfigFactory - Add meter provider exemplar_filter: 'trace_based' default in FileConfigFactory Closes open-telemetry#5945 Assisted-by: Claude Opus 4.6
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6717 +/- ##
=======================================
Coverage 94.86% 94.87%
=======================================
Files 376 376
Lines 12712 12732 +20
Branches 2883 2896 +13
=======================================
+ Hits 12059 12079 +20
Misses 653 653
🚀 New features to boost your workflow:
|
Extend test-for-coverage.yaml with tracer_provider (limits), meter_provider (periodic reader), and logger_provider (limits) to exercise the new default-applying code paths. Assisted-by: Claude Opus 4.6
trentm
reviewed
May 15, 2026
Contributor
trentm
left a comment
There was a problem hiding this comment.
I would like to hold on these changes. See the discussion at #6679 (comment) about where defaults should be applied for declarative config handling.
tl;dr: I don't think the "parse" handling in the configuration package should be adding any defaults to the parsed config object.
I'm not going to block, though. While I hope the #6679 discussion is resolved soon enough, it has been stalled for a little while.
Member
Author
|
Moving to draft until #6679 is resolved |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
Several spec-defined default values were missing or inconsistent between
FileConfigFactory(YAML config) andEnvironmentConfigFactory(env vars), causing different behavior depending on config source.Key issues:
schedule_delaydefaulted to 5000ms (BSP value) instead of the spec-defined 1000ms for batch log record processorslog_leveldefaulted to'info'in file config but was unset in env configexemplar_filterdefaulted to'trace_based'in env config but not in file configShort description of the changes
FileConfigFactory.ts:applyBatchProcessorDefaultsinto separate span (5000ms) and log (1000ms) schedule_delay defaultsevent_count_limit,link_count_limit,event_attribute_count_limit,link_attribute_count_limit(all 128)attribute_count_limitdefault (128)exemplar_filterdefault ('trace_based')utils.ts:log_level: 'info'toinitializeDefaultConfiguration()so env-based config matches file-based configType of change
How Has This Been Tested?
82 configuration package tests pass. 189 sdk-node tests pass. Full repo compiles cleanly.
Checklist:
Closes #5945