From 7b9774709a1edd8a6703cfaf5346321eb86ffd44 Mon Sep 17 00:00:00 2001 From: Jack Warren Date: Tue, 31 Oct 2023 10:29:47 -0400 Subject: [PATCH] [DDO-3272] Move metrics code out of deprecated folders (#343) Co-authored-by: Katie Welch --- sherlock/internal/boot/application.go | 12 ++--- .../v2controllers/metrics_updater_test.go | 52 ------------------- .../metrics_cron.go} | 18 ++++--- sherlock/internal/models/metrics_cron_test.go | 11 ++++ 4 files changed, 29 insertions(+), 64 deletions(-) delete mode 100644 sherlock/internal/deprecated_controllers/v2controllers/metrics_updater_test.go rename sherlock/internal/{deprecated_models/v2models/metrics_updater.go => models/metrics_cron.go} (97%) create mode 100644 sherlock/internal/models/metrics_cron_test.go diff --git a/sherlock/internal/boot/application.go b/sherlock/internal/boot/application.go index 02eaa1edf..949f354ab 100644 --- a/sherlock/internal/boot/application.go +++ b/sherlock/internal/boot/application.go @@ -10,9 +10,9 @@ import ( "github.com/broadinstitute/sherlock/sherlock/internal/config" "github.com/broadinstitute/sherlock/sherlock/internal/db" "github.com/broadinstitute/sherlock/sherlock/internal/deployhooks" - "github.com/broadinstitute/sherlock/sherlock/internal/deprecated_models/v2models" "github.com/broadinstitute/sherlock/sherlock/internal/github" "github.com/broadinstitute/sherlock/sherlock/internal/metrics" + "github.com/broadinstitute/sherlock/sherlock/internal/models" "github.com/broadinstitute/sherlock/sherlock/internal/slack" "github.com/rs/zerolog/log" "gorm.io/gorm" @@ -94,15 +94,15 @@ func (a *Application) Start() { if config.Config.Bool("metrics.v2.enable") { log.Info().Msgf("BOOT | registering metric views...") if err = metrics.RegisterViews(); err != nil { - log.Fatal().Msgf("v2metrics.RegisterViews() err: %v", err) + log.Fatal().Msgf("metrics.RegisterViews() err: %v", err) } log.Info().Msgf("BOOT | calculating metric values...") - if err = v2models.UpdateMetrics(ctx, a.gormDB); err != nil { - log.Fatal().Msgf("v2models.UpdateMetrics() err: %v", err) + if err = models.UpdateMetrics(ctx, a.gormDB); err != nil { + log.Fatal().Msgf("models.UpdateMetrics() err: %v", err) } - go v2models.KeepMetricsUpdated(ctx, a.gormDB) + go models.KeepMetricsUpdated(ctx, a.gormDB) } if config.Config.Bool("slack.enable") { @@ -116,7 +116,7 @@ func (a *Application) Start() { if config.Config.Bool("github.enable") { log.Info().Msgf("BOOT | initializing GitHub client...") if err = github.Init(ctx); err != nil { - log.Fatal().Msgf("githubz.Init() err: %v", err) + log.Fatal().Msgf("github.Init() err: %v", err) } } diff --git a/sherlock/internal/deprecated_controllers/v2controllers/metrics_updater_test.go b/sherlock/internal/deprecated_controllers/v2controllers/metrics_updater_test.go deleted file mode 100644 index 634936828..000000000 --- a/sherlock/internal/deprecated_controllers/v2controllers/metrics_updater_test.go +++ /dev/null @@ -1,52 +0,0 @@ -package v2controllers - -import ( - "context" - "github.com/broadinstitute/sherlock/sherlock/internal/config" - "github.com/broadinstitute/sherlock/sherlock/internal/deprecated_db" - "github.com/broadinstitute/sherlock/sherlock/internal/deprecated_models/v2models" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/suite" - "gorm.io/gorm" - "testing" -) - -// v2's metrics code is entirely in the model, but we aren't able to run functional tests from the model because -// we get circular imports trying to connect to the database from there (database connection code is in another package -// that itself requires the model for migrations). That's not a problem anywhere else because we run functional tests -// via controller methods anyway. There isn't a controller component to v2's metrics, but our best option to run -// a basic "does the SQL validate" test is to run it from here similar to other functional tests. -// -// In other words, I (Jack) thinks having this rudimentary test not co-located is okay for now because literally all the -// other testing is on the happy path and I don't feel like fighting Go imports here. Maybe we'll add higher-level -// metrics code down the line and this will make sense as is, or once v1 is gone maybe the database connection code -// can live in the model itself and then we can move this there. - -func TestMetricsUpdaterSuite(t *testing.T) { - if testing.Short() { - t.Skip("skipping functional test") - } - suite.Run(t, new(metricsUpdaterSuite)) -} - -type metricsUpdaterSuite struct { - suite.Suite - db *gorm.DB -} - -func (suite *metricsUpdaterSuite) SetupSuite() { - config.LoadTestConfig() - suite.db = deprecated_db.ConnectAndConfigureFromTest(suite.T()) -} - -func (suite *metricsUpdaterSuite) TearDownSuite() { - deprecated_db.Truncate(suite.T(), suite.db) -} - -func (suite *metricsUpdaterSuite) TestUpdateMetrics() { - suite.Run("doesn't error when running custom SQL", func() { - deprecated_db.Truncate(suite.T(), suite.db) - err := v2models.UpdateMetrics(context.Background(), suite.db) - assert.NoError(suite.T(), err) - }) -} diff --git a/sherlock/internal/deprecated_models/v2models/metrics_updater.go b/sherlock/internal/models/metrics_cron.go similarity index 97% rename from sherlock/internal/deprecated_models/v2models/metrics_updater.go rename to sherlock/internal/models/metrics_cron.go index 2aec47c79..715c77677 100644 --- a/sherlock/internal/deprecated_models/v2models/metrics_updater.go +++ b/sherlock/internal/models/metrics_cron.go @@ -1,4 +1,4 @@ -package v2models +package models import ( "context" @@ -189,7 +189,7 @@ group by result_per_version.chart_release_id } func reportDataTypeCounts(ctx context.Context, db *gorm.DB) error { - for dataType, model := range map[string]Model{ + for dataType, model := range map[string]any{ "chart": Chart{}, "environment": Environment{}, "cluster": Cluster{}, @@ -359,7 +359,10 @@ func UpdateMetrics(ctx context.Context, db *gorm.DB) error { return err } - charts, err := InternalChartStore.ListAllMatchingByUpdated(db, 0, &Chart{}) + var charts []Chart + if err = db.Model(&Chart{}).Order("updated_at desc").Find(&charts).Error; err != nil { + return err + } if err != nil { return err } @@ -392,7 +395,10 @@ func UpdateMetrics(ctx context.Context, db *gorm.DB) error { stats.Record(ctx, metrics.AppVersionCountMeasure.M(count)) } - chartReleases, err := InternalChartReleaseStore.ListAllMatchingByUpdated(db, 0, &ChartRelease{ChartID: chart.ID}) + var chartReleases []ChartRelease + if err = db.Model(&ChartRelease{}).Where(&ChartRelease{ChartID: chart.ID}).Order("updated_at desc").Find(&chartReleases).Error; err != nil { + return err + } if err != nil { return err } @@ -462,7 +468,7 @@ func UpdateMetrics(ctx context.Context, db *gorm.DB) error { } lastUpdateTime = time.Now() - log.Info().Msgf("MTRC | v2 metrics updated, took %s", lastUpdateTime.Sub(updateStartTime).String()) + log.Info().Msgf("MTRC | metrics updated, took %s", lastUpdateTime.Sub(updateStartTime).String()) return nil } @@ -471,7 +477,7 @@ func KeepMetricsUpdated(ctx context.Context, db *gorm.DB) { for { time.Sleep(interval) if err := UpdateMetrics(ctx, db); err != nil { - log.Warn().Err(err).Msgf("failed to update v2 metrics, now %s stale", time.Since(lastUpdateTime).String()) + log.Warn().Err(err).Msgf("MTRC | failed to update metrics, now %s stale", time.Since(lastUpdateTime).String()) } } } diff --git a/sherlock/internal/models/metrics_cron_test.go b/sherlock/internal/models/metrics_cron_test.go new file mode 100644 index 000000000..42af2ad94 --- /dev/null +++ b/sherlock/internal/models/metrics_cron_test.go @@ -0,0 +1,11 @@ +package models + +import ( + "context" +) + +func (s *modelSuite) TestUpdateMetrics() { + s.Run("doesn't error when running custom SQL", func() { + s.NoError(UpdateMetrics(context.Background(), s.DB)) + }) +}