Build Blockstore stats from config #8814

itaigilo · 2025-03-17T18:25:16Z

Currently, some blockstore-related stats () are constructed in a way which doesn't support more than a single blockstore type.
This extends it to allow multiple types.

This also simplifies NewMetadata params, and the code of its callers.

github-actions · 2025-03-17T18:33:10Z

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions · 2025-03-17T18:33:40Z

E2E Test Results - Quickstart

itaigilo · 2025-03-18T16:54:41Z

pkg/api/controller.go

@@ -112,24 +112,39 @@ type Controller struct {

 var usageCounter = stats.NewUsageCounter()

-func NewController(cfg config.Config, catalog *catalog.Catalog, authenticator auth.Authenticator, authService auth.Service, authenticationService authentication.Service, blockAdapter block.Adapter, metadataManager auth.MetadataManager, migrator Migrator, collector stats.Collector, cloudMetadataProvider cloud.MetadataProvider, actions actionsHandler, auditChecker AuditChecker, logger logging.Logger, sessionStore sessions.Store, pathProvider upload.PathProvider, usageReporter stats.UsageReporterOperations) *Controller {
+func NewController(


It was too damn long for a single line.
Removed cloudMetadataProvider.

itaigilo · 2025-03-18T16:56:07Z

pkg/api/controller.go

-		MetadataManager:       metadataManager,
-		Migrator:              migrator,
-		Collector:             collector,
-		CloudMetadataProvider: cloudMetadataProvider,


Only removed this one. The rest is whitespaces.

itaigilo · 2025-03-18T16:57:33Z

pkg/api/serve.go

@@ -33,7 +32,24 @@ const (
 	extensionValidationExcludeBody = "x-validation-exclude-body"
 )

-func Serve(cfg config.Config, catalog *catalog.Catalog, middlewareAuthenticator auth.Authenticator, authService auth.Service, authenticationService authentication.Service, blockAdapter block.Adapter, metadataManager auth.MetadataManager, migrator Migrator, collector stats.Collector, cloudMetadataProvider cloud.MetadataProvider, actions actionsHandler, auditChecker AuditChecker, logger logging.Logger, gatewayDomains []string, snippets []params.CodeSnippet, pathProvider upload.PathProvider, usageReporter stats.UsageReporterOperations) http.Handler {
+func Serve(


It was too damn long for a single line.
Removed cloudMetadataProvider, and renamed middlewareAuthenticator to authenticator (not related, but simplifies).

Can you please provide the style template you are using, if we change to something like this I need to break it down manually on every refactor.

Adding this to the list of topics for the Code-Style effort -
So this will be done systematically.

guy-har

Looks good, my main concern is inserting all the storage adapter logic into the stats library

guy-har · 2025-03-20T08:28:48Z

pkg/stats/metadata.go

@@ -27,7 +27,7 @@ type MetadataProvider interface {
 	GetMetadata(context.Context) (map[string]string, error)
 }

-func NewMetadata(ctx context.Context, logger logging.Logger, blockstoreType string, metadataProvider MetadataProvider, cloudMetadataProvider cloud.MetadataProvider) *Metadata {
+func NewMetadata(ctx context.Context, logger logging.Logger, metadataProvider MetadataProvider, storageConfig config.StorageConfig) *Metadata {


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.
func NewMetadata(ctx context.Context, logger logging.Logger, blockstoreType string, metadataProvider []MetadataProvider) *Metadata

The thing is -
blockstoreType should also be an array.
So it'll be a bunch of repeated code outside of this function.

Note that BuildMetadataProvider in this file was also "aware" of blockstore logic -
It wasn't part of the NewMetadata() function, but each NewMetadata() caller was calling this function to pas its result as a param.
So I don't think that using the same trick gives much better encapsulation, only more repeated code...

I'm not following, why should blockstore type be an array?
I know, it was done in order to not have circular dependency and not to build a factory.

@guy-har I tried what you suggested, which is basically moving buildMetadataProvider to the blockfactory, so it can be implemented for Enterprise as well.
However, as you suspected, this creates a circular dependency -
Since blockfactory is using the stats package when building the block.Adapter.
This is probably why buildMetadataProvider is in this file in first place.

We can extract the metadata builder into a separate module (in modules) that will be implement by Enterprise, and I think this can solve the circular dependencies (though it requires validation) - but I think that's a huge overkill for this.

Any other suggestions?

guy-har · 2025-03-20T09:20:27Z

pkg/stats/metadata.go

+func getErrorMetadata() map[string]string {
+	return map[string]string{
+		cloud.IDKey:     "err",
+		cloud.IDTypeKey: "err",
+	}
+}
+


What's ErrorMetadata? Is it stats we want to collect?

In some cases we know the blockstore type, but failing to get the metadata from the cloud-provider.
In this case, we'll report err to the BI (instead of nil).

Was this requested?

Not as a product requirement, but from a functional pov -
You can check the tests here, and see that once it's nil and not err, the BI consumer will think it's not configured, instead of this being configured but failing.

I can return nil here, but this also means we won't be able to unit-test these params.

guy-har · 2025-03-20T09:27:01Z

pkg/api/serve.go

@@ -33,7 +32,24 @@ const (
 	extensionValidationExcludeBody = "x-validation-exclude-body"
 )

-func Serve(cfg config.Config, catalog *catalog.Catalog, middlewareAuthenticator auth.Authenticator, authService auth.Service, authenticationService authentication.Service, blockAdapter block.Adapter, metadataManager auth.MetadataManager, migrator Migrator, collector stats.Collector, cloudMetadataProvider cloud.MetadataProvider, actions actionsHandler, auditChecker AuditChecker, logger logging.Logger, gatewayDomains []string, snippets []params.CodeSnippet, pathProvider upload.PathProvider, usageReporter stats.UsageReporterOperations) http.Handler {
+func Serve(


Can you please provide the style template you are using, if we change to something like this I need to break it down manually on every refactor.

itaigilo

@guy-har thanks for your review.

Addressed your comments 🙏

itaigilo · 2025-03-20T10:11:35Z

pkg/stats/metadata.go

@@ -27,7 +27,7 @@ type MetadataProvider interface {
 	GetMetadata(context.Context) (map[string]string, error)
 }

-func NewMetadata(ctx context.Context, logger logging.Logger, blockstoreType string, metadataProvider MetadataProvider, cloudMetadataProvider cloud.MetadataProvider) *Metadata {
+func NewMetadata(ctx context.Context, logger logging.Logger, metadataProvider MetadataProvider, storageConfig config.StorageConfig) *Metadata {


The thing is -
blockstoreType should also be an array.
So it'll be a bunch of repeated code outside of this function.

Note that BuildMetadataProvider in this file was also "aware" of blockstore logic -
It wasn't part of the NewMetadata() function, but each NewMetadata() caller was calling this function to pas its result as a param.
So I don't think that using the same trick gives much better encapsulation, only more repeated code...

itaigilo · 2025-03-20T10:14:03Z

pkg/stats/metadata.go

+func getErrorMetadata() map[string]string {
+	return map[string]string{
+		cloud.IDKey:     "err",
+		cloud.IDTypeKey: "err",
+	}
+}
+


In some cases we know the blockstore type, but failing to get the metadata from the cloud-provider.
In this case, we'll report err to the BI (instead of nil).

guy-har

I'm sorry, but I wasn't convinced
We can take this offline if you prefer

nopcoder

Seems we embed the cloud metadata into stats. I understand that the factory was already there (which I think we can move it out) but it looks like the author use an interface wanted to make the cloud metadata provided the same way stats metadata are provided.

The stats package should provide a abstract way to construct and invoke the metadata providers - for the setup process or the process startup while trying to keep the cloud metadata outside of the stats package.

itaigilo · 2025-03-22T18:43:31Z

Seems we embed the cloud metadata into stats. I understand that the factory was already there (which I think we can move it out) but it looks like the author use an interface wanted to make the cloud metadata provided the same way stats metadata are provided.

The stats package should provide a abstract way to construct and invoke the metadata providers - for the setup process or the process startup while trying to keep the cloud metadata outside of the stats package.

@nopcoder can you please elaborate?

There's already an abstract way for metadata providers, which is MetadataProvider.
And it would be great if we'd have a builder (module) which the Enterprise could implement.
However, as I wrote to @guy-har 's comment, if placed in the block factory module (which is the reasonable place for it imho), it creates a circular dependency. And having a separate module seems to me like an overkill.

Which solution do you have in mind?

nopcoder · 2025-03-22T20:24:28Z

Which solution do you have in mind?
@itaigilo @guy-har

Sorry, I didn't go over the comments first.
I was looking into refactoring in order to provide specific storage for the cloud metadata and found that it may not have any benefit.

Explain the real issue found in the metadata provider implementation, based on the current code (except aws which I'm not 100% sure it is the case) the code try to identify information about the cloud environment lakeFS is running on :)

Seems the assumption that the default blockstore adapter will match the cloud provider and the cloud metadata will provide more information.
So, I'm blocking this one because we need to rethink how we plan to address this one.
The option I suggest is to perform cloud discovery and consider sending the information without re-collecting on "init" (setup flow)

itaigilo · 2025-03-23T23:13:02Z

So, I'm blocking this one because we need to rethink how we plan to address this one. The option I suggest is to perform cloud discovery and consider sending the information without re-collecting on "init" (setup flow)

@nopcoder If I get it right, you're blocking this because you think that the design of the existing code is problematic (which I agree), and that this PR adds to the problem instead of solving it (which I also agree).

However, refactoring the stats code will take a decent amount time (since there is no suggested solution yet), and will introduce risks of messing up the important BI data (considering the fact that this area is not covered by tests at all).

So if that's the case, I think we should take a different approach. Since MSB data isn't being reported at the moment, I suggest to first fix the Issue itself (using the non-optimal solution in this PR), and prioritize the refactoring using our normal process (according to the usual needs + risk + availability etc.).

All this, of course, will be void if someone can come up with a nice solution for this with as low risk as possible for the stats that are currently being sent to the BI.

nopcoder · 2025-03-24T00:35:56Z

I want to add that I have a suggestion, in a untested pr format, related to the cloud bi information that will check all cloud providers.

also related to the block adapter itself I think it is a different case, not related to the cloud that we can discuss with bi.

Refactor NewMetadata

868c0d5

itaigilo added BI exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached msb labels Mar 17, 2025

itaigilo added 2 commits March 18, 2025 16:41

Improve stats funcs

85bc259

Update tests

7f88f21

itaigilo commented Mar 18, 2025

View reviewed changes

itaigilo added 3 commits March 18, 2025 16:58

Remove commented code

2937e4d

Cleanup

0a6b831

Cleanup

d7b2381

itaigilo marked this pull request as ready for review March 18, 2025 17:20

itaigilo requested review from a team and nopcoder March 19, 2025 08:20

guy-har requested changes Mar 20, 2025

View reviewed changes

itaigilo commented Mar 20, 2025

View reviewed changes

itaigilo requested a review from guy-har March 20, 2025 10:14

guy-har requested changes Mar 20, 2025

View reviewed changes

itaigilo requested a review from guy-har March 21, 2025 06:48

nopcoder requested changes Mar 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build Blockstore stats from config #8814

Build Blockstore stats from config #8814

itaigilo commented Mar 17, 2025

github-actions bot commented Mar 17, 2025 •

edited

Loading

github-actions bot commented Mar 17, 2025 •

edited

Loading

itaigilo Mar 18, 2025 •

edited

Loading

itaigilo Mar 18, 2025

itaigilo Mar 18, 2025

guy-har Mar 20, 2025

itaigilo Mar 21, 2025

guy-har left a comment

guy-har Mar 20, 2025 •

edited

Loading

itaigilo Mar 20, 2025

guy-har Mar 20, 2025

itaigilo Mar 21, 2025

guy-har Mar 20, 2025

itaigilo Mar 20, 2025

guy-har Mar 20, 2025

itaigilo Mar 21, 2025

guy-har Mar 20, 2025

itaigilo left a comment

itaigilo Mar 20, 2025

itaigilo Mar 20, 2025

guy-har left a comment

nopcoder left a comment

itaigilo commented Mar 22, 2025

nopcoder commented Mar 22, 2025

itaigilo commented Mar 23, 2025

nopcoder commented Mar 24, 2025

Build Blockstore stats from config #8814

Are you sure you want to change the base?

Build Blockstore stats from config #8814

Conversation

itaigilo commented Mar 17, 2025

github-actions bot commented Mar 17, 2025 • edited Loading

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions bot commented Mar 17, 2025 • edited Loading

E2E Test Results - Quickstart

itaigilo Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guy-har left a comment

Choose a reason for hiding this comment

guy-har Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaigilo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guy-har left a comment

Choose a reason for hiding this comment

nopcoder left a comment

Choose a reason for hiding this comment

itaigilo commented Mar 22, 2025

nopcoder commented Mar 22, 2025

itaigilo commented Mar 23, 2025

nopcoder commented Mar 24, 2025

github-actions bot commented Mar 17, 2025 •

edited

Loading

github-actions bot commented Mar 17, 2025 •

edited

Loading

itaigilo Mar 18, 2025 •

edited

Loading

guy-har Mar 20, 2025 •

edited

Loading