Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: remove unused properties from the WAL config file #3911

Merged
merged 5 commits into from
Jul 26, 2024
Merged

fix: remove unused properties from the WAL config file #3911

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f804cff
remove unused properties from wal config
javiermolinar Jul 25, 2024
ddf6b72
changelog
javiermolinar Jul 25, 2024
6b2106b
added PR number to the changelog
javiermolinar Jul 25, 2024
ee5098d
update changelog with breaking change
javiermolinar Jul 26, 2024
fa844cb
Merge branch 'main' into fix-wal-config
javiermolinar Jul 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
* [BUGFIX] Improved handling of complete blocks in localblocks processor after enabling flusing [#3805](https://github.com/grafana/tempo/pull/3805) (@mapno)
* [BUGFIX] Handle out of boundaries spans kinds [#3861](https://github.com/grafana/tempo/pull/3861) (@javiermolinar)
* [BUGFIX] Maintain previous tenant blocklist on tenant errors [#3860](https://github.com/grafana/tempo/pull/3860) (@zalegrala)

* [BUGFIX] Remove unused properties from the WAL configuration [#3911](https://github.com/grafana/tempo/pull/3911) (@javiermolianr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to list this as a breaking change. we do that by typing **Breaking Change** on the changelog line like this:

tempo/CHANGELOG.md

Line 3 in d0f0a5f

* [CHANGE] **BREAKING CHANGE** Remove `autocomplete_filtering_enabled` feature flag [#3729](https://github.com/grafana/tempo/pull/3729) (@mapno)

that will help us find and note the removal of these config optoins for our users when we cut 2.6

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I didn't think this was a breaking change because the configuration was not honored ever :P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point, but we are removing config options that previously were parsed. It might break someone's config I suppose.



## v2.5.0
Expand Down
10 changes: 0 additions & 10 deletions docs/sources/tempo/configuration/_index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1433,16 +1433,6 @@ The storage WAL configuration block.
# Example: "/var/tempo/wal
[path: <string> | default = ""]

# Where to store the completed wal files
# If not set (""), will join the `path` with "completed" to generate the effective path
# Example: "/var/tempo/wal/completed"
[completedfilepath: <string> | default = join(.path, "/completed")]

# Where to store the intermediate blocks while they are being appended to.
# Will always join the `path` with "blocks" to generate the effective path
# Example: "/var/tempo/wal/blocks" (ignored)
[blocksfilepath: <ignored> | = join(.path, "/blocks")]

# WAL encoding/compression.
# options: none, gzip, lz4-64k, lz4-256k, lz4-1M, lz4, snappy, zstd, s2
[v2_encoding: <string> | default = "zstd" ]
Expand Down
4 changes: 0 additions & 4 deletions docs/sources/tempo/configuration/manifest.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,8 +581,6 @@ metrics_generator:
remote_write_add_org_id_header: true
traces_storage:
path: ""
completedfilepath: ""
blocksfilepath: ""
v2_encoding: none
search_encoding: none
ingestion_time_range_slack: 0s
Expand All @@ -597,8 +595,6 @@ storage:
queue_depth: 20000
wal:
path: /var/tempo/wal
completedfilepath: /var/tempo/wal/completed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice. i'm impressed you found and updated the manifest :)

blocksfilepath: /var/tempo/wal/blocks
v2_encoding: snappy
search_encoding: none
ingestion_time_range_slack: 2m0s
Expand Down
32 changes: 8 additions & 24 deletions tempodb/wal/wal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,11 @@ type WAL struct {
}

type Config struct {
Filepath string `yaml:"path"`
CompletedFilepath string
BlocksFilepath string
Encoding backend.Encoding `yaml:"v2_encoding"`
SearchEncoding backend.Encoding `yaml:"search_encoding"`
IngestionSlack time.Duration `yaml:"ingestion_time_range_slack"`
Version string `yaml:"version,omitempty"`
Filepath string `yaml:"path"`
Encoding backend.Encoding `yaml:"v2_encoding"`
SearchEncoding backend.Encoding `yaml:"search_encoding"`
IngestionSlack time.Duration `yaml:"ingestion_time_range_slack"`
Version string `yaml:"version,omitempty"`
}

func ValidateConfig(c *Config) error {
Expand All @@ -53,29 +51,15 @@ func New(c *Config) (*WAL, error) {
return nil, err
}

// The /completed/ folder is now obsolete and no new data is written,
// but it needs to be cleared out one last time for any files left
// from a previous version.
if c.CompletedFilepath == "" {
completedFilepath := filepath.Join(c.Filepath, completedDir)
err = os.RemoveAll(completedFilepath)
if err != nil {
return nil, err
}

c.CompletedFilepath = completedFilepath
}

// Setup local backend in /blocks/
p := filepath.Join(c.Filepath, blocksDir)
err = os.MkdirAll(p, os.ModePerm)
blocksFolderPath := filepath.Join(c.Filepath, blocksDir)
err = os.MkdirAll(blocksFolderPath, os.ModePerm)
if err != nil {
return nil, err
}
c.BlocksFilepath = p

l, err := local.NewBackend(&local.Config{
Path: p,
Path: blocksFolderPath,
})
if err != nil {
return nil, err
Expand Down
27 changes: 1 addition & 26 deletions tempodb/wal/wal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"io"
"math/rand"
"os"
"path"
"path/filepath"
"testing"
"time"
Expand All @@ -29,30 +28,6 @@ import (
"github.com/grafana/tempo/tempodb/encoding/common"
)

const (
testTenantID = "fake"
)

func TestCompletedDirIsRemoved(t *testing.T) {
// Create /completed/testfile and verify it is removed.
tempDir := t.TempDir()

err := os.MkdirAll(path.Join(tempDir, completedDir), os.ModePerm)
require.NoError(t, err, "unexpected error creating completedDir")

_, err = os.Create(path.Join(tempDir, completedDir, "testfile"))
require.NoError(t, err, "unexpected error creating testfile")

_, err = New(&Config{
Filepath: tempDir,
Version: encoding.DefaultEncoding().Version(),
})
require.NoError(t, err, "unexpected error creating temp wal")

_, err = os.Stat(path.Join(tempDir, completedDir))
require.Error(t, err, "completedDir should not exist")
}

func TestAppendBlockStartEnd(t *testing.T) {
for _, e := range encoding.AllEncodings() {
t.Run(e.Version(), func(t *testing.T) {
Expand Down Expand Up @@ -488,7 +463,7 @@ func BenchmarkFindUnknownTraceID(b *testing.B) {
for _, enc := range encoding.AllEncodings() {
version := enc.Version()
b.Run(version, func(b *testing.B) {
runWALBenchmark(b, version, 1, func(ids [][]byte, objs []*tempopb.Trace, block common.WALBlock) {
runWALBenchmark(b, version, 1, func(_ [][]byte, _ []*tempopb.Trace, block common.WALBlock) {
for i := 0; i < b.N; i++ {
_, err := block.FindTraceByID(context.Background(), common.ID{}, common.DefaultSearchOptions())
require.NoError(b, err)
Expand Down
Loading