Build Blockstore stats from config #8814
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,7 @@ type MetadataProvider interface { | |
| GetMetadata(context.Context) (map[string]string, error) | ||
| } | ||
|
|
||
| func NewMetadata(ctx context.Context, logger logging.Logger, metadataProvider MetadataProvider, storageConfig config.StorageConfig) *Metadata { | ||
|
There was a problem hiding this comment. The internals of the provider should stay externally, I would expect all this code to be in a storageAdapterMetadata provider. Maybe even provided by the block adapter factory. If we're already here I would suggest replacing the current signature of this function. There was a problem hiding this comment. The thing is - Note that There was a problem hiding this comment. I'm not following, why should blockstore type be an array? There was a problem hiding this comment. @guy-har I tried what you suggested, which is basically moving We can extract the metadata builder into a separate module (in Any other suggestions? |
||
| res := &Metadata{} | ||
| authMetadata, err := metadataProvider.GetMetadata(ctx) | ||
| if err != nil { | ||
|
|
@@ -39,28 +39,57 @@ func NewMetadata(ctx context.Context, logger logging.Logger, blockstoreType stri | |
| } | ||
| res.Entries = append(res.Entries, MetadataEntry{Name: k, Value: v}) | ||
| } | ||
|
|
||
| for _, id := range storageConfig.GetStorageIDs() { | ||
| if adapterCfg := storageConfig.GetStorageByID(id); adapterCfg != nil { | ||
| appendOrUpdateEntry(res, BlockstoreTypeKey, adapterCfg.BlockstoreType()) | ||
|
|
||
| provider := buildMetadataProvider(logger, adapterCfg) | ||
| cloudMetadata := provider.GetMetadata() | ||
| if cloudMetadata == nil { | ||
| cloudMetadata = getErrorMetadata() | ||
| } | ||
| for k, v := range cloudMetadata { | ||
| appendOrUpdateEntry(res, k, v) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return res | ||
| } | ||
|
|
||
| func appendOrUpdateEntry(metadata *Metadata, key, value string) { | ||
| for i, entry := range metadata.Entries { | ||
| if entry.Name == key { | ||
| metadata.Entries[i].Value = entry.Value + "," + value | ||
| return | ||
| } | ||
| } | ||
| metadata.Entries = append(metadata.Entries, MetadataEntry{Name: key, Value: value}) | ||
| } | ||
|
|
||
| func getErrorMetadata() map[string]string { | ||
| return map[string]string{ | ||
| cloud.IDKey: "err", | ||
| cloud.IDTypeKey: "err", | ||
| } | ||
| } | ||
|
|
||
|
Comment on lines
+71
to
+77
There was a problem hiding this comment. What's ErrorMetadata? Is it stats we want to collect? There was a problem hiding this comment. In some cases we know the blockstore type, but failing to get the metadata from the cloud-provider. There was a problem hiding this comment. Not as a product requirement, but from a functional pov - I can return |
||
| type noopMetadataProvider struct{} | ||
|
|
||
| func (n *noopMetadataProvider) GetMetadata() map[string]string { | ||
| return map[string]string{ | ||
| cloud.IDKey: "nil", | ||
| cloud.IDTypeKey: "nil", | ||
| } | ||
| } | ||
|
|
||
| func buildMetadataProvider(logger logging.Logger, c config.AdapterConfig) cloud.MetadataProvider { | ||
| switch c.BlockstoreType() { | ||
| case block.BlockstoreTypeGS: | ||
| return gcp.NewMetadataProvider(logger) | ||
| case block.BlockstoreTypeS3: | ||
| s3Params, err := c.BlockstoreS3Params() | ||
| if err != nil { | ||
| logger.WithError(err).Warn("Failed to create S3 client for MetadataProvider") | ||
| return &noopMetadataProvider{} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| package stats_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| "github.com/treeverse/lakefs/pkg/cloud" | ||
| "github.com/treeverse/lakefs/pkg/config" | ||
| "github.com/treeverse/lakefs/pkg/logging" | ||
| "github.com/treeverse/lakefs/pkg/stats" | ||
| ) | ||
|
|
||
| type mockMetadataProvider struct { | ||
| metadata map[string]string | ||
| err error | ||
| } | ||
|
|
||
| func (m *mockMetadataProvider) GetMetadata(ctx context.Context) (map[string]string, error) { | ||
| return m.metadata, m.err | ||
| } | ||
|
|
||
| const ( | ||
| installationIDKey = "installation_id" | ||
| installationIDValue = "test_installation_id" | ||
| testEntryKey = "test_key" | ||
| testEntryValue = "test_value" | ||
| ) | ||
|
|
||
| func TestNewMetadata(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| blockstore config.Blockstore | ||
| expectedBlockstoreType string | ||
| expectedIDKeyType string | ||
| expectedIDKey string | ||
| }{ | ||
| { | ||
| name: "s3", | ||
| blockstore: config.Blockstore{ | ||
| Type: "s3", | ||
| S3: &config.BlockstoreS3{}, | ||
| }, | ||
| expectedBlockstoreType: "s3", | ||
| expectedIDKeyType: "err", // no s3 cloud provider in the test env, hence err | ||
| expectedIDKey: "err", // no s3 cloud provider in the test env, hence err | ||
| }, | ||
| { | ||
| name: "gs", | ||
| blockstore: config.Blockstore{ | ||
| Type: "gs", | ||
| GS: &config.BlockstoreGS{}, | ||
| }, | ||
| expectedBlockstoreType: "gs", | ||
| expectedIDKeyType: "err", // no gs cloud provider in the test env, hence err | ||
| expectedIDKey: "err", // no gs cloud provider in the test env, hence err | ||
| }, | ||
| { | ||
| name: "local", | ||
| blockstore: config.Blockstore{ | ||
| Type: "local", | ||
| }, | ||
| expectedBlockstoreType: "local", | ||
| expectedIDKeyType: "nil", | ||
| expectedIDKey: "nil", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| ctx := context.Background() | ||
| logger := logging.FromContext(ctx) | ||
| provider := &mockMetadataProvider{ | ||
| metadata: map[string]string{ | ||
| installationIDKey: installationIDValue, | ||
| testEntryKey: testEntryValue, | ||
| }, | ||
| } | ||
|
|
||
| t.Run(tt.name, func(t *testing.T) { | ||
| cfg := &config.BaseConfig{ | ||
| Blockstore: tt.blockstore, | ||
| } | ||
|
|
||
| metadata := stats.NewMetadata(ctx, logger, provider, cfg.StorageConfig()) | ||
|
|
||
| require.Equal(t, installationIDValue, metadata.InstallationID) | ||
| require.Len(t, metadata.Entries, 5) | ||
| require.Contains(t, metadata.Entries, stats.MetadataEntry{Name: installationIDKey, Value: installationIDValue}) | ||
| require.Contains(t, metadata.Entries, stats.MetadataEntry{Name: testEntryKey, Value: testEntryValue}) | ||
| require.Contains(t, metadata.Entries, stats.MetadataEntry{Name: stats.BlockstoreTypeKey, Value: tt.expectedBlockstoreType}) | ||
| require.Contains(t, metadata.Entries, stats.MetadataEntry{Name: cloud.IDTypeKey, Value: tt.expectedIDKeyType}) | ||
| require.Contains(t, metadata.Entries, stats.MetadataEntry{Name: cloud.IDKey, Value: tt.expectedIDKey}) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was too damn long for a single line.
Removed
cloudMetadataProvider.