refactor: Mark storage.prefix as non experimental

This PR migrates from the experimental flag `storage.storage-prefix` to non experimental `storage.prefix` flag.

This is on the back of a user that was not able to find this flag and
reaching out in the community slack.

Author: simonswine

cmd/profilecli/bucket.go

@@ -80,7 +80,7 @@ func newBucketWebTool(params *bucketWebToolParams) *bucketWebTool {
 
 func initObjectStoreBucket(params *bucketWebToolParams) (phlareobj.Bucket, error) {
 	objectStoreConfig := objstoreclient.Config{
-		StoragePrefix: "",
+		Prefix: "",
 		StorageBackendConfig: objstoreclient.StorageBackendConfig{
 			Backend: params.objectStoreType,
 			GCS: gcs.Config{

cmd/profilecli/compact.go

@@ -66,7 +66,7 @@ func compact(ctx context.Context, src, dst string, metas []*block.Meta, shards i
 				Directory: src,
 			},
 		},
-		StoragePrefix: "",
+		Prefix: "",
 	}, "profilecli")
 	if err != nil {
 		return err

cmd/profilecli/query-blocks.go

@@ -178,6 +178,6 @@ func getRemoteBucket(ctx context.Context, params *blocksQueryParams) (objstore.B
 				BucketName: params.BucketName,
 			},
 		},
-		StoragePrefix: fmt.Sprintf("%s/phlaredb", params.TenantID),
+		Prefix: fmt.Sprintf("%s/phlaredb", params.TenantID),
 	}, params.BucketName)
 }

cmd/pyroscope/help-all.txt.tmpl

@@ -917,6 +917,8 @@ Usage of ./pyroscope:
     	JSON either from a Google Developers Console client_credentials.json file, or a Google Developers service account key. Needs to be valid JSON, not a filesystem path.
   -storage.gcs.tls-handshake-timeout duration
     	Maximum time to wait for a TLS handshake. 0 means no limit. (default 10s)
+  -storage.prefix string
+    	Prefix for all objects stored in the backend storage. For simplicity, it may only contain digits and English alphabet characters, hyphens, underscores, dots and forward slashes.
   -storage.s3.access-key-id string
     	S3 access key ID
   -storage.s3.bucket-lookup-type string
@@ -958,7 +960,7 @@ Usage of ./pyroscope:
   -storage.s3.tls-handshake-timeout duration
     	Maximum time to wait for a TLS handshake. 0 means no limit. (default 10s)
   -storage.storage-prefix string
-    	[experimental] Prefix for all objects stored in the backend storage. For simplicity, it may only contain digits and English alphabet letters.
+    	[experimental] Deprecated: Use 'storage..prefix' instead. Prefix for all objects stored in the backend storage. For simplicity, it may only contain digits and English alphabet characters, hyphens, underscores, dots and forward slashes.
   -storage.swift.auth-url string
     	OpenStack Swift authentication URL
   -storage.swift.auth-version int

cmd/pyroscope/help.txt.tmpl

@@ -335,6 +335,8 @@ Usage of ./pyroscope:
     	JSON either from a Google Developers Console client_credentials.json file, or a Google Developers service account key. Needs to be valid JSON, not a filesystem path.
   -storage.gcs.tls-handshake-timeout duration
     	Maximum time to wait for a TLS handshake. 0 means no limit. (default 10s)
+  -storage.prefix string
+    	Prefix for all objects stored in the backend storage. For simplicity, it may only contain digits and English alphabet characters, hyphens, underscores, dots and forward slashes.
   -storage.s3.access-key-id string
     	S3 access key ID
   -storage.s3.bucket-name string

pkg/objstore/client/config.go

@@ -6,6 +6,8 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/go-kit/log"
+	"github.com/go-kit/log/level"
 	"github.com/samber/lo"
 	"github.com/thanos-io/objstore"
 
