Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add env to disable metrics operation when it is disabled #96

Closed
wants to merge 40 commits into from
Closed
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
2f3b6aa
feat: add env to disable metrics operation when it is disabled
boodyvo Sep 6, 2024
6feb607
feat: add env to disable metrics operation when it is disabled
boodyvo Sep 6, 2024
9fa07f2
feat: move to the same master code to check if tests are still working
boodyvo Sep 6, 2024
f50590b
feat: wip: intermediate commit
boodyvo Sep 9, 2024
a9546e6
feat: wip: add support to remove database
boodyvo Sep 10, 2024
65958fc
feat: wip: add support to remove database
boodyvo Sep 10, 2024
f1eda9a
feat: fix ci
boodyvo Sep 10, 2024
9766160
feat: update ci
boodyvo Sep 10, 2024
4ab9f9c
feat: update ci
boodyvo Sep 10, 2024
1900257
feat: remove leftovers
boodyvo Sep 10, 2024
3f6e629
feat: fix metrics tests for local environment
boodyvo Sep 10, 2024
1297dc2
feat: add local ci and fixed local e2e tests
boodyvo Sep 10, 2024
41c342f
feat: add local ci and fixed local e2e tests
boodyvo Sep 10, 2024
7f622c6
feat: add local ci and fixed local e2e tests
boodyvo Sep 10, 2024
d97f8c7
feat: add local ci and fixed local e2e tests
boodyvo Sep 10, 2024
25c1213
feat: tests updates for local env
boodyvo Sep 10, 2024
f13e41f
feat: tests updates for local env
boodyvo Sep 10, 2024
44cb8a9
feat: tests updates for local env
boodyvo Sep 10, 2024
60e55ed
feat: updates for the comments
boodyvo Sep 12, 2024
25cd2fd
feat: update tests to make it possible to skip metrics
boodyvo Sep 12, 2024
a37ea53
feat: update env passing for no metrics ci
boodyvo Sep 12, 2024
849c8d0
feat: update env passing for no metrics ci
boodyvo Sep 12, 2024
28bdc4b
feat: update env passing for no metrics ci
boodyvo Sep 12, 2024
afa086d
feat: test no metrics
boodyvo Sep 12, 2024
6ac2ee2
feat: test no metrics
boodyvo Sep 12, 2024
461c9fc
feat: test metrics for proxy passing env
boodyvo Sep 12, 2024
1e4f891
feat: test metrics for proxy passing env
boodyvo Sep 12, 2024
b05ed6d
feat: test metrics for proxy passing env
boodyvo Sep 12, 2024
7ffbc0e
feat: test metrics for proxy passing env
boodyvo Sep 12, 2024
5d429f0
feat: test metrics for proxy passing env
boodyvo Sep 12, 2024
77f20a4
feat: test metrics for proxy passing env
boodyvo Sep 12, 2024
2f34eed
feat: test metrics for proxy passing env
boodyvo Sep 13, 2024
29f31e2
feat: remove extra env
boodyvo Sep 16, 2024
36049ba
feat: add variable back
boodyvo Sep 16, 2024
d1ea3cb
feat: add variable back
boodyvo Sep 16, 2024
74142d0
feat: add comment
boodyvo Sep 16, 2024
46912b6
feat: add comment
boodyvo Sep 16, 2024
c242326
feat: add comment
boodyvo Sep 16, 2024
9220e53
feat: add comment
boodyvo Sep 16, 2024
cd9b0d0
feat: update envs
boodyvo Sep 16, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions .github/workflows/ci-e2e-no-metrics-tests.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
name: Continuous Integration (E2E Testing Checks without metrics database)

