TrueCloudLab/frostfs-s3-gw
15
2
Fork 17
Code Issues 22 Pull requests 5 Projects Releases 15 Packages Activity Actions

[#321] Use correct owner id in billing metrics #322

Merged
alexvanin merged 1 commit from mbiryukova/frostfs-s3-gw:bugfix/anon_user_in_metrics into master 2024-02-28 14:27:03 +00:00
Conversation 1 Commits 1 Files changed 11 +161 -32
mbiryukova commented 2024-02-26 13:22:04 +00:00
Member
Copy link

Closes #321

Closes #321
mbiryukova self-assigned this 2024-02-26 13:22:04 +00:00
mbiryukova requested review from storage-services-committers 2024-02-26 13:29:01 +00:00
mbiryukova requested review from storage-services-developers 2024-02-26 13:29:01 +00:00
dkirillov commented 2024-02-27 06:21:44 +00:00
Member
Copy link

Can we add some tests? (see tests where middlewares are invoked https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/branch/master/api/router_test.go)

Can we add some tests? (see tests where middlewares are invoked https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/branch/master/api/router_test.go)
mbiryukova force-pushed bugfix/anon_user_in_metrics from 3137a70e92 to 42b1014308 2024-02-27 09:07:41 +00:00 Compare
mbiryukova force-pushed bugfix/anon_user_in_metrics from 42b1014308 to e268ae19f8 2024-02-27 09:16:58 +00:00 Compare
dkirillov approved these changes 2024-02-27 12:07:44 +00:00
dkirillov left a comment
Member
Copy link

Please, update CHANGELOG.md

Please, update CHANGELOG.md
api/router_mock_test.go Outdated
@ -16,1 +29,4 @@
}
type centerMock struct {
t *testing.T
dkirillov commented 2024-02-27 11:36:35 +00:00
Member
Copy link

Let's add flag that determine anon/not anon behavior. Also this flag should be available from test to have opportunity toggle behavior with the same router.

diff --git a/api/router_mock_test.go b/api/router_mock_test.go
index c23b7185..d7c47e61 100644
--- a/api/router_mock_test.go
+++ b/api/router_mock_test.go
@@ -9,6 +9,7 @@ import (
        "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/data"
        "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/middleware"
        "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/creds/accessbox"
+       "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/bearer"
        bearertest "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/bearer/test"
        "github.com/nspcc-dev/neo-go/pkg/crypto/keys"
        "github.com/nspcc-dev/neo-go/pkg/util"
@@ -29,18 +30,25 @@ func (c *anonCenterMock) Authenticate(*http.Request) (*middleware.Box, error) {
 }
 
 type centerMock struct {
-       t *testing.T
+       t    *testing.T
+       anon bool
 }
 
 func (c *centerMock) Authenticate(*http.Request) (*middleware.Box, error) {
-       token := bearertest.Token()
-       key, err := keys.NewPrivateKey()
-       require.NoError(c.t, err)
-       require.NoError(c.t, token.Sign(key.PrivateKey))
+       var token *bearer.Token
+
+       if !c.anon {
+               bt := bearertest.Token()
+               token = &bt
+               key, err := keys.NewPrivateKey()
+               require.NoError(c.t, err)
+               require.NoError(c.t, token.Sign(key.PrivateKey))
+       }
+
        return &middleware.Box{
                AccessBox: &accessbox.Box{
                        Gate: &accessbox.GateData{
-                               BearerToken: &token,
+                               BearerToken: token,
                        },
                },
        }, nil
diff --git a/api/router_test.go b/api/router_test.go
index a69ab7ba..50f0d88f 100644
--- a/api/router_test.go
+++ b/api/router_test.go
@@ -36,7 +36,7 @@ func (m *routerMock) ServeHTTP(w http.ResponseWriter, r *http.Request) {
        m.router.ServeHTTP(w, r)
 }
 
-func prepareRouter(t *testing.T, center s3middleware.Center) *routerMock {
+func prepareRouter(t *testing.T) *routerMock {
        middlewareSettings := &middlewareSettingsMock{}
        policyChecker := inmemory.NewInMemoryLocalOverrides()
 
@@ -46,7 +46,7 @@ func prepareRouter(t *testing.T, center s3middleware.Center) *routerMock {
                        BacklogTimeout: 30 * time.Second,
                },
                Handler:            &handlerMock{t: t},
-               Center:             center,
+               Center:             &centerMock{t: t},
                Log:                zaptest.NewLogger(t),
                Metrics:            &metrics.AppMetrics{},
                MiddlewareSettings: middlewareSettings,

in TestOwnerIDRetrieving we can toggle behavior using chiRouter.cfg.Center.(*centerMock).anon = true

Let's add flag that determine anon/not anon behavior. Also this flag should be available from test to have opportunity toggle behavior with the same router. ```diff diff --git a/api/router_mock_test.go b/api/router_mock_test.go index c23b7185..d7c47e61 100644 --- a/api/router_mock_test.go +++ b/api/router_mock_test.go @@ -9,6 +9,7 @@ import ( "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/data" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/middleware" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/creds/accessbox" + "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/bearer" bearertest "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/bearer/test" "github.com/nspcc-dev/neo-go/pkg/crypto/keys" "github.com/nspcc-dev/neo-go/pkg/util" @@ -29,18 +30,25 @@ func (c *anonCenterMock) Authenticate(*http.Request) (*middleware.Box, error) { } type centerMock struct { - t *testing.T + t *testing.T + anon bool } func (c *centerMock) Authenticate(*http.Request) (*middleware.Box, error) { - token := bearertest.Token() - key, err := keys.NewPrivateKey() - require.NoError(c.t, err) - require.NoError(c.t, token.Sign(key.PrivateKey)) + var token *bearer.Token + + if !c.anon { + bt := bearertest.Token() + token = &bt + key, err := keys.NewPrivateKey() + require.NoError(c.t, err) + require.NoError(c.t, token.Sign(key.PrivateKey)) + } + return &middleware.Box{ AccessBox: &accessbox.Box{ Gate: &accessbox.GateData{ - BearerToken: &token, + BearerToken: token, }, }, }, nil diff --git a/api/router_test.go b/api/router_test.go index a69ab7ba..50f0d88f 100644 --- a/api/router_test.go +++ b/api/router_test.go @@ -36,7 +36,7 @@ func (m *routerMock) ServeHTTP(w http.ResponseWriter, r *http.Request) { m.router.ServeHTTP(w, r) } -func prepareRouter(t *testing.T, center s3middleware.Center) *routerMock { +func prepareRouter(t *testing.T) *routerMock { middlewareSettings := &middlewareSettingsMock{} policyChecker := inmemory.NewInMemoryLocalOverrides() @@ -46,7 +46,7 @@ func prepareRouter(t *testing.T, center s3middleware.Center) *routerMock { BacklogTimeout: 30 * time.Second, }, Handler: &handlerMock{t: t}, - Center: center, + Center: &centerMock{t: t}, Log: zaptest.NewLogger(t), Metrics: &metrics.AppMetrics{}, MiddlewareSettings: middlewareSettings, ``` in `TestOwnerIDRetrieving` we can toggle behavior using `chiRouter.cfg.Center.(*centerMock).anon = true`
dkirillov marked this conversation as resolved
api/router_mock_test.go Outdated
@ -39,0 +65,4 @@
type frostFSIDMock struct {
}
func (f *frostFSIDMock) ValidatePublicKey(_ *keys.PublicKey) error {
dkirillov commented 2024-02-27 11:24:52 +00:00
Member
Copy link

We can write just func (f *frostFSIDMock) ValidatePublicKey(*keys.PublicKey) error { (the same for GetUserGroupID )

We can write just `func (f *frostFSIDMock) ValidatePublicKey(*keys.PublicKey) error {` (the same for `GetUserGroupID` )
dkirillov marked this conversation as resolved
api/router_test.go Outdated
@ -253,6 +254,26 @@ func TestDefaultBehaviorPolicyChecker(t *testing.T) {
assertAPIError(t, w, apiErrors.ErrAccessDenied)
}
func TestOwnerIDRetrieving(t *testing.T) {
dkirillov commented 2024-02-27 12:07:22 +00:00
Member
Copy link

This is great test, but can we add another one that check metrics?

For example, we can check if metics contains valid data chiRouter.cfg.Metrics.UsersAPIStats().DumpMetrics()

But for this we need make change in AppMetrics

diff --git a/api/middleware/metrics.go b/api/middleware/metrics.go
index 52f738a1..0a956126 100644
--- a/api/middleware/metrics.go
+++ b/api/middleware/metrics.go
@@ -80,7 +80,7 @@ func stats(f http.HandlerFunc, resolveCID cidResolveFunc, appMetrics *metrics.Ap
                durationSecs := time.Since(statsWriter.startTime).Seconds()
 
                cnrID := resolveCID(r.Context(), reqInfo)
-               appMetrics.Update(reqInfo.User, reqInfo.BucketName, cnrID, settings.ResolveNamespaceAlias(reqInfo.Namespace),
+               appMetrics.UsersAPIStats().Update(reqInfo.User, reqInfo.BucketName, cnrID, settings.ResolveNamespaceAlias(reqInfo.Namespace),
                        requestTypeFromAPI(reqInfo.API), in.countBytes, out.countBytes)
 
                code := statsWriter.statusCode
diff --git a/api/router_mock_test.go b/api/router_mock_test.go
index c23b7185..31235c5d 100644
--- a/api/router_mock_test.go
+++ b/api/router_mock_test.go
@@ -10,6 +10,7 @@ import (
        "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/middleware"
        "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/creds/accessbox"
        bearertest "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/bearer/test"
+       "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/pool"
        "github.com/nspcc-dev/neo-go/pkg/crypto/keys"
        "github.com/nspcc-dev/neo-go/pkg/util"
        "github.com/stretchr/testify/require"
@@ -17,6 +18,13 @@ import (
 
 const FrostfsNamespaceHeader = "X-Frostfs-Namespace"
 
+type poolStatisticMock struct {
+}
+
+func (p *poolStatisticMock) Statistic() pool.Statistic {
+       return pool.Statistic{}
+}
+
 type anonCenterMock struct {
 }
diff --git a/api/router_test.go b/api/router_test.go
index a69ab7ba..8010a4f3 100644
--- a/api/router_test.go
+++ b/api/router_test.go
@@ -21,6 +21,7 @@ import (
        "git.frostfs.info/TrueCloudLab/policy-engine/schema/s3"
        "github.com/go-chi/chi/v5"
        "github.com/go-chi/chi/v5/middleware"
+       "github.com/prometheus/client_golang/prometheus"
        "github.com/stretchr/testify/require"
        "go.uber.org/zap/zaptest"
 )
@@ -40,6 +41,15 @@ func prepareRouter(t *testing.T, center s3middleware.Center) *routerMock {
        middlewareSettings := &middlewareSettingsMock{}
        policyChecker := inmemory.NewInMemoryLocalOverrides()
 
+       logger := zaptest.NewLogger(t)
+
+       metricsConfig := metrics.AppMetricsConfig{
+               Logger:         logger,
+               PoolStatistics: &poolStatisticMock{},
+               Registerer:     prometheus.NewRegistry(),
+               Enabled:        true,
+       }
+
        cfg := Config{
                Throttle: middleware.ThrottleOpts{
                        Limit:          10,
@@ -47,8 +57,8 @@ func prepareRouter(t *testing.T, center s3middleware.Center) *routerMock {
                },
                Handler:            &handlerMock{t: t},
                Center:             center,
-               Log:                zaptest.NewLogger(t),
-               Metrics:            &metrics.AppMetrics{},
+               Log:                logger,
+               Metrics:            metrics.NewAppMetrics(metricsConfig),
                MiddlewareSettings: middlewareSettings,
                PolicyChecker:      policyChecker,
                Domains:            []string{"domain1", "domain2"},
diff --git a/cmd/s3-gw/app.go b/cmd/s3-gw/app.go
index a4448216..57877ad8 100644
--- a/cmd/s3-gw/app.go
+++ b/cmd/s3-gw/app.go
@@ -439,7 +439,13 @@ func (a *App) initControlAPI() {
 }
 
 func (a *App) initMetrics() {
-       a.metrics = metrics.NewAppMetrics(a.log, frostfs.NewPoolStatistic(a.pool), a.cfg.GetBool(cfgPrometheusEnabled))
+       cfg := metrics.AppMetricsConfig{
+               Logger:         a.log,
+               PoolStatistics: frostfs.NewPoolStatistic(a.pool),
+               Enabled:        a.cfg.GetBool(cfgPrometheusEnabled),
+       }
+
+       a.metrics = metrics.NewAppMetrics(cfg)
        a.metrics.State().SetHealth(metrics.HealthStatusStarting)
 }
 
diff --git a/metrics/app.go b/metrics/app.go
index aa652386..64b8e52a 100644
--- a/metrics/app.go
+++ b/metrics/app.go
@@ -5,6 +5,7 @@ import (
        "sync"
 
        "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/internal/logs"
+       "github.com/prometheus/client_golang/prometheus"
        dto "github.com/prometheus/client_model/go"
        "go.uber.org/zap"
 )
@@ -16,14 +17,27 @@ type AppMetrics struct {
        enabled bool
 }
 
-func NewAppMetrics(logger *zap.Logger, poolStatistics StatisticScraper, enabled bool) *AppMetrics {
-       if !enabled {
-               logger.Warn(logs.MetricsAreDisabled)
+type AppMetricsConfig struct {
+       Logger         *zap.Logger
+       PoolStatistics StatisticScraper
+       Registerer     prometheus.Registerer
+       Enabled        bool
+}
+
+func NewAppMetrics(cfg AppMetricsConfig) *AppMetrics {
+       if !cfg.Enabled {
+               cfg.Logger.Warn(logs.MetricsAreDisabled)
+       }
+
+       registry := cfg.Registerer
+       if registry == nil {
+               registry = prometheus.DefaultRegisterer
        }
+
        return &AppMetrics{
-               logger:  logger,
-               gate:    NewGateMetrics(poolStatistics),
-               enabled: enabled,
+               logger:  cfg.Logger,
+               gate:    NewGateMetrics(cfg.PoolStatistics, registry),
+               enabled: cfg.Enabled,
        }
 }
 
@@ -65,12 +79,12 @@ func (m *AppMetrics) Handler() http.Handler {
        return m.gate.Handler()
 }
 
-func (m *AppMetrics) Update(user, bucket, cnrID, ns string, reqType RequestType, in, out uint64) {
+func (m *AppMetrics) UsersAPIStats() *UsersAPIStats {
        if !m.isEnabled() {
-               return
+               return nil
        }
 
-       m.gate.Billing.apiStat.Update(user, bucket, cnrID, ns, reqType, in, out)
+       return m.gate.Billing.apiStat
 }
 
diff --git a/metrics/billing.go b/metrics/billing.go
index 565fedaf..6594f8a4 100644
--- a/metrics/billing.go
+++ b/metrics/billing.go
@@ -111,6 +111,10 @@ type (
 )
 
 func (u *UsersAPIStats) Update(user, bucket, cnrID, ns string, reqType RequestType, in, out uint64) {
+       if u == nil {
+               return
+       }
+
        u.Lock()
        defer u.Unlock()
 
@@ -140,6 +144,10 @@ func (u *UsersAPIStats) Update(user, bucket, cnrID, ns string, reqType RequestTy
 }
 
 func (u *UsersAPIStats) DumpMetrics() UserMetrics {
+       if u == nil {
+               return UserMetrics{}
+       }
+
        u.Lock()
        defer u.Unlock()
 
@@ -195,7 +203,7 @@ type billingMetrics struct {
 
        userRequestsDesc *prometheus.Desc
        userTrafficDesc  *prometheus.Desc
-       apiStat          UsersAPIStats
+       apiStat          *UsersAPIStats
 }
 
 func newBillingMetrics() *billingMetrics {
@@ -203,7 +211,7 @@ func newBillingMetrics() *billingMetrics {
                registry:         prometheus.NewRegistry(),
                userRequestsDesc: newDesc(appMetricsDesc[billingSubsystem][userRequestsMetric]),
                userTrafficDesc:  newDesc(appMetricsDesc[billingSubsystem][userTrafficMetric]),
-               apiStat:          UsersAPIStats{},
+               apiStat:          &UsersAPIStats{},
        }
 }
 

diff --git a/metrics/desc_test.go b/metrics/desc_test.go
index 9ff251b9..6c4c60d8 100644
--- a/metrics/desc_test.go
+++ b/metrics/desc_test.go
@@ -24,7 +24,12 @@ var metricsPath = flag.String("out", "", "File to export s3 gateway metrics to."
 
 func TestDescribeAll(t *testing.T) {
        // to check correct metrics type mapping
-       _ = NewAppMetrics(zaptest.NewLogger(t), mock{}, true)
+       cfg := AppMetricsConfig{
+               Logger:         zaptest.NewLogger(t),
+               PoolStatistics: mock{},
+               Enabled:        true,
+       }
+       _ = NewAppMetrics(cfg)
 
        flag.Parse()
 
diff --git a/metrics/gate.go b/metrics/gate.go
index 02c23293..5a928366 100644
--- a/metrics/gate.go
+++ b/metrics/gate.go
@@ -24,9 +24,7 @@ type GateMetrics struct {
        HTTPServer *httpServerMetrics
 }
 
-func NewGateMetrics(scraper StatisticScraper) *GateMetrics {
-       registry := prometheus.DefaultRegisterer
-
+func NewGateMetrics(scraper StatisticScraper, registry prometheus.Registerer) *GateMetrics {
        stateMetric := newStateMetrics()
        registry.MustRegister(stateMetric)
This is great test, but can we add another one that check metrics? For example, we can check if metics contains valid data `chiRouter.cfg.Metrics.UsersAPIStats().DumpMetrics()` But for this we need make change in `AppMetrics` ```diff diff --git a/api/middleware/metrics.go b/api/middleware/metrics.go index 52f738a1..0a956126 100644 --- a/api/middleware/metrics.go +++ b/api/middleware/metrics.go @@ -80,7 +80,7 @@ func stats(f http.HandlerFunc, resolveCID cidResolveFunc, appMetrics *metrics.Ap durationSecs := time.Since(statsWriter.startTime).Seconds() cnrID := resolveCID(r.Context(), reqInfo) - appMetrics.Update(reqInfo.User, reqInfo.BucketName, cnrID, settings.ResolveNamespaceAlias(reqInfo.Namespace), + appMetrics.UsersAPIStats().Update(reqInfo.User, reqInfo.BucketName, cnrID, settings.ResolveNamespaceAlias(reqInfo.Namespace), requestTypeFromAPI(reqInfo.API), in.countBytes, out.countBytes) code := statsWriter.statusCode diff --git a/api/router_mock_test.go b/api/router_mock_test.go index c23b7185..31235c5d 100644 --- a/api/router_mock_test.go +++ b/api/router_mock_test.go @@ -10,6 +10,7 @@ import ( "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/middleware" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/creds/accessbox" bearertest "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/bearer/test" + "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/pool" "github.com/nspcc-dev/neo-go/pkg/crypto/keys" "github.com/nspcc-dev/neo-go/pkg/util" "github.com/stretchr/testify/require" @@ -17,6 +18,13 @@ import ( const FrostfsNamespaceHeader = "X-Frostfs-Namespace" +type poolStatisticMock struct { +} + +func (p *poolStatisticMock) Statistic() pool.Statistic { + return pool.Statistic{} +} + type anonCenterMock struct { } diff --git a/api/router_test.go b/api/router_test.go index a69ab7ba..8010a4f3 100644 --- a/api/router_test.go +++ b/api/router_test.go @@ -21,6 +21,7 @@ import ( "git.frostfs.info/TrueCloudLab/policy-engine/schema/s3" "github.com/go-chi/chi/v5" "github.com/go-chi/chi/v5/middleware" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/require" "go.uber.org/zap/zaptest" ) @@ -40,6 +41,15 @@ func prepareRouter(t *testing.T, center s3middleware.Center) *routerMock { middlewareSettings := &middlewareSettingsMock{} policyChecker := inmemory.NewInMemoryLocalOverrides() + logger := zaptest.NewLogger(t) + + metricsConfig := metrics.AppMetricsConfig{ + Logger: logger, + PoolStatistics: &poolStatisticMock{}, + Registerer: prometheus.NewRegistry(), + Enabled: true, + } + cfg := Config{ Throttle: middleware.ThrottleOpts{ Limit: 10, @@ -47,8 +57,8 @@ func prepareRouter(t *testing.T, center s3middleware.Center) *routerMock { }, Handler: &handlerMock{t: t}, Center: center, - Log: zaptest.NewLogger(t), - Metrics: &metrics.AppMetrics{}, + Log: logger, + Metrics: metrics.NewAppMetrics(metricsConfig), MiddlewareSettings: middlewareSettings, PolicyChecker: policyChecker, Domains: []string{"domain1", "domain2"}, diff --git a/cmd/s3-gw/app.go b/cmd/s3-gw/app.go index a4448216..57877ad8 100644 --- a/cmd/s3-gw/app.go +++ b/cmd/s3-gw/app.go @@ -439,7 +439,13 @@ func (a *App) initControlAPI() { } func (a *App) initMetrics() { - a.metrics = metrics.NewAppMetrics(a.log, frostfs.NewPoolStatistic(a.pool), a.cfg.GetBool(cfgPrometheusEnabled)) + cfg := metrics.AppMetricsConfig{ + Logger: a.log, + PoolStatistics: frostfs.NewPoolStatistic(a.pool), + Enabled: a.cfg.GetBool(cfgPrometheusEnabled), + } + + a.metrics = metrics.NewAppMetrics(cfg) a.metrics.State().SetHealth(metrics.HealthStatusStarting) } diff --git a/metrics/app.go b/metrics/app.go index aa652386..64b8e52a 100644 --- a/metrics/app.go +++ b/metrics/app.go @@ -5,6 +5,7 @@ import ( "sync" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/internal/logs" + "github.com/prometheus/client_golang/prometheus" dto "github.com/prometheus/client_model/go" "go.uber.org/zap" ) @@ -16,14 +17,27 @@ type AppMetrics struct { enabled bool } -func NewAppMetrics(logger *zap.Logger, poolStatistics StatisticScraper, enabled bool) *AppMetrics { - if !enabled { - logger.Warn(logs.MetricsAreDisabled) +type AppMetricsConfig struct { + Logger *zap.Logger + PoolStatistics StatisticScraper + Registerer prometheus.Registerer + Enabled bool +} + +func NewAppMetrics(cfg AppMetricsConfig) *AppMetrics { + if !cfg.Enabled { + cfg.Logger.Warn(logs.MetricsAreDisabled) + } + + registry := cfg.Registerer + if registry == nil { + registry = prometheus.DefaultRegisterer } + return &AppMetrics{ - logger: logger, - gate: NewGateMetrics(poolStatistics), - enabled: enabled, + logger: cfg.Logger, + gate: NewGateMetrics(cfg.PoolStatistics, registry), + enabled: cfg.Enabled, } } @@ -65,12 +79,12 @@ func (m *AppMetrics) Handler() http.Handler { return m.gate.Handler() } -func (m *AppMetrics) Update(user, bucket, cnrID, ns string, reqType RequestType, in, out uint64) { +func (m *AppMetrics) UsersAPIStats() *UsersAPIStats { if !m.isEnabled() { - return + return nil } - m.gate.Billing.apiStat.Update(user, bucket, cnrID, ns, reqType, in, out) + return m.gate.Billing.apiStat } diff --git a/metrics/billing.go b/metrics/billing.go index 565fedaf..6594f8a4 100644 --- a/metrics/billing.go +++ b/metrics/billing.go @@ -111,6 +111,10 @@ type ( ) func (u *UsersAPIStats) Update(user, bucket, cnrID, ns string, reqType RequestType, in, out uint64) { + if u == nil { + return + } + u.Lock() defer u.Unlock() @@ -140,6 +144,10 @@ func (u *UsersAPIStats) Update(user, bucket, cnrID, ns string, reqType RequestTy } func (u *UsersAPIStats) DumpMetrics() UserMetrics { + if u == nil { + return UserMetrics{} + } + u.Lock() defer u.Unlock() @@ -195,7 +203,7 @@ type billingMetrics struct { userRequestsDesc *prometheus.Desc userTrafficDesc *prometheus.Desc - apiStat UsersAPIStats + apiStat *UsersAPIStats } func newBillingMetrics() *billingMetrics { @@ -203,7 +211,7 @@ func newBillingMetrics() *billingMetrics { registry: prometheus.NewRegistry(), userRequestsDesc: newDesc(appMetricsDesc[billingSubsystem][userRequestsMetric]), userTrafficDesc: newDesc(appMetricsDesc[billingSubsystem][userTrafficMetric]), - apiStat: UsersAPIStats{}, + apiStat: &UsersAPIStats{}, } } diff --git a/metrics/desc_test.go b/metrics/desc_test.go index 9ff251b9..6c4c60d8 100644 --- a/metrics/desc_test.go +++ b/metrics/desc_test.go @@ -24,7 +24,12 @@ var metricsPath = flag.String("out", "", "File to export s3 gateway metrics to." func TestDescribeAll(t *testing.T) { // to check correct metrics type mapping - _ = NewAppMetrics(zaptest.NewLogger(t), mock{}, true) + cfg := AppMetricsConfig{ + Logger: zaptest.NewLogger(t), + PoolStatistics: mock{}, + Enabled: true, + } + _ = NewAppMetrics(cfg) flag.Parse() diff --git a/metrics/gate.go b/metrics/gate.go index 02c23293..5a928366 100644 --- a/metrics/gate.go +++ b/metrics/gate.go @@ -24,9 +24,7 @@ type GateMetrics struct { HTTPServer *httpServerMetrics } -func NewGateMetrics(scraper StatisticScraper) *GateMetrics { - registry := prometheus.DefaultRegisterer - +func NewGateMetrics(scraper StatisticScraper, registry prometheus.Registerer) *GateMetrics { stateMetric := newStateMetrics() registry.MustRegister(stateMetric) ```
dkirillov commented 2024-02-27 12:13:02 +00:00
Member
Copy link

This is ok for master I suppose, but for support probably we should keep as little changes as possible.

cc @alexvanin

This is ok for `master` I suppose, but for `support` probably we should keep as little changes as possible. cc @alexvanin
👍 1
dkirillov marked this conversation as resolved
mbiryukova force-pushed bugfix/anon_user_in_metrics from e268ae19f8 to 659e7dbf89 2024-02-27 15:46:36 +00:00 Compare
alexvanin approved these changes 2024-02-28 10:33:45 +00:00
dkirillov approved these changes 2024-02-28 11:47:02 +00:00
api/router_test.go Outdated
@ -256,0 +284,4 @@
require.Equal(t, "anon", resp.ReqInfo.User)
}
func TestDumpMetrics(t *testing.T) {
dkirillov commented 2024-02-28 11:46:58 +00:00
Member
Copy link

Can we rename this to not be confused with test that dumps (prints) all supported metrics (test TestDescribeAll with tag dump_metrics)?

Can we rename this to not be confused with test that dumps (prints) all supported metrics (test `TestDescribeAll` with tag `dump_metrics`)?
dkirillov marked this conversation as resolved
mbiryukova force-pushed bugfix/anon_user_in_metrics from 659e7dbf89 to 2981a47e99 2024-02-28 11:57:28 +00:00 Compare
dkirillov approved these changes 2024-02-28 12:15:56 +00:00
alexvanin merged commit 2981a47e99 into master 2024-02-28 14:27:03 +00:00
alexvanin deleted branch bugfix/anon_user_in_metrics 2024-02-28 14:27:03 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
alexvanin
dkirillov
TrueCloudLab/storage-services-developers
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
mbiryukova
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-s3-gw#322
No description provided.
Delete branch "mbiryukova/frostfs-s3-gw:bugfix/anon_user_in_metrics"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?