From 3106abf1a6e45d2ae2e818e0c8bcec87629cd05a Mon Sep 17 00:00:00 2001 From: "lean.dev" <34773040+leandro-deveikis@users.noreply.github.com> Date: Wed, 13 Aug 2025 14:01:50 +0100 Subject: [PATCH] SecretsManager: clear unused metrics and standarize labels(#109515) --- .../apis/secret/service/metrics/metrics.go | 52 +------ .../apis/secret/service/secure_value.go | 7 +- pkg/storage/secret/metadata/decrypt_store.go | 1 - .../secret/metadata/decrypt_store_test.go | 9 +- pkg/storage/secret/metadata/keeper_store.go | 146 +++++++++++++++--- .../secret/metadata/metrics/metrics.go | 128 ++++----------- .../secret/metadata/secure_value_store.go | 67 ++++++-- 7 files changed, 221 insertions(+), 189 deletions(-) diff --git a/pkg/registry/apis/secret/service/metrics/metrics.go b/pkg/registry/apis/secret/service/metrics/metrics.go index b179ead20b7..a1167b1e6ce 100644 --- a/pkg/registry/apis/secret/service/metrics/metrics.go +++ b/pkg/registry/apis/secret/service/metrics/metrics.go @@ -9,20 +9,17 @@ import ( const ( namespace = "grafana_secrets_manager" subsystem = "service" + // labels + successLabel = "success" ) // SecureValueServiceMetrics is a struct that contains all the metrics for SecureValue. type SecureValueServiceMetrics struct { SecureValueCreateDuration *prometheus.HistogramVec - SecureValueCreateCount *prometheus.CounterVec SecureValueUpdateDuration *prometheus.HistogramVec - SecureValueUpdateCount *prometheus.CounterVec SecureValueReadDuration *prometheus.HistogramVec - SecureValueReadCount *prometheus.CounterVec SecureValueListDuration *prometheus.HistogramVec - SecureValueListCount *prometheus.CounterVec SecureValueDeleteDuration *prometheus.HistogramVec - SecureValueDeleteCount *prometheus.CounterVec } func newSecureValueServiceMetrics() *SecureValueServiceMetrics { @@ -33,65 +30,35 @@ func newSecureValueServiceMetrics() *SecureValueServiceMetrics { Name: "secure_value_create_duration_seconds", Help: "Duration of Secure Value create operations", Buckets: prometheus.DefBuckets, - }, []string{"success"}), - SecureValueCreateCount: prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: namespace, - Subsystem: subsystem, - Name: "secure_value_create_count", - Help: "Count of Secure Value create operations", - }, []string{"success"}), + }, []string{successLabel}), SecureValueReadDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ Namespace: namespace, Subsystem: subsystem, Name: "secure_value_read_duration_seconds", Help: "Duration of Secure Value read operations", Buckets: prometheus.DefBuckets, - }, []string{"success"}), - SecureValueReadCount: prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: namespace, - Subsystem: subsystem, - Name: "secure_value_read_count", - Help: "Count of Secure Value read operations", - }, []string{"success"}), + }, []string{successLabel}), SecureValueUpdateDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ Namespace: namespace, Subsystem: subsystem, Name: "secure_value_update_duration_seconds", Help: "Duration of Secure Value update operations", Buckets: prometheus.DefBuckets, - }, []string{"success"}), - SecureValueUpdateCount: prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: namespace, - Subsystem: subsystem, - Name: "secure_value_update_count", - Help: "Count of Secure Value update operations", - }, []string{"success"}), + }, []string{successLabel}), SecureValueListDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ Namespace: namespace, Subsystem: subsystem, Name: "secure_value_list_duration_seconds", Help: "Duration of Secure Value list operations", Buckets: prometheus.DefBuckets, - }, []string{"success"}), - SecureValueListCount: prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: namespace, - Subsystem: subsystem, - Name: "secure_value_list_count", - Help: "Count of Secure Value list operations", - }, []string{"success"}), + }, []string{successLabel}), SecureValueDeleteDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ Namespace: namespace, Subsystem: subsystem, Name: "secure_value_delete_duration_seconds", Help: "Duration of Secure Value delete operations", Buckets: prometheus.DefBuckets, - }, []string{"success"}), - SecureValueDeleteCount: prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: namespace, - Subsystem: subsystem, - Name: "secure_value_delete_count", - Help: "Count of Secure Value delete operations", - }, []string{"success"}), + }, []string{successLabel}), } } @@ -107,15 +74,10 @@ func NewSecureValueServiceMetrics(reg prometheus.Registerer) *SecureValueService if reg != nil { reg.MustRegister( m.SecureValueCreateDuration, - m.SecureValueCreateCount, m.SecureValueReadDuration, - m.SecureValueReadCount, m.SecureValueUpdateDuration, - m.SecureValueUpdateCount, m.SecureValueListDuration, - m.SecureValueListCount, m.SecureValueDeleteDuration, - m.SecureValueDeleteCount, ) } metricsInstance = m diff --git a/pkg/registry/apis/secret/service/secure_value.go b/pkg/registry/apis/secret/service/secure_value.go index f8b1f91d727..ae3422acc7c 100644 --- a/pkg/registry/apis/secret/service/secure_value.go +++ b/pkg/registry/apis/secret/service/secure_value.go @@ -90,7 +90,6 @@ func (s *SecureValueService) Create(ctx context.Context, sv *secretv1beta1.Secur logging.FromContext(ctx).Info("SecureValueService.Create finished", args...) s.metrics.SecureValueCreateDuration.WithLabelValues(strconv.FormatBool(success)).Observe(time.Since(start).Seconds()) - s.metrics.SecureValueCreateCount.WithLabelValues(strconv.FormatBool(success)).Inc() }() return s.createNewVersion(ctx, sv, actorUID) @@ -126,7 +125,6 @@ func (s *SecureValueService) Update(ctx context.Context, newSecureValue *secretv logging.FromContext(ctx).Info("SecureValueService.Update finished", args...) s.metrics.SecureValueUpdateDuration.WithLabelValues(strconv.FormatBool(success)).Observe(time.Since(start).Seconds()) - s.metrics.SecureValueUpdateCount.WithLabelValues(strconv.FormatBool(success)).Inc() }() if newSecureValue.Spec.Value == nil { @@ -231,7 +229,7 @@ func (s *SecureValueService) Read(ctx context.Context, namespace xkube.Namespace defer func() { args := []any{ "name", name, - "namespace", namespace, + "namespace", namespace.String(), } success := readErr == nil @@ -245,7 +243,6 @@ func (s *SecureValueService) Read(ctx context.Context, namespace xkube.Namespace logging.FromContext(ctx).Info("SecureValueService.Read finished", args...) s.metrics.SecureValueReadDuration.WithLabelValues(strconv.FormatBool(success)).Observe(time.Since(start).Seconds()) - s.metrics.SecureValueReadCount.WithLabelValues(strconv.FormatBool(success)).Inc() }() defer span.End() @@ -281,7 +278,6 @@ func (s *SecureValueService) List(ctx context.Context, namespace xkube.Namespace logging.FromContext(ctx).Info("SecureValueService.List finished", args...) s.metrics.SecureValueListDuration.WithLabelValues(strconv.FormatBool(success)).Observe(time.Since(start).Seconds()) - s.metrics.SecureValueListCount.WithLabelValues(strconv.FormatBool(success)).Inc() }() user, ok := claims.AuthInfoFrom(ctx) @@ -353,7 +349,6 @@ func (s *SecureValueService) Delete(ctx context.Context, namespace xkube.Namespa logging.FromContext(ctx).Info("SecureValueService.Delete finished", args...) s.metrics.SecureValueDeleteDuration.WithLabelValues(strconv.FormatBool(success)).Observe(time.Since(start).Seconds()) - s.metrics.SecureValueDeleteCount.WithLabelValues(strconv.FormatBool(success)).Inc() }() // TODO: does this need to be for update? diff --git a/pkg/storage/secret/metadata/decrypt_store.go b/pkg/storage/secret/metadata/decrypt_store.go index de98a817a86..cd959883824 100644 --- a/pkg/storage/secret/metadata/decrypt_store.go +++ b/pkg/storage/secret/metadata/decrypt_store.go @@ -95,7 +95,6 @@ func (s *decryptStorage) Decrypt(ctx context.Context, namespace xkube.Namespace, success := decryptErr == nil s.metrics.DecryptDuration.WithLabelValues(strconv.FormatBool(success)).Observe(time.Since(start).Seconds()) - s.metrics.DecryptRequestCount.WithLabelValues(strconv.FormatBool(success)).Inc() }() // Basic authn check before reading a secure value metadata, it is here on purpose. diff --git a/pkg/storage/secret/metadata/decrypt_store_test.go b/pkg/storage/secret/metadata/decrypt_store_test.go index 3c35a443794..bc50b29c4a6 100644 --- a/pkg/storage/secret/metadata/decrypt_store_test.go +++ b/pkg/storage/secret/metadata/decrypt_store_test.go @@ -292,13 +292,14 @@ func TestIntegrationDecrypt(t *testing.T) { require.NotEmpty(t, exposed) require.Equal(t, "value", exposed.DangerouslyExposeAndConsumeValue()) - require.Len(t, fakeLogger.InfoMsgs, 2) + require.Len(t, fakeLogger.InfoMsgs, 3) require.Equal(t, fakeLogger.InfoMsgs[0], "SecureValueMetadataStorage.Read") - require.Equal(t, fakeLogger.InfoMsgs[1], "Secrets Audit Log") + require.Equal(t, fakeLogger.InfoMsgs[1], "KeeperMetadataStorage.GetKeeperConfig") + require.Equal(t, fakeLogger.InfoMsgs[2], "Secrets Audit Log") - require.Len(t, fakeLogger.InfoArgs, 2) + require.Len(t, fakeLogger.InfoArgs, 3) // we only want to check the audit log args - args := fakeLogger.InfoArgs[1] + args := fakeLogger.InfoArgs[2] require.Contains(t, args, "grafana_decrypter_identity") require.Contains(t, args, "decrypter_identity") for i, arg := range args { diff --git a/pkg/storage/secret/metadata/keeper_store.go b/pkg/storage/secret/metadata/keeper_store.go index 92707351ba7..7c9f12860ea 100644 --- a/pkg/storage/secret/metadata/keeper_store.go +++ b/pkg/storage/secret/metadata/keeper_store.go @@ -3,6 +3,7 @@ package metadata import ( "context" "fmt" + "strconv" "time" "github.com/prometheus/client_golang/prometheus" @@ -10,6 +11,7 @@ import ( "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/trace" + "github.com/grafana/grafana-app-sdk/logging" secretv1beta1 "github.com/grafana/grafana/apps/secret/pkg/apis/secret/v1beta1" "github.com/grafana/grafana/pkg/registry/apis/secret/contracts" "github.com/grafana/grafana/pkg/registry/apis/secret/xkube" @@ -40,7 +42,7 @@ func ProvideKeeperMetadataStorage( }, nil } -func (s *keeperMetadataStorage) Create(ctx context.Context, keeper *secretv1beta1.Keeper, actorUID string) (*secretv1beta1.Keeper, error) { +func (s *keeperMetadataStorage) Create(ctx context.Context, keeper *secretv1beta1.Keeper, actorUID string) (_ *secretv1beta1.Keeper, createErr error) { start := time.Now() ctx, span := s.tracer.Start(ctx, "KeeperMetadataStorage.Create", trace.WithAttributes( attribute.String("name", keeper.GetName()), @@ -49,6 +51,26 @@ func (s *keeperMetadataStorage) Create(ctx context.Context, keeper *secretv1beta )) defer span.End() + defer func() { + success := createErr == nil + + args := []any{ + "name", keeper.GetName(), + "namespace", keeper.GetNamespace(), + "actorUID", actorUID, + } + + if !success { + span.SetStatus(codes.Error, "KeeperMetadataStorage.Create failed") + span.RecordError(createErr) + args = append(args, "error", createErr) + } + + logging.FromContext(ctx).Info("KeeperMetadataStorage.Create", args...) + + s.metrics.KeeperMetadataCreateDuration.WithLabelValues(strconv.FormatBool(success)).Observe(time.Since(start).Seconds()) + }() + row, err := toKeeperCreateRow(keeper, actorUID) if err != nil { return nil, fmt.Errorf("failed to create row: %w", err) @@ -99,13 +121,10 @@ func (s *keeperMetadataStorage) Create(ctx context.Context, keeper *secretv1beta return nil, fmt.Errorf("failed to convert to kubernetes object: %w", err) } - s.metrics.KeeperMetadataCreateDuration.WithLabelValues(string(createdKeeper.Spec.GetType())).Observe(time.Since(start).Seconds()) - s.metrics.KeeperMetadataCreateCount.WithLabelValues(string(createdKeeper.Spec.GetType())).Inc() - return createdKeeper, nil } -func (s *keeperMetadataStorage) Read(ctx context.Context, namespace xkube.Namespace, name string, opts contracts.ReadOpts) (*secretv1beta1.Keeper, error) { +func (s *keeperMetadataStorage) Read(ctx context.Context, namespace xkube.Namespace, name string, opts contracts.ReadOpts) (_ *secretv1beta1.Keeper, readErr error) { start := time.Now() ctx, span := s.tracer.Start(ctx, "KeeperMetadataStorage.Read", trace.WithAttributes( attribute.String("name", name), @@ -114,6 +133,26 @@ func (s *keeperMetadataStorage) Read(ctx context.Context, namespace xkube.Namesp )) defer span.End() + defer func() { + success := readErr == nil + + args := []any{ + "name", name, + "namespace", namespace.String(), + } + + args = append(args, "success", success) + if !success { + span.SetStatus(codes.Error, "KeeperMetadataStorage.Read failed") + span.RecordError(readErr) + args = append(args, "error", readErr) + } + + logging.FromContext(ctx).Info("KeeperMetadataStorage.Read", args...) + + s.metrics.KeeperMetadataGetDuration.WithLabelValues(strconv.FormatBool(success)).Observe(time.Since(start).Seconds()) + }() + keeperDB, err := s.read(ctx, namespace.String(), name, opts) if err != nil { return nil, err @@ -124,9 +163,6 @@ func (s *keeperMetadataStorage) Read(ctx context.Context, namespace xkube.Namesp return nil, fmt.Errorf("failed to convert to kubernetes object: %w", err) } - s.metrics.KeeperMetadataGetDuration.WithLabelValues(string(keeper.Spec.GetType())).Observe(time.Since(start).Seconds()) - s.metrics.KeeperMetadataGetCount.WithLabelValues(string(keeper.Spec.GetType())).Inc() - return keeper, nil } @@ -168,7 +204,7 @@ func (s *keeperMetadataStorage) read(ctx context.Context, namespace, name string return &keeper, nil } -func (s *keeperMetadataStorage) Update(ctx context.Context, newKeeper *secretv1beta1.Keeper, actorUID string) (*secretv1beta1.Keeper, error) { +func (s *keeperMetadataStorage) Update(ctx context.Context, newKeeper *secretv1beta1.Keeper, actorUID string) (_ *secretv1beta1.Keeper, updateErr error) { start := time.Now() ctx, span := s.tracer.Start(ctx, "KeeperMetadataStorage.Update", trace.WithAttributes( attribute.String("name", newKeeper.GetName()), @@ -177,6 +213,25 @@ func (s *keeperMetadataStorage) Update(ctx context.Context, newKeeper *secretv1b )) defer span.End() + defer func() { + success := updateErr == nil + args := []any{ + "name", newKeeper.GetName(), + "namespace", newKeeper.GetNamespace(), + "actorUID", actorUID, + } + + args = append(args, "success", success) + if !success { + span.SetStatus(codes.Error, "KeeperMetadataStorage.Update failed") + span.RecordError(updateErr) + args = append(args, "error", updateErr) + } + + logging.FromContext(ctx).Info("KeeperMetadataStorage.Update", args...) + s.metrics.KeeperMetadataUpdateDuration.WithLabelValues(strconv.FormatBool(success)).Observe(time.Since(start).Seconds()) + }() + var newRow *keeperDB err := s.db.Transaction(ctx, func(ctx context.Context) error { @@ -239,13 +294,10 @@ func (s *keeperMetadataStorage) Update(ctx context.Context, newKeeper *secretv1b return nil, fmt.Errorf("failed to convert to kubernetes object: %w", err) } - s.metrics.KeeperMetadataUpdateDuration.WithLabelValues(string(keeper.Spec.GetType())).Observe(time.Since(start).Seconds()) - s.metrics.KeeperMetadataUpdateCount.WithLabelValues(string(keeper.Spec.GetType())).Inc() - return keeper, nil } -func (s *keeperMetadataStorage) Delete(ctx context.Context, namespace xkube.Namespace, name string) error { +func (s *keeperMetadataStorage) Delete(ctx context.Context, namespace xkube.Namespace, name string) (delErr error) { start := time.Now() ctx, span := s.tracer.Start(ctx, "KeeperMetadataStorage.Delete", trace.WithAttributes( attribute.String("name", name), @@ -253,6 +305,26 @@ func (s *keeperMetadataStorage) Delete(ctx context.Context, namespace xkube.Name )) defer span.End() + defer func() { + success := delErr == nil + + args := []any{ + "name", name, + "namespace", namespace.String(), + } + + args = append(args, "success", success) + if !success { + span.SetStatus(codes.Error, "KeeperMetadataStorage.Delete failed") + span.RecordError(delErr) + args = append(args, "error", delErr) + } + + logging.FromContext(ctx).Info("KeeperMetadataStorage.Delete", args...) + + s.metrics.KeeperMetadataDeleteDuration.WithLabelValues(strconv.FormatBool(success)).Observe(time.Since(start).Seconds()) + }() + req := deleteKeeper{ SQLTemplate: sqltemplate.New(s.dialect), Namespace: namespace.String(), @@ -279,9 +351,6 @@ func (s *keeperMetadataStorage) Delete(ctx context.Context, namespace xkube.Name return fmt.Errorf("expected 1 row affected, got %d for %s on %s", rowsAffected, name, namespace) } - s.metrics.KeeperMetadataDeleteDuration.Observe(time.Since(start).Seconds()) - s.metrics.KeeperMetadataDeleteCount.Inc() - return nil } @@ -294,6 +363,22 @@ func (s *keeperMetadataStorage) List(ctx context.Context, namespace xkube.Namesp defer func() { span.SetAttributes(attribute.Int("returnedList.count", len(keeperList))) + success := err == nil + + args := []any{ + "namespace", namespace.String(), + "success", success, + } + + if !success { + span.SetStatus(codes.Error, "KeeperMetadataStorage.List failed") + span.RecordError(err) + args = append(args, "error", err) + } + + logging.FromContext(ctx).Info("KeeperMetadataStorage.List", args...) + + s.metrics.KeeperMetadataListDuration.WithLabelValues(strconv.FormatBool(success)).Observe(time.Since(start).Seconds()) }() req := listKeeper{ @@ -336,9 +421,6 @@ func (s *keeperMetadataStorage) List(ctx context.Context, namespace xkube.Namesp return nil, fmt.Errorf("read rows error: %w", err) } - s.metrics.KeeperMetadataListDuration.Observe(time.Since(start).Seconds()) - s.metrics.KeeperMetadataListCount.Inc() - return keepers, nil } @@ -491,19 +573,39 @@ func (s *keeperMetadataStorage) validateSecureValueReferences(ctx context.Contex return nil } -func (s *keeperMetadataStorage) GetKeeperConfig(ctx context.Context, namespace string, name *string, opts contracts.ReadOpts) (secretv1beta1.KeeperConfig, error) { +func (s *keeperMetadataStorage) GetKeeperConfig(ctx context.Context, namespace string, name *string, opts contracts.ReadOpts) (_ secretv1beta1.KeeperConfig, getErr error) { ctx, span := s.tracer.Start(ctx, "KeeperMetadataStorage.GetKeeperConfig", trace.WithAttributes( attribute.String("namespace", namespace), attribute.Bool("isForUpdate", opts.ForUpdate), )) + start := time.Now() defer span.End() + defer func() { + success := getErr == nil + + args := []any{ + "namespace", namespace, + "isForUpdate", strconv.FormatBool(opts.ForUpdate), + } + + args = append(args, "success", success) + if !success { + span.SetStatus(codes.Error, "KeeperMetadataStorage.GetKeeperConfig failed") + span.RecordError(getErr) + args = append(args, "error", getErr) + } + + logging.FromContext(ctx).Info("KeeperMetadataStorage.GetKeeperConfig", args...) + + s.metrics.KeeperMetadataGetKeeperConfigDuration.WithLabelValues(strconv.FormatBool(success)).Observe(time.Since(start).Seconds()) + }() + // Check if keeper is the systemwide one. if name == nil { return &secretv1beta1.SystemKeeperConfig{}, nil } - start := time.Now() span.SetAttributes(attribute.String("name", *name)) // Load keeper config from metadata store, or TODO: keeper cache. @@ -514,8 +616,6 @@ func (s *keeperMetadataStorage) GetKeeperConfig(ctx context.Context, namespace s keeperConfig := toProvider(secretv1beta1.KeeperType(kp.Type), kp.Payload) - s.metrics.KeeperMetadataGetKeeperConfigDuration.Observe(time.Since(start).Seconds()) - // TODO: this would be a good place to check if credentials are secure values and load them. return keeperConfig, nil } diff --git a/pkg/storage/secret/metadata/metrics/metrics.go b/pkg/storage/secret/metadata/metrics/metrics.go index f9baba1de64..5d904253dde 100644 --- a/pkg/storage/secret/metadata/metrics/metrics.go +++ b/pkg/storage/secret/metadata/metrics/metrics.go @@ -9,33 +9,26 @@ import ( const ( namespace = "grafana_secrets_manager" subsystem = "storage" + // labels + successLabel = "success" ) // StorageMetrics is a struct that contains all the metrics for all operations of secrets storage. type StorageMetrics struct { KeeperMetadataCreateDuration *prometheus.HistogramVec - KeeperMetadataCreateCount *prometheus.CounterVec KeeperMetadataUpdateDuration *prometheus.HistogramVec - KeeperMetadataUpdateCount *prometheus.CounterVec - KeeperMetadataDeleteDuration prometheus.Histogram - KeeperMetadataDeleteCount prometheus.Counter + KeeperMetadataDeleteDuration *prometheus.HistogramVec KeeperMetadataGetDuration *prometheus.HistogramVec - KeeperMetadataGetCount *prometheus.CounterVec - KeeperMetadataListDuration prometheus.Histogram - KeeperMetadataListCount prometheus.Counter - KeeperMetadataGetKeeperConfigDuration prometheus.Histogram + KeeperMetadataListDuration *prometheus.HistogramVec + KeeperMetadataGetKeeperConfigDuration *prometheus.HistogramVec SecureValueMetadataCreateDuration *prometheus.HistogramVec - SecureValueMetadataCreateCount *prometheus.CounterVec - SecureValueMetadataGetDuration prometheus.Histogram - SecureValueMetadataGetCount prometheus.Counter - SecureValueMetadataListDuration prometheus.Histogram - SecureValueMetadataListCount prometheus.Counter - SecureValueSetExternalIDDuration prometheus.Histogram - SecureValueSetStatusDuration prometheus.Histogram + SecureValueMetadataGetDuration *prometheus.HistogramVec + SecureValueMetadataListDuration *prometheus.HistogramVec + SecureValueSetExternalIDDuration *prometheus.HistogramVec + SecureValueSetStatusDuration *prometheus.HistogramVec - DecryptDuration *prometheus.HistogramVec - DecryptRequestCount *prometheus.CounterVec + DecryptDuration *prometheus.HistogramVec } func newStorageMetrics() *StorageMetrics { @@ -47,72 +40,42 @@ func newStorageMetrics() *StorageMetrics { Name: "keeper_metadata_create_duration_seconds", Help: "Duration of keeper metadata create operations", Buckets: prometheus.DefBuckets, - }, []string{"keeper_type"}), - KeeperMetadataCreateCount: prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: namespace, - Subsystem: subsystem, - Name: "keeper_metadata_create_count", - Help: "Count of keeper metadata create operations", - }, []string{"keeper_type"}), + }, []string{successLabel}), KeeperMetadataUpdateDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ Namespace: namespace, Subsystem: subsystem, Name: "keeper_metadata_update_duration_seconds", Help: "Duration of keeper metadata update operations", Buckets: prometheus.DefBuckets, - }, []string{"keeper_type"}), - KeeperMetadataUpdateCount: prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: namespace, - Subsystem: subsystem, - Name: "keeper_metadata_update_count", - Help: "Count of keeper metadata update operations", - }, []string{"keeper_type"}), - KeeperMetadataDeleteDuration: prometheus.NewHistogram(prometheus.HistogramOpts{ + }, []string{successLabel}), + KeeperMetadataDeleteDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ Namespace: namespace, Subsystem: subsystem, Name: "keeper_metadata_delete_duration_seconds", Help: "Duration of keeper metadata delete operations", Buckets: prometheus.DefBuckets, - }), - KeeperMetadataDeleteCount: prometheus.NewCounter(prometheus.CounterOpts{ - Namespace: namespace, - Subsystem: subsystem, - Name: "keeper_metadata_delete_count", - Help: "Count of keeper metadata delete operations", - }), + }, []string{successLabel}), KeeperMetadataGetDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ Namespace: namespace, Subsystem: subsystem, Name: "keeper_metadata_get_duration_seconds", Help: "Duration of keeper metadata get operations", Buckets: prometheus.DefBuckets, - }, []string{"keeper_type"}), - KeeperMetadataGetCount: prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: namespace, - Subsystem: subsystem, - Name: "keeper_metadata_get_count", - Help: "Count of keeper metadata get operations", - }, []string{"keeper_type"}), - KeeperMetadataListDuration: prometheus.NewHistogram(prometheus.HistogramOpts{ + }, []string{successLabel}), + KeeperMetadataListDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ Namespace: namespace, Subsystem: subsystem, Name: "keeper_metadata_list_duration_seconds", Help: "Duration of keeper metadata list operations", Buckets: prometheus.DefBuckets, - }), - KeeperMetadataListCount: prometheus.NewCounter(prometheus.CounterOpts{ - Namespace: namespace, - Subsystem: subsystem, - Name: "keeper_metadata_list_count", - Help: "Count of keeper metadata list operations", - }), - KeeperMetadataGetKeeperConfigDuration: prometheus.NewHistogram(prometheus.HistogramOpts{ + }, []string{successLabel}), + KeeperMetadataGetKeeperConfigDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ Namespace: namespace, Subsystem: subsystem, Name: "keeper_metadata_get_keeper_config_duration_seconds", Help: "Duration of keeper metadata get keeper config operations", Buckets: prometheus.DefBuckets, - }), + }, []string{successLabel}), // Secure value metrics SecureValueMetadataCreateDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ @@ -121,53 +84,35 @@ func newStorageMetrics() *StorageMetrics { Name: "secure_value_metadata_create_duration_seconds", Help: "Duration of secure value metadata create operations", Buckets: prometheus.DefBuckets, - }, []string{"successful"}), - SecureValueMetadataCreateCount: prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: namespace, - Subsystem: subsystem, - Name: "secure_value_metadata_create_count", - Help: "Count of secure value metadata create operations", - }, []string{"successful"}), - SecureValueMetadataGetDuration: prometheus.NewHistogram(prometheus.HistogramOpts{ + }, []string{successLabel}), + SecureValueMetadataGetDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ Namespace: namespace, Subsystem: subsystem, Name: "secure_value_metadata_get_duration_seconds", Help: "Duration of secure value metadata get operations", Buckets: prometheus.DefBuckets, - }), - SecureValueMetadataGetCount: prometheus.NewCounter(prometheus.CounterOpts{ - Namespace: namespace, - Subsystem: subsystem, - Name: "secure_value_metadata_get_count", - Help: "Count of secure value metadata get operations", - }), - SecureValueMetadataListDuration: prometheus.NewHistogram(prometheus.HistogramOpts{ + }, []string{successLabel}), + SecureValueMetadataListDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ Namespace: namespace, Subsystem: subsystem, Name: "secure_value_metadata_list_duration_seconds", Help: "Duration of secure value metadata list operations", Buckets: prometheus.DefBuckets, - }), - SecureValueMetadataListCount: prometheus.NewCounter(prometheus.CounterOpts{ - Namespace: namespace, - Subsystem: subsystem, - Name: "secure_value_metadata_list_count", - Help: "Count of secure value metadata list operations", - }), - SecureValueSetExternalIDDuration: prometheus.NewHistogram(prometheus.HistogramOpts{ + }, []string{successLabel}), + SecureValueSetExternalIDDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ Namespace: namespace, Subsystem: subsystem, Name: "secure_value_set_external_id_duration_seconds", Help: "Duration of secure value set external id operations", Buckets: prometheus.DefBuckets, - }), - SecureValueSetStatusDuration: prometheus.NewHistogram(prometheus.HistogramOpts{ + }, []string{successLabel}), + SecureValueSetStatusDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ Namespace: namespace, Subsystem: subsystem, Name: "secure_value_set_status_duration_seconds", Help: "Duration of secure value set status operations", Buckets: prometheus.DefBuckets, - }), + }, []string{successLabel}), // Decrypt metrics DecryptDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ @@ -176,13 +121,7 @@ func newStorageMetrics() *StorageMetrics { Name: "decrypt_duration_seconds", Help: "Duration of decrypt operations", Buckets: prometheus.DefBuckets, - }, []string{"successful"}), - DecryptRequestCount: prometheus.NewCounterVec(prometheus.CounterOpts{ - Namespace: namespace, - Subsystem: subsystem, - Name: "decrypt_request_count", - Help: "Count of decrypt operations", - }, []string{"successful"}), + }, []string{successLabel}), } } @@ -199,26 +138,17 @@ func NewStorageMetrics(reg prometheus.Registerer) *StorageMetrics { if reg != nil { reg.MustRegister( m.KeeperMetadataCreateDuration, - m.KeeperMetadataCreateCount, m.KeeperMetadataUpdateDuration, - m.KeeperMetadataUpdateCount, m.KeeperMetadataDeleteDuration, - m.KeeperMetadataDeleteCount, m.KeeperMetadataGetDuration, - m.KeeperMetadataGetCount, m.KeeperMetadataListDuration, - m.KeeperMetadataListCount, m.KeeperMetadataGetKeeperConfigDuration, m.SecureValueMetadataCreateDuration, - m.SecureValueMetadataCreateCount, m.SecureValueMetadataGetDuration, - m.SecureValueMetadataGetCount, m.SecureValueMetadataListDuration, - m.SecureValueMetadataListCount, m.SecureValueSetExternalIDDuration, m.SecureValueSetStatusDuration, m.DecryptDuration, - m.DecryptRequestCount, ) } diff --git a/pkg/storage/secret/metadata/secure_value_store.go b/pkg/storage/secret/metadata/secure_value_store.go index 5782ee9a3e4..448d8ce2698 100644 --- a/pkg/storage/secret/metadata/secure_value_store.go +++ b/pkg/storage/secret/metadata/secure_value_store.go @@ -55,13 +55,14 @@ func (s *secureValueMetadataStorage) Create(ctx context.Context, sv *secretv1bet defer span.End() defer func() { + success := svmCreateErr == nil + args := []any{ "name", name, "namespace", namespace, "actorUID", actorUID, } - success := svmCreateErr == nil args = append(args, "success", success) if !success { span.SetStatus(codes.Error, "SecureValueMetadataStorage.Create failed") @@ -72,7 +73,6 @@ func (s *secureValueMetadataStorage) Create(ctx context.Context, sv *secretv1bet logging.FromContext(ctx).Info("SecureValueMetadataStorage.Create", args...) s.metrics.SecureValueMetadataCreateDuration.WithLabelValues(strconv.FormatBool(success)).Observe(time.Since(start).Seconds()) - s.metrics.SecureValueMetadataCreateCount.WithLabelValues(strconv.FormatBool(success)).Inc() }() // Set inside the transaction callback @@ -265,10 +265,22 @@ func (s *secureValueMetadataStorage) Read(ctx context.Context, namespace xkube.N defer span.End() defer func() { - logging.FromContext(ctx).Info("SecureValueMetadataStorage.Read", "namespace", namespace, "name", name, "success", readErr == nil, "error", readErr) + success := readErr == nil - s.metrics.SecureValueMetadataGetDuration.Observe(time.Since(start).Seconds()) - s.metrics.SecureValueMetadataGetCount.Inc() + args := []any{ + "name", name, + "namespace", namespace.String(), + "success", success, + } + + if !success { + span.SetStatus(codes.Error, "SecureValueMetadataStorage.Read failed") + span.RecordError(readErr) + args = append(args, "error", readErr) + } + + logging.FromContext(ctx).Info("SecureValueMetadataStorage.Read", args...) + s.metrics.SecureValueMetadataGetDuration.WithLabelValues(strconv.FormatBool(success)).Observe(time.Since(start).Seconds()) }() secureValue, err := s.readActiveVersion(ctx, namespace, name, opts) @@ -284,7 +296,7 @@ func (s *secureValueMetadataStorage) Read(ctx context.Context, namespace xkube.N return secureValueKub, nil } -func (s *secureValueMetadataStorage) List(ctx context.Context, namespace xkube.Namespace) (svList []secretv1beta1.SecureValue, error error) { +func (s *secureValueMetadataStorage) List(ctx context.Context, namespace xkube.Namespace) (svList []secretv1beta1.SecureValue, listErr error) { start := time.Now() ctx, span := s.tracer.Start(ctx, "SecureValueMetadataStorage.List", trace.WithAttributes( attribute.String("namespace", namespace.String()), @@ -292,7 +304,23 @@ func (s *secureValueMetadataStorage) List(ctx context.Context, namespace xkube.N defer span.End() defer func() { + success := listErr == nil span.SetAttributes(attribute.Int("returnedList.count", len(svList))) + + args := []any{ + "namespace", namespace.String(), + "success", success, + } + + if !success { + span.SetStatus(codes.Error, "SecureValueMetadataStorage.List failed") + span.RecordError(listErr) + args = append(args, "error", listErr) + } + + logging.FromContext(ctx).Info("SecureValueMetadataStorage.List", args...) + + s.metrics.SecureValueMetadataListDuration.WithLabelValues(strconv.FormatBool(success)).Observe(time.Since(start).Seconds()) }() req := listSecureValue{ @@ -344,9 +372,6 @@ func (s *secureValueMetadataStorage) List(ctx context.Context, namespace xkube.N return nil, fmt.Errorf("read rows error: %w", err) } - s.metrics.SecureValueMetadataListDuration.Observe(time.Since(start).Seconds()) - s.metrics.SecureValueMetadataListCount.Inc() - return secureValues, nil } @@ -423,7 +448,7 @@ func (s *secureValueMetadataStorage) SetVersionToInactive(ctx context.Context, n return nil } -func (s *secureValueMetadataStorage) SetExternalID(ctx context.Context, namespace xkube.Namespace, name string, version int64, externalID contracts.ExternalID) error { +func (s *secureValueMetadataStorage) SetExternalID(ctx context.Context, namespace xkube.Namespace, name string, version int64, externalID contracts.ExternalID) (setExtIDErr error) { start := time.Now() ctx, span := s.tracer.Start(ctx, "SecureValueMetadataStorage.SetExternalID", trace.WithAttributes( attribute.String("name", name), @@ -431,8 +456,29 @@ func (s *secureValueMetadataStorage) SetExternalID(ctx context.Context, namespac attribute.String("externalID", externalID.String()), attribute.Int64("version", version), )) + defer span.End() + defer func() { + success := setExtIDErr == nil + args := []any{ + "name", name, + "namespace", namespace.String(), + "success", success, + "version", strconv.FormatInt(version, 10), + "externalID", externalID.String(), + } + + if !success { + span.SetStatus(codes.Error, "SecureValueMetadataStorage.SetExternalID failed") + span.RecordError(setExtIDErr) + args = append(args, "error", setExtIDErr) + } + + logging.FromContext(ctx).Info("SecureValueMetadataStorage.SetExternalID", args...) + s.metrics.SecureValueSetExternalIDDuration.WithLabelValues(strconv.FormatBool(success)).Observe(time.Since(start).Seconds()) + }() + req := updateExternalIdSecureValue{ SQLTemplate: sqltemplate.New(s.dialect), Namespace: namespace.String(), @@ -459,7 +505,6 @@ func (s *secureValueMetadataStorage) SetExternalID(ctx context.Context, namespac if modifiedCount > 1 { return fmt.Errorf("secureValueMetadataStorage.SetExternalID: modified more than one secret, this is a bug, check the where condition: modifiedCount=%d", modifiedCount) } - s.metrics.SecureValueSetExternalIDDuration.Observe(time.Since(start).Seconds()) return nil }