on:
workflow_call:
jobs:
e2e-no-metrics-test:
runs-on: ubuntu-latest
steps:
- name: checkout repo from current commit
uses: actions/checkout@v3
- name: set up Go
uses: actions/setup-go@v3
with:
go-version: "1.21"
check-latest: true
cache: false
- name: pull pre-built images
run: sudo docker compose -f ci.docker-compose.yml pull
- name: build and start proxy service and it's dependencies
run: sudo docker compose -f ci.docker-compose.yml --env-file .env --env-file no_metric.env up -d --build proxy redis
- name: wait for proxy service to be running
run: bash ${GITHUB_WORKSPACE}/scripts/wait-for-proxy-service-running.sh
env:
PROXY_CONTAINER_PORT: 7777
- name: run e2e tests
run: SKIP_METRICS=true make e2e-test
- name: print proxy service logs
run: sudo docker compose -f ci.docker-compose.yml logs proxy
# because we especially want the logs if the test(s) fail 😅
if: always()
# Finally, "Post Run jpribyl/[email protected]",
boodyvo marked this conversation as resolved.
Show resolved Hide resolved
# which is the process of saving the cache, will be executed.
6 changes: 5 additions & 1 deletion .github/workflows/ci-main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,14 @@ jobs:
# run default ci checks against main branch
default-checks:
uses: ./.github/workflows/ci-default.yml
# run e2e testing ci checks against main branch
# run e2e testing ci for internal testnet checks against main branch
e2e-tests:
needs: [lint-checks, default-checks]
uses: ./.github/workflows/ci-e2e-tests.yml
# run e2e testing without metrics db ci checks against main branch
e2e-no-metrics-tests:
needs: [lint-checks, default-checks]
uses: ./.github/workflows/ci-e2e-no-metrics-tests.yml
# build, tag and publish new service docker images
release-docker-images:
needs: [e2e-tests]
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/ci-pr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,5 @@ jobs:
uses: ./.github/workflows/ci-default.yml
e2e-tests:
uses: ./.github/workflows/ci-e2e-tests.yml
e2e-no-metrics-tests:
uses: ./.github/workflows/ci-e2e-no-metrics-tests.yml
5 changes: 5 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ unit-test:
e2e-test:
go test -count=1 -v -cover -coverprofile cover.out --race ./... -run "^TestE2ETest*"

.PHONY: e2e-test-no-metrics
# run tests that execute against a local or remote instance of the API without database for metrics
e2e-test-no-metrics:
SKIP_METRICS=true go test -count=1 -v -cover -coverprofile cover.out --race ./... -run "^TestE2ETest*"

