fix: block builder honor the global storage block config for block and WAL versions#6532
Merged
javiermolinar merged 2 commits intografana:mainfrom Feb 24, 2026
Conversation
…d WAL versions The block builder was hardcoding the default encoding version in BlockConfig.RegisterFlagsAndApplyDefaults, ignoring the global storage.trace.block.version config. This replicates the approach from grafana#6509 (live store fix): introduce a GlobalBlockConfig field injected by the application, and a coalesceBlockVersion function that resolves the encoding with proper fallback (default -> global -> local block_config). Fixes grafana#6451
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the block builder’s block/WAL encoding resolution so it honors the global storage block version (storage.trace.block.version) instead of hardcoding the default encoding version during flag/default registration. This aligns block builder behavior with the precedence rules introduced for live store in #6509 and addresses #6451.
Changes:
- Injects a
GlobalBlockConfig *common.BlockConfiginto block builder config and uses it as a fallback for encoding version resolution. - Adds
coalesceBlockVersion()to resolve the effective encoding version (default → global storage block config → block builder local block config) and ensures the WAL version follows the resolved block version. - Updates validation and startup to use the coalesced version and adds unit tests covering the precedence and error cases.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| modules/blockbuilder/config.go | Removes hardcoded default version assignment and adds global config injection + coalescing logic used by validation/startup. |
| modules/blockbuilder/blockbuilder.go | Uses coalesceBlockVersion() to determine encoding during service start. |
| modules/blockbuilder/config_test.go | Adds unit tests for version coalescing precedence and invalid version handling. |
| cmd/tempo/app/modules.go | Injects the global storage.trace.block config into the block builder during app startup. |
| CHANGELOG.md | Adds a BUGFIX entry documenting the behavior change. |
Comments suppressed due to low confidence (3)
modules/blockbuilder/config.go:46
- The comments around
GlobalBlockConfigandcoalesceBlockVersionuse ambiguous terms likeblock.versionandblock_config. In this context it’d be clearer to reference the actual config locations (for examplestorage.trace.block.versionandblock_builder.block.version) so it’s obvious which layer wins and what “local” means.
// GlobalBlockConfig is the main storage trace block config (storage.trace.block). Used as fallback
// when block.version is not set. This config is injected by the application when creating the BlockBuilder.
GlobalBlockConfig *common.BlockConfig `yaml:"-"`
modules/blockbuilder/config.go:114
coalesceBlockVersion’s doc comment says it checks “global block config, then block_config”, but the actual precedence is default →GlobalBlockConfig.Version→BlockConfig.BlockCfg.Version(block-builder local). Consider rewording the comment to match the code and the real config keys to avoid confusion during future changes.
// coalesceBlockVersion resolves the block encoding from configs.
// Starts with the default version and overrides as each layer is checked (global block config, then block_config).
// The WAL version always follows the block version.
// Returns an error if the resolved version isn't writable.
modules/blockbuilder/blockbuilder.go:183
starting()now callscoalesceBlockVersion(&b.cfg), butblockbuilder.New()already callscfg.Validate(), which also invokescoalesceBlockVersion. Unless there’s a scenario whereValidate()can be skipped or the version can change between construction and start, this is redundant work and another mutation point for config; consider relying on the validated/coalesced config value here.
b.enc, err = coalesceBlockVersion(&b.cfg)
if err != nil {
return fmt.Errorf("failed to create encoding: %w", err)
Contributor
javiermolinar
left a comment
There was a problem hiding this comment.
Just a minor nit. You need to recreate the manifest again.
| // Starts with the default version and overrides as each layer is checked (global block config, then block_config). | ||
| // The WAL version always follows the block version. | ||
| // Returns an error if the resolved version isn't writable. | ||
| func coalesceBlockVersion(cfg *Config) (encoding.VersionedEncoding, error) { |
Contributor
There was a problem hiding this comment.
Can we extract this into a shared helper? So we can use it in the LiveStore as well keeping module-specific WAL handling
… manifest Extract version resolution into encoding.CoalesceVersion() so both block builder and live store share the same logic. The live store's module-specific WAL override chain is preserved by passing the extra WAL version argument. Regenerate the configuration manifest to reflect the removed hardcoded defaults.
javiermolinar
approved these changes
Feb 24, 2026
Contributor
javiermolinar
left a comment
There was a problem hiding this comment.
LGTM. Thank you for your contribution!
zalegrala
pushed a commit
to zalegrala/tempo
that referenced
this pull request
Feb 27, 2026
…d WAL versions (grafana#6532) * fix: block builder honor the global storage block config for block and WAL versions The block builder was hardcoding the default encoding version in BlockConfig.RegisterFlagsAndApplyDefaults, ignoring the global storage.trace.block.version config. This replicates the approach from grafana#6509 (live store fix): introduce a GlobalBlockConfig field injected by the application, and a coalesceBlockVersion function that resolves the encoding with proper fallback (default -> global -> local block_config). Fixes grafana#6451 * refactor: extract shared CoalesceVersion helper, update LiveStore and manifest Extract version resolution into encoding.CoalesceVersion() so both block builder and live store share the same logic. The live store's module-specific WAL override chain is preserved by passing the extra WAL version argument. Regenerate the configuration manifest to reflect the removed hardcoded defaults.
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.
What this PR does:
The block builder was hardcoding the default encoding version in
BlockConfig.RegisterFlagsAndApplyDefaults, ignoring the globalstorage.trace.block.versionconfig. This replicates the approach from #6509 (live store fix) for the block builder:GlobalBlockConfig *common.BlockConfigtoConfig, injected by the application at startupcoalesceBlockVersion()that resolves the encoding with proper fallback: default → globalstorage.trace.block→ localblock_configencoding.DefaultEncoding().Version()fromBlockConfig.RegisterFlagsAndApplyDefaultsValidate()to usecoalesceBlockVersion()for early startup error on invalid versionWhich issue(s) this PR fixes:
Fixes #6451
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]