@@ -47,6 +49,7 @@ var (
 	ErrStoragePrefixStartsWithSlash   = errors.New("storage prefix starts with a slash")
 	ErrStoragePrefixEmptyPathSegment  = errors.New("storage prefix contains an empty path segment")
 	ErrStoragePrefixInvalidCharacters = errors.New("storage prefix contains invalid characters: only alphanumeric, hyphen, underscore, dot, and no segement should be . or ..")
+	ErrStoragePrefixBothFlagsSet      = errors.New("both storage.prefix and storage.storage-prefix are set, please use only storage.prefix, as storage.storage-prefix is deprecated.")
 )
 
 type ErrInvalidCharactersInStoragePrefix struct {
@@ -116,7 +119,8 @@ func (cfg *StorageBackendConfig) Validate() error {
 type Config struct {
 	StorageBackendConfig `yaml:",inline"`
 
-	StoragePrefix string `yaml:"storage_prefix" category:"experimental"`
+	Prefix                  string `yaml:"prefix"`
+	DeprecatedStoragePrefix string `yaml:"storage_prefix" category:"experimental"` // Deprecated: use Prefix instead
 
 	// Not used internally, meant to allow callers to wrap Buckets
 	// created using this config
@@ -130,7 +134,8 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 
 func (cfg *Config) RegisterFlagsWithPrefixAndDefaultDirectory(prefix, dir string, f *flag.FlagSet) {
 	cfg.StorageBackendConfig.RegisterFlagsWithPrefixAndDefaultDirectory(prefix, dir, f)
-	f.StringVar(&cfg.StoragePrefix, prefix+"storage-prefix", "", "Prefix for all objects stored in the backend storage. For simplicity, it may only contain digits and English alphabet letters.")
+	f.StringVar(&cfg.Prefix, prefix+"prefix", "", "Prefix for all objects stored in the backend storage. For simplicity, it may only contain digits and English alphabet characters, hyphens, underscores, dots and forward slashes.")
+	f.StringVar(&cfg.DeprecatedStoragePrefix, prefix+"storage-prefix", "", "Deprecated: Use '"+prefix+".prefix' instead. Prefix for all objects stored in the backend storage. For simplicity, it may only contain digits and English alphabet characters, hyphens, underscores, dots and forward slashes.")
 }
 
 func (cfg *Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
@@ -176,8 +181,22 @@ func validStoragePrefix(prefix string) error {
 	return nil
 }
 
-func (cfg *Config) Validate() error {
-	if err := validStoragePrefix(cfg.StoragePrefix); err != nil {
+func (cfg *Config) getPrefix() string {
+	if cfg.Prefix != "" {
+		return cfg.Prefix
+	}
+
+	return cfg.DeprecatedStoragePrefix
+}
+
+func (cfg *Config) Validate(logger log.Logger) error {
+	if cfg.DeprecatedStoragePrefix != "" {
+		if cfg.Prefix != "" {
+			return ErrStoragePrefixBothFlagsSet
+		}
+		level.Warn(logger).Log("msg", "bucket config has a deprecated storage.storage-prefix flag set. Please, use storage.prefix instead.")
+	}
+	if err := validStoragePrefix(cfg.getPrefix()); err != nil {
 		return err
 	}
 

pkg/objstore/client/factory.go

@@ -24,6 +24,7 @@ func NewBucket(ctx context.Context, cfg Config, name string) (phlareobj.Bucket,
 	)
 	logger := phlarecontext.Logger(ctx)
 	reg := phlarecontext.Registry(ctx)
+	prefixPath := cfg.getPrefix()
 
 	switch cfg.Backend {
 	case S3:
@@ -52,10 +53,10 @@ func NewBucket(ctx context.Context, cfg Config, name string) (phlareobj.Bucket,
 		if err != nil {
 			return nil, err
 		}
-		if cfg.StoragePrefix == "" {
+		if prefixPath == "" {
 			return fs, nil
 		}
-		return phlareobj.NewPrefixedBucket(fs, cfg.StoragePrefix), nil
+		return phlareobj.NewPrefixedBucket(fs, prefixPath), nil
 	default:
 		return nil, ErrUnsupportedStorageBackend
 	}
@@ -73,8 +74,8 @@ func NewBucket(ctx context.Context, cfg Config, name string) (phlareobj.Bucket,
 	}
 	bkt := phlareobj.NewBucket(objtracing.WrapWithTraces(objstore.WrapWithMetrics(backendClient, reg, name)))
 
-	if cfg.StoragePrefix != "" {
-		bkt = phlareobj.NewPrefixedBucket(bkt, cfg.StoragePrefix)
+	if prefixPath != "" {
+		bkt = phlareobj.NewPrefixedBucket(bkt, prefixPath)
 	}
 	return bkt, nil
 }

pkg/objstore/client/factory_test.go

@@ -12,6 +12,7 @@ import (
 	"path"
 	"testing"
 
+	"github.com/go-kit/log"
 	"github.com/grafana/dskit/flagext"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -106,70 +107,98 @@ func TestClient_ConfigValidation(t *testing.T) {
 		name          string
 		cfg           Config
 		expectedError error
+		expectedLog   string
 	}{
 		{
-			name: "storage_prefix/valid",
-			cfg:  Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "helloWORLD123"},
+			name: "prefix/valid",
+			cfg:  Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, Prefix: "helloWORLD123"},
 		},
 		{
-			name: "storage_prefix/valid-subdir",
-			cfg:  Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "hello/world/env"},
+			name: "prefix/valid-subdir",
+			cfg:  Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, Prefix: "hello/world/env"},
 		},
 		{
-			name: "storage_prefix/valid-subdir-trailing-slash",
-			cfg:  Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "hello/world/env/"},
+			name: "prefix/valid-subdir-trailing-slash",
+			cfg:  Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, Prefix: "hello/world/env/"},
 		},
 		{
-			name:          "storage_prefix/invalid-directory-up",
-			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: ".."},
+			name:          "prefix/invalid-directory-up",
+			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, Prefix: ".."},
 			expectedError: ErrStoragePrefixInvalidCharacters,
 		},
 		{
-			name:          "storage_prefix/invalid-directory",
-			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "."},
+			name:          "prefix/invalid-directory",
+			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, Prefix: "."},
 			expectedError: ErrStoragePrefixInvalidCharacters,
 		},
 		{
-			name:          "storage_prefix/invalid-absolute-path",
-			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "/hello/world"},
+			name:          "prefix/invalid-absolute-path",
+			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, Prefix: "/hello/world"},
 			expectedError: ErrStoragePrefixStartsWithSlash,
 		},
 		{
-			name:          "storage_prefix/invalid-..-in-a-path-segement",
-			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "hello/../test"},
+			name:          "prefix/invalid-..-in-a-path-segement",
+			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, Prefix: "hello/../test"},
 			expectedError: ErrStoragePrefixInvalidCharacters,
 		},
 		{
-			name:          "storage_prefix/invalid-empty-path-segement",
-			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "hello//test"},
+			name:          "prefix/invalid-empty-path-segement",
+			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, Prefix: "hello//test"},
 			expectedError: ErrStoragePrefixEmptyPathSegment,
 		},
 		{
-			name:          "storage_prefix/invalid-emoji",
-			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "👋"},
+			name:          "prefix/invalid-emoji",
+			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, Prefix: "👋"},
 			expectedError: ErrStoragePrefixInvalidCharacters,
 		},
 		{
-			name:          "storage_prefix/invalid-emoji",
-			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "hello!world"},
+			name:          "prefix/invalid-emoji",
+			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, Prefix: "hello!world"},
 			expectedError: ErrStoragePrefixInvalidCharacters,
 		},
 		{
 			name:          "unsupported backend",
 			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: "flash drive"}},
 			expectedError: ErrUnsupportedStorageBackend,
 		},