.PHONY: ci-setup
# set up your local environment such that running `make e2e-test` runs against testnet (like in CI)
ci-setup:
Expand Down
2 changes: 1 addition & 1 deletion ci.docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
services:
# run postgres for proxy service to store observability metrics
postgres:
image: postgres:15
image: postgres:13.12
env_file: .env
ports:
- "${POSTGRES_HOST_PORT}:${POSTGRES_CONTAINER_PORT}"
Expand Down
4 changes: 4 additions & 0 deletions clients/database/database.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@ import (
// that haven't been run on the database being used by the proxy service
// returning error (if any) and a list of migrations that have been
// run and any that were not
// If db is nil, returns empty slice and nil error, as there is no database to migrate.
func Migrate(ctx context.Context, db *bun.DB, migrations migrate.Migrations, logger *logging.ServiceLogger) (*migrate.MigrationSlice, error) {
if db == nil {
return &migrate.MigrationSlice{}, nil
}
// set up migration config
migrator := migrate.NewMigrator(db, &migrations)

Expand Down
14 changes: 14 additions & 0 deletions clients/database/database_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package database

import (
"context"
"github.com/stretchr/testify/require"
"github.com/uptrace/bun/migrate"
"testing"
)

func TestMigrateNoDatabase(t *testing.T) {
migrations, err := Migrate(context.Background(), nil, migrate.Migrations{}, nil)
require.NoError(t, err)
require.Empty(t, migrations)
}
20 changes: 17 additions & 3 deletions clients/database/postgres.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,15 @@ import (
// PostgresDatabaseConfig contains values for creating a
// new connection to a postgres database
type PostgresDatabaseConfig struct {
// DatabaseDisabled is used to disable the database, and it won't be used at all. All operations will be skipped.
DatabaseDisabled bool

DatabaseName string
DatabaseEndpointURL string
DatabaseUsername string
DatabasePassword string
ReadTimeoutSeconds int64
WriteTimeousSeconds int64
WriteTimeoutSeconds int64
DatabaseMaxIdleConnections int64
DatabaseConnectionMaxIdleSeconds int64
DatabaseMaxOpenConnections int64
Expand All @@ -33,12 +36,19 @@ type PostgresDatabaseConfig struct {

// PostgresClient wraps a connection to a postgres database
type PostgresClient struct {
isDisabled bool
*bun.DB
}

// NewPostgresClient returns a new connection to the specified
// postgres data and error (if any)
func NewPostgresClient(config PostgresDatabaseConfig) (PostgresClient, error) {
if config.DatabaseDisabled {
return PostgresClient{
isDisabled: true,
}, nil
}

// configure postgres database connection options
var pgOptions *pgdriver.Connector

Expand All @@ -54,7 +64,7 @@ func NewPostgresClient(config PostgresDatabaseConfig) (PostgresClient, error) {
pgdriver.WithPassword(config.DatabasePassword),
pgdriver.WithDatabase(config.DatabaseName),
pgdriver.WithReadTimeout(time.Second*time.Duration(config.ReadTimeoutSeconds)),
pgdriver.WithWriteTimeout(time.Second*time.Duration(config.WriteTimeousSeconds)),
pgdriver.WithWriteTimeout(time.Second*time.Duration(config.WriteTimeoutSeconds)),
)
} else {
pgOptions = pgdriver.NewConnector(
Expand All @@ -64,7 +74,7 @@ func NewPostgresClient(config PostgresDatabaseConfig) (PostgresClient, error) {
pgdriver.WithPassword(config.DatabasePassword),
pgdriver.WithDatabase(config.DatabaseName),
pgdriver.WithReadTimeout(time.Second*time.Duration(config.ReadTimeoutSeconds)),
pgdriver.WithWriteTimeout(time.Second*time.Duration(config.WriteTimeousSeconds)),
pgdriver.WithWriteTimeout(time.Second*time.Duration(config.WriteTimeoutSeconds)),
)
}

Expand Down Expand Up @@ -94,5 +104,9 @@ func NewPostgresClient(config PostgresDatabaseConfig) (PostgresClient, error) {
// HealthCheck returns an error if the database can not
// be connected to and queried, nil otherwise
func (pg *PostgresClient) HealthCheck() error {
if pg.isDisabled {
return nil
}

return pg.Ping()
}
25 changes: 25 additions & 0 deletions clients/database/postgres_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package database

import (
"github.com/stretchr/testify/require"
"testing"
)

func TestDisabledDBCreation(t *testing.T) {
config := PostgresDatabaseConfig{
DatabaseDisabled: true,
}
db, err := NewPostgresClient(config)
require.NoError(t, err)
require.True(t, db.isDisabled)
}

func TestHealthcheckNoDatabase(t *testing.T) {
config := PostgresDatabaseConfig{
DatabaseDisabled: true,
}
db, err := NewPostgresClient(config)
require.NoError(t, err)
err = db.HealthCheck()
require.NoError(t, err)
}
33 changes: 29 additions & 4 deletions clients/database/request_metric.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,13 @@ type ProxiedRequestMetric struct {
}

// Save saves the current ProxiedRequestMetric to
// the database, returning error (if any)
// the database, returning error (if any).
// If db is nil, returns nil error.
func (prm *ProxiedRequestMetric) Save(ctx context.Context, db *bun.DB) error {
if db == nil {
return nil
}
Comment on lines +40 to +42

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: any reason we don't want to return an explicit error rather than a silent return?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh, this is to avoid refactoring. The implementation we have in not methods-based, but expects that we pass the db. To remove db dependency during initialization we return not fake, but nil db and pass it. As it is expected (and added to the comments), we allow to pass db as nil. In such case we don't need errors.

Another ways to do that:

  • switch to interface and add empty implementation, that does nothing
  • return particulr errors, but in this case we need to refactor processing among the codebase

This helps us to keep the current metrics in the code, while we don't have the decision about about how do we want to implement metrics

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an additional further PR (already created), that we discussed with @pirtleshell and in the task: https://app.shortcut.com/kava-labs/story/14336
This one provide a better refactoring, adding interfaces.


_, err := db.NewInsert().Model(prm).Exec(ctx)

return err
Expand All @@ -44,8 +49,13 @@ func (prm *ProxiedRequestMetric) Save(ctx context.Context, db *bun.DB) error {
// ListProxiedRequestMetricsWithPagination returns a page of max
// `limit` ProxiedRequestMetrics from the offset specified by`cursor`
// error (if any) along with a cursor to use to fetch the next page
// if the cursor is 0 no more pages exists
// if the cursor is 0 no more pages exists.
// Uses only in tests. If db is nil, returns empty slice and 0 cursor.
func ListProxiedRequestMetricsWithPagination(ctx context.Context, db *bun.DB, cursor int64, limit int) ([]ProxiedRequestMetric, int64, error) {
if db == nil {
return []ProxiedRequestMetric{}, 0, nil
}

var proxiedRequestMetrics []ProxiedRequestMetric
var nextCursor int64

Expand All @@ -62,8 +72,13 @@ func ListProxiedRequestMetricsWithPagination(ctx context.Context, db *bun.DB, cu

// CountAttachedProxiedRequestMetricPartitions returns the current
// count of attached partitions for the ProxiedRequestMetricsTableName
// and error (if any)
// and error (if any).
// If db is nil, returns 0 and nil error.
func CountAttachedProxiedRequestMetricPartitions(ctx context.Context, db *bun.DB) (int64, error) {
if db == nil {
return 0, nil
}

var count int64

countPartitionsQuery := fmt.Sprintf(`
Expand All @@ -88,7 +103,12 @@ func CountAttachedProxiedRequestMetricPartitions(ctx context.Context, db *bun.DB

// GetLastCreatedAttachedProxiedRequestMetricsPartitionName gets the table name
// for the last created (and attached) proxied request metrics partition
// Used for status check. If db is nil, returns empty string and nil error.
func GetLastCreatedAttachedProxiedRequestMetricsPartitionName(ctx context.Context, db *bun.DB) (string, error) {
if db == nil {
return "", nil
}

var lastCreatedAttachedPartitionName string

lastCreatedAttachedPartitionNameQuery := fmt.Sprintf(`
Expand All @@ -114,8 +134,13 @@ WHERE parent.relname='%s' order by child.oid desc limit 1;`, ProxiedRequestMetri

// DeleteProxiedRequestMetricsOlderThanNDays deletes
// all proxied request metrics older than the specified
// days, returning error (if any)
// days, returning error (if any).
// Used during pruning process. If db is nil, returns nil error.
func DeleteProxiedRequestMetricsOlderThanNDays(ctx context.Context, db *bun.DB, n int64) error {
if db == nil {
return nil
}

_, err := db.NewDelete().Model((*ProxiedRequestMetric)(nil)).Where(fmt.Sprintf("request_time < now() - interval '%d' day", n)).Exec(ctx)

return err
Expand Down
37 changes: 37 additions & 0 deletions clients/database/request_metric_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package database

import (
"context"
"github.com/stretchr/testify/require"
"testing"
)

func TestNoDatabaseSave(t *testing.T) {
prm := ProxiedRequestMetric{}
err := prm.Save(context.Background(), nil)
require.NoError(t, err)
}

func TestNoDatabaseListProxiedRequestMetricsWithPagination(t *testing.T) {
proxiedRequestMetrics, cursor, err := ListProxiedRequestMetricsWithPagination(context.Background(), nil, 0, 0)
require.NoError(t, err)
require.Empty(t, proxiedRequestMetrics)
require.Zero(t, cursor)
}

func TestNoDatabaseCountAttachedProxiedRequestMetricPartitions(t *testing.T) {
count, err := CountAttachedProxiedRequestMetricPartitions(context.Background(), nil)
require.NoError(t, err)
require.Zero(t, count)
}

func TestGetLastCreatedAttachedProxiedRequestMetricsPartitionName(t *testing.T) {
partitionName, err := GetLastCreatedAttachedProxiedRequestMetricsPartitionName(context.Background(), nil)
require.NoError(t, err)
require.Empty(t, partitionName)
}

func TestDeleteProxiedRequestMetricsOlderThanNDays(t *testing.T) {
err := DeleteProxiedRequestMetricsOlderThanNDays(context.Background(), nil, 0)
require.NoError(t, err)
}
4 changes: 4 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ type Config struct {
MetricPruningRoutineInterval time.Duration
MetricPruningRoutineDelayFirstRun time.Duration
MetricPruningMaxRequestMetricsHistoryDays int
MetricDatabaseEnabled bool
CacheEnabled bool
RedisEndpointURL string
RedisPassword string
Expand Down Expand Up @@ -101,6 +102,8 @@ const (
DEFAULT_METRIC_PRUNING_ENABLED = true
METRIC_PRUNING_ROUTINE_INTERVAL_SECONDS_ENVIRONMENT_KEY = "METRIC_PRUNING_ROUTINE_INTERVAL_SECONDS"
// 60 seconds * 60 minutes * 24 hours = 1 day
METRIC_DATABASE_ENABLED_ENVIRONMENT_KEY = "METRIC_DATABASE_ENABLED"
DEFAULT_METRIC_DATABASE_ENABLED = true
DEFAULT_METRIC_PRUNING_ROUTINE_INTERVAL_SECONDS = 86400
METRIC_PRUNING_ROUTINE_DELAY_FIRST_RUN_SECONDS_ENVIRONMENT_KEY = "METRIC_PRUNING_ROUTINE_DELAY_FIRST_RUN_SECONDS"
DEFAULT_METRIC_PRUNING_ROUTINE_DELAY_FIRST_RUN_SECONDS = 10
Expand Down Expand Up @@ -380,6 +383,7 @@ func ReadConfig() Config {
MetricPruningRoutineInterval: time.Duration(time.Duration(EnvOrDefaultInt(METRIC_PRUNING_ROUTINE_INTERVAL_SECONDS_ENVIRONMENT_KEY, DEFAULT_METRIC_PRUNING_ROUTINE_INTERVAL_SECONDS)) * time.Second),
MetricPruningRoutineDelayFirstRun: time.Duration(time.Duration(EnvOrDefaultInt(METRIC_PRUNING_ROUTINE_DELAY_FIRST_RUN_SECONDS_ENVIRONMENT_KEY, DEFAULT_METRIC_PRUNING_ROUTINE_DELAY_FIRST_RUN_SECONDS)) * time.Second),
MetricPruningMaxRequestMetricsHistoryDays: EnvOrDefaultInt(METRIC_PRUNING_MAX_REQUEST_METRICS_HISTORY_DAYS_ENVIRONMENT_KEY, DEFAULT_METRIC_PRUNING_MAX_REQUEST_METRICS_HISTORY_DAYS),
MetricDatabaseEnabled: EnvOrDefaultBool(METRIC_DATABASE_ENABLED_ENVIRONMENT_KEY, DEFAULT_METRIC_DATABASE_ENABLED),
CacheEnabled: EnvOrDefaultBool(CACHE_ENABLED_ENVIRONMENT_KEY, false),
RedisEndpointURL: os.Getenv(REDIS_ENDPOINT_URL_ENVIRONMENT_KEY),
RedisPassword: os.Getenv(REDIS_PASSWORD_ENVIRONMENT_KEY),
Expand Down
2 changes: 1 addition & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
services:
# run postgres for proxy service to store observability metrics
postgres:
image: postgres:15
image: postgres:13.12
evgeniy-scherbina marked this conversation as resolved.
Show resolved Hide resolved
env_file: .env
ports:
- "${POSTGRES_HOST_PORT}:${POSTGRES_CONTAINER_PORT}"
Expand Down
15 changes: 14 additions & 1 deletion main.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ func init() {
}

func startMetricPartitioningRoutine(serviceConfig config.Config, service service.ProxyService, serviceLogger logging.ServiceLogger) <-chan error {
if !serviceConfig.MetricDatabaseEnabled {
serviceLogger.Info().Msg("skipping starting metric partitioning routine since it is disabled via config")

return nil
}

metricPartitioningRoutineConfig := routines.MetricPartitioningRoutineConfig{
Interval: serviceConfig.MetricPartitioningRoutineInterval,
DelayFirstRun: serviceConfig.MetricPartitioningRoutineDelayFirstRun,
Expand Down Expand Up @@ -67,6 +73,12 @@ func startMetricPartitioningRoutine(serviceConfig config.Config, service service
}

func startMetricCompactionRoutine(serviceConfig config.Config, service service.ProxyService, serviceLogger logging.ServiceLogger) <-chan error {
if !serviceConfig.MetricDatabaseEnabled {
serviceLogger.Info().Msg("skipping starting metric compaction routine since it is disabled via config")

return nil
}

metricCompactionRoutineConfig := routines.MetricCompactionRoutineConfig{
Interval: serviceConfig.MetricCompactionRoutineInterval,
Database: service.Database,
Expand Down Expand Up @@ -95,8 +107,9 @@ func startMetricCompactionRoutine(serviceConfig config.Config, service service.P
}

func startMetricPruningRoutine(serviceConfig config.Config, service service.ProxyService, serviceLogger logging.ServiceLogger) <-chan error {
if !serviceConfig.MetricPruningEnabled {
if !serviceConfig.MetricPruningEnabled || !serviceConfig.MetricDatabaseEnabled {
serviceLogger.Info().Msg("skipping starting metric pruning routine since it is disabled via config")

return make(<-chan error)
}

Expand Down
5 changes: 5 additions & 0 deletions main_batch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ func TestE2ETest_ValidBatchEvmRequests(t *testing.T) {

for _, tc := range testCases {
startTime := time.Now()
time.Sleep(1 * time.Second) // ensure startTime will be far from metrics starting
t.Run(tc.name, func(t *testing.T) {
reqInJSON, err := json.Marshal(tc.req)
require.NoError(t, err)
Expand Down Expand Up @@ -230,6 +231,10 @@ func TestE2ETest_ValidBatchEvmRequests(t *testing.T) {
require.Equal(t, resp.Header[accessControlAllowOriginHeaderName], []string{"*"})
}

if shouldSkipMetrics() {
return
}

// wait for all metrics to be created.
// besides verification, waiting for the metrics ensures future tests don't fail b/c metrics are being processed
waitForMetricsInWindow(t, tc.expectedNumMetrics, db, startTime, []string{})
Expand Down
Loading
Loading