+		{
+			name:        "prefix/valid-legacy-subdir-trailing-slash",
+			cfg:         Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, DeprecatedStoragePrefix: "hello/world/env/"},
+			expectedLog: "config has a deprecated storage.storage-prefix flag set",
+		},
+		{
+			name:          "prefix/invalid-emoji",
+			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, DeprecatedStoragePrefix: "hello!world"},
+			expectedError: ErrStoragePrefixInvalidCharacters,
+			expectedLog:   "config has a deprecated storage.storage-prefix flag set",
+		},
+		{
+			name:          "prefix/invalid-both-configs",
+			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, DeprecatedStoragePrefix: "hello-world1", Prefix: "hello-world2"},
+			expectedError: ErrStoragePrefixBothFlagsSet,
+		},
 	}
 
+	logBuf := new(bytes.Buffer)
+	logger := log.NewLogfmtLogger(logBuf)
+
 	for _, tc := range testCases {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
-			actualErr := tc.cfg.Validate()
+			// Reset log buffer
+			logBuf.Reset()
+
+			actualErr := tc.cfg.Validate(logger)
 			if tc.expectedError != nil {
 				assert.Equal(t, actualErr, tc.expectedError)
 			} else {
 				assert.NoError(t, actualErr)
 			}
+			if tc.expectedLog != "" {
+				assert.Contains(t, logBuf.String(), tc.expectedLog)
+			} else {
+				assert.Empty(t, logBuf.String())
+			}
 		})
 	}
 }
@@ -185,7 +214,7 @@ func TestNewPrefixedBucketClient(t *testing.T) {
 					Directory: tempDir,
 				},
 			},
-			StoragePrefix: "prefix",
+			Prefix: "prefix",
 		}
 
 		client, err := NewBucket(ctx, cfg, "test")

pkg/objstore/reader_test.go

@@ -21,7 +21,7 @@ func Test_FileSystem(t *testing.T) {
 				Directory: testDir,
 			},
 		},
-		StoragePrefix: "testdata/",
+		Prefix: "testdata/",
 	}, "foo")
 	require.NoError(t, err)
 

pkg/phlare/phlare.go

@@ -300,7 +300,7 @@ func (c *Config) Validate() error {
 		return err
 	}
 
-	if err := c.Storage.Bucket.Validate(); err != nil {
+	if err := c.Storage.Bucket.Validate(util.Logger); err != nil {
 		return err
 	}