From ec161baa9ef082392f1d0cc94c58531a84b5b24e Mon Sep 17 00:00:00 2001 From: Dave Page Date: Fri, 7 Aug 2026 15:54:55 +0100 Subject: [PATCH] Remove dead code that needed test edits rather than deletions The third and final pass over the reachability findings. The earlier mechanical pass deferred 42 functions because deleting them broke tests that also cover live code, so those needed the tests editing rather than removing. Working through them individually, 18 turned out to be genuinely removable and the rest did not. The largest group is the alerter's notification-channel write API: ten methods covering channel create, update and delete, email recipients, connection links, notification history and reminder state, none of which has a caller. The alerter reads notification channels; the server owns every write. Those went along with the five integration tests that existed solely to exercise them, and their assertions in the shared closed-pool error-path test were excised individually so the remaining assertions still run. The rest are a datastore pool accessor, a monitored-pool max-connections getter, two compaction analytics reporters, a probe-availability lookup, and three session tracing helpers. Twenty-four of the 42 are deliberately left in place, because deleting them would have meant weakening tests that exercise live code: - Injection seams that exist so tests can substitute fakes: AlertHandler.setAlertResolver and setUnacknowledgeFn, newVisibilityListerWithSource, NewWebhookNotifierAllowInternal (whose sibling's doc comment explicitly points at it for localhost testing), and overview.NewHandler. - Accessors their own tests depend on: CompactionCache.Clear and ReloadableConfig.Get, both of which had tests that silently inverted when the call was excised, plus Hub.Count and GetAnomalyCandidateByID, which verifies a live write path. - The logging level setters SetLevel and SetLogLevel. Both packages default to a level that suppresses output, so their tests need the setter to exercise filtering of live logging code at all. - MessageClass.String, a Stringer used for test diagnostics. - AuthHandler.Close, which stops h.totalRateLimiter and is called by nothing in cmd/mcp-server. That looks like a leaked goroutine in production rather than dead code, and wants fixing rather than deleting; noted for a follow-up. Four more are blocked until the earlier branch merges, because the only remaining callers are themselves being deleted there: IsAPITokenFromContext (called from access.go), Logger.Trace (from LogQueryTrace) and cron's Parser.Validate (from the package-level Validate). Coverage: collector 85.6% (from 85.5%), server 55.5% (unchanged), alerter 64.4% (from 65.9%). The alerter drop is the arithmetic of removing well-covered code, and it stays above the 60% repository goal. The only failing tests are the two pre-existing vector(3) fixture failures from #337, which reproduce identically on unmodified main. --- alerter/src/internal/database/datastore.go | 5 - .../src/internal/database/datastore_test.go | 10 - .../database/error_paths_integration_test.go | 48 --- .../internal/database/notification_queries.go | 193 ---------- ...ification_queries_full_integration_test.go | 345 ------------------ alerter/src/internal/database/queries.go | 20 - .../database/queries_full_integration_test.go | 26 -- collector/src/database/monitored_pool.go | 7 - collector/src/database/monitored_pool_test.go | 9 - docs/changelog.md | 12 + server/src/internal/compactor/analytics.go | 33 -- .../src/internal/compactor/analytics_test.go | 120 ------ server/src/internal/tracing/tracer.go | 28 -- server/src/internal/tracing/tracer_test.go | 9 - 14 files changed, 12 insertions(+), 853 deletions(-) diff --git a/alerter/src/internal/database/datastore.go b/alerter/src/internal/database/datastore.go index f14e7f37..639238bc 100644 --- a/alerter/src/internal/database/datastore.go +++ b/alerter/src/internal/database/datastore.go @@ -70,8 +70,3 @@ func (d *Datastore) Close() { d.pool.Close() } } - -// Pool returns the underlying connection pool -func (d *Datastore) Pool() *pgxpool.Pool { - return d.pool -} diff --git a/alerter/src/internal/database/datastore_test.go b/alerter/src/internal/database/datastore_test.go index 938be463..c21cd919 100644 --- a/alerter/src/internal/database/datastore_test.go +++ b/alerter/src/internal/database/datastore_test.go @@ -208,15 +208,8 @@ func TestDatastoreCloseNilPool(t *testing.T) { // TestDatastorePoolAccessor tests the Pool accessor method func TestDatastorePoolAccessor(t *testing.T) { - ds := &Datastore{ - pool: nil, - config: nil, - } // Pool should return nil when no pool is set - if ds.Pool() != nil { - t.Errorf("Pool() should return nil for uninitialized datastore") - } } // TestNewDatastoreSuccess exercises the happy path through NewDatastore @@ -251,9 +244,6 @@ func TestNewDatastoreSuccess(t *testing.T) { t.Fatalf("NewDatastore: %v", err) } defer ds.Close() - if ds.Pool() == nil { - t.Errorf("expected non-nil pool from NewDatastore") - } } // TestNewDatastoreInvalidConfig exercises the parse-failure branch of diff --git a/alerter/src/internal/database/error_paths_integration_test.go b/alerter/src/internal/database/error_paths_integration_test.go index 910543e1..0b44ce7b 100644 --- a/alerter/src/internal/database/error_paths_integration_test.go +++ b/alerter/src/internal/database/error_paths_integration_test.go @@ -82,9 +82,6 @@ func TestQueriesReturnErrorOnClosedPool(t *testing.T) { if _, err := ds.DeleteOldAnomalyCandidates(ctx, time.Now()); err == nil { t.Errorf("DeleteOldAnomalyCandidates should error on closed pool") } - if _, err := ds.GetProbeAvailability(ctx, 1, "x"); err == nil { - t.Errorf("GetProbeAvailability should error on closed pool") - } if _, err := ds.GetEnabledBlackoutSchedules(ctx); err == nil { t.Errorf("GetEnabledBlackoutSchedules should error on closed pool") } @@ -206,54 +203,15 @@ func TestNotificationQueriesReturnErrorOnClosedPool(t *testing.T) { defer cleanup() ctx := context.Background() - owner := "tester" - ch := &NotificationChannel{ - OwnerUsername: &owner, - Enabled: true, - ChannelType: ChannelTypeWebhook, - Name: "x", - HTTPMethod: "POST", - Headers: map[string]string{}, - SMTPPort: 587, - SMTPUseTLS: true, - ReminderEnabled: true, - ReminderIntervalHours: 1, - CreatedAt: time.Now(), - UpdatedAt: time.Now(), - } if _, err := ds.GetNotificationChannel(ctx, 1); err == nil { t.Errorf("GetNotificationChannel should error on closed pool") } if _, err := ds.GetNotificationChannelsForConnection(ctx, 1); err == nil { t.Errorf("GetNotificationChannelsForConnection should error on closed pool") } - if err := ds.CreateNotificationChannel(ctx, ch); err == nil { - t.Errorf("CreateNotificationChannel should error on closed pool") - } - if err := ds.UpdateNotificationChannel(ctx, ch); err == nil { - t.Errorf("UpdateNotificationChannel should error on closed pool") - } - if err := ds.DeleteNotificationChannel(ctx, 1); err == nil { - t.Errorf("DeleteNotificationChannel should error on closed pool") - } if _, err := ds.GetEmailRecipients(ctx, 1); err == nil { t.Errorf("GetEmailRecipients should error on closed pool") } - if err := ds.CreateEmailRecipient(ctx, &EmailRecipient{}); err == nil { - t.Errorf("CreateEmailRecipient should error on closed pool") - } - if err := ds.DeleteEmailRecipient(ctx, 1); err == nil { - t.Errorf("DeleteEmailRecipient should error on closed pool") - } - if err := ds.LinkConnectionToChannel(ctx, &ConnectionNotificationChannel{}); err == nil { - t.Errorf("LinkConnectionToChannel should error on closed pool") - } - if err := ds.UnlinkConnectionFromChannel(ctx, 1, 1); err == nil { - t.Errorf("UnlinkConnectionFromChannel should error on closed pool") - } - if _, err := ds.GetConnectionChannelLinks(ctx, 1); err == nil { - t.Errorf("GetConnectionChannelLinks should error on closed pool") - } if err := ds.CreateNotificationHistory(ctx, &NotificationHistory{}); err == nil { t.Errorf("CreateNotificationHistory should error on closed pool") } @@ -263,12 +221,6 @@ func TestNotificationQueriesReturnErrorOnClosedPool(t *testing.T) { if _, err := ds.GetPendingNotifications(ctx); err == nil { t.Errorf("GetPendingNotifications should error on closed pool") } - if _, err := ds.GetNotificationHistoryForAlert(ctx, 1); err == nil { - t.Errorf("GetNotificationHistoryForAlert should error on closed pool") - } - if _, err := ds.GetReminderState(ctx, 1, 1); err == nil { - t.Errorf("GetReminderState should error on closed pool") - } if err := ds.UpsertReminderState(ctx, &NotificationReminderState{}); err == nil { t.Errorf("UpsertReminderState should error on closed pool") } diff --git a/alerter/src/internal/database/notification_queries.go b/alerter/src/internal/database/notification_queries.go index 37671faf..7cc17ff4 100644 --- a/alerter/src/internal/database/notification_queries.go +++ b/alerter/src/internal/database/notification_queries.go @@ -127,70 +127,6 @@ func (d *Datastore) GetNotificationChannelsForConnection(ctx context.Context, co return channels, nil } -// CreateNotificationChannel inserts a new notification channel -func (d *Datastore) CreateNotificationChannel(ctx context.Context, channel *NotificationChannel) error { - return d.pool.QueryRow(ctx, ` - INSERT INTO notification_channels ( - owner_username, owner_token, enabled, channel_type, name, - description, webhook_url_encrypted, endpoint_url, http_method, - headers_json, auth_type, auth_credentials_encrypted, smtp_host, - smtp_port, smtp_username, smtp_password_encrypted, - smtp_use_tls, from_address, from_name, template_alert_fire, - template_alert_clear, template_reminder, reminder_enabled, - reminder_interval_hours, is_estate_default, created_at, updated_at - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, - $16, $17, $18, $19, $20, $21, $22, $23, $24, $25, $26, $27) - RETURNING id - `, channel.OwnerUsername, channel.OwnerToken, channel.Enabled, - channel.ChannelType, channel.Name, channel.Description, channel.WebhookURL, - channel.EndpointURL, channel.HTTPMethod, channel.Headers, channel.AuthType, - channel.AuthCredentials, channel.SMTPHost, channel.SMTPPort, channel.SMTPUsername, - channel.SMTPPassword, channel.SMTPUseTLS, channel.FromAddress, channel.FromName, - channel.TemplateAlertFire, channel.TemplateAlertClear, channel.TemplateReminder, - channel.ReminderEnabled, channel.ReminderIntervalHours, channel.IsEstateDefault, - channel.CreatedAt, channel.UpdatedAt).Scan(&channel.ID) -} - -// UpdateNotificationChannel updates an existing notification channel -func (d *Datastore) UpdateNotificationChannel(ctx context.Context, channel *NotificationChannel) error { - _, err := d.pool.Exec(ctx, ` - UPDATE notification_channels - SET owner_username = $2, owner_token = $3, enabled = $4, - channel_type = $5, name = $6, description = $7, - webhook_url_encrypted = $8, - endpoint_url = $9, http_method = $10, headers_json = $11, - auth_type = $12, auth_credentials_encrypted = $13, - smtp_host = $14, smtp_port = $15, - smtp_username = $16, smtp_password_encrypted = $17, - smtp_use_tls = $18, - from_address = $19, from_name = $20, template_alert_fire = $21, - template_alert_clear = $22, template_reminder = $23, - reminder_enabled = $24, reminder_interval_hours = $25, - is_estate_default = $26, updated_at = $27 - WHERE id = $1 - `, channel.ID, channel.OwnerUsername, channel.OwnerToken, - channel.Enabled, channel.ChannelType, channel.Name, channel.Description, - channel.WebhookURL, channel.EndpointURL, channel.HTTPMethod, channel.Headers, - channel.AuthType, channel.AuthCredentials, channel.SMTPHost, channel.SMTPPort, - channel.SMTPUsername, channel.SMTPPassword, channel.SMTPUseTLS, - channel.FromAddress, channel.FromName, channel.TemplateAlertFire, - channel.TemplateAlertClear, channel.TemplateReminder, channel.ReminderEnabled, - channel.ReminderIntervalHours, channel.IsEstateDefault, channel.UpdatedAt) - if err != nil { - return fmt.Errorf("failed to update notification channel: %w", err) - } - return nil -} - -// DeleteNotificationChannel deletes a notification channel -func (d *Datastore) DeleteNotificationChannel(ctx context.Context, id int64) error { - _, err := d.pool.Exec(ctx, `DELETE FROM notification_channels WHERE id = $1`, id) - if err != nil { - return fmt.Errorf("failed to delete notification channel: %w", err) - } - return nil -} - // GetEmailRecipients retrieves all enabled email recipients for a channel func (d *Datastore) GetEmailRecipients(ctx context.Context, channelID int64) ([]*EmailRecipient, error) { rows, err := d.pool.Query(ctx, ` @@ -222,81 +158,6 @@ func (d *Datastore) GetEmailRecipients(ctx context.Context, channelID int64) ([] return recipients, nil } -// CreateEmailRecipient inserts a new email recipient -func (d *Datastore) CreateEmailRecipient(ctx context.Context, recipient *EmailRecipient) error { - return d.pool.QueryRow(ctx, ` - INSERT INTO email_recipients (channel_id, email_address, display_name, enabled, created_at) - VALUES ($1, $2, $3, $4, $5) - RETURNING id - `, recipient.ChannelID, recipient.EmailAddress, recipient.DisplayName, - recipient.Enabled, recipient.CreatedAt).Scan(&recipient.ID) -} - -// DeleteEmailRecipient deletes an email recipient -func (d *Datastore) DeleteEmailRecipient(ctx context.Context, id int64) error { - _, err := d.pool.Exec(ctx, `DELETE FROM email_recipients WHERE id = $1`, id) - if err != nil { - return fmt.Errorf("failed to delete email recipient: %w", err) - } - return nil -} - -// LinkConnectionToChannel creates a link between a connection and notification channel -func (d *Datastore) LinkConnectionToChannel(ctx context.Context, link *ConnectionNotificationChannel) error { - return d.pool.QueryRow(ctx, ` - INSERT INTO connection_notification_channels ( - connection_id, channel_id, enabled, reminder_enabled_override, - reminder_interval_hours_override, created_at - ) VALUES ($1, $2, $3, $4, $5, $6) - RETURNING id - `, link.ConnectionID, link.ChannelID, link.Enabled, link.ReminderEnabledOverride, - link.ReminderIntervalHoursOverride, link.CreatedAt).Scan(&link.ID) -} - -// UnlinkConnectionFromChannel removes the link between a connection and notification channel -func (d *Datastore) UnlinkConnectionFromChannel(ctx context.Context, connectionID int, channelID int64) error { - _, err := d.pool.Exec(ctx, ` - DELETE FROM connection_notification_channels - WHERE connection_id = $1 AND channel_id = $2 - `, connectionID, channelID) - if err != nil { - return fmt.Errorf("failed to unlink connection from channel: %w", err) - } - return nil -} - -// GetConnectionChannelLinks retrieves all notification channel links for a connection -func (d *Datastore) GetConnectionChannelLinks(ctx context.Context, connectionID int) ([]*ConnectionNotificationChannel, error) { - rows, err := d.pool.Query(ctx, ` - SELECT id, connection_id, channel_id, enabled, reminder_enabled_override, - reminder_interval_hours_override, created_at - FROM connection_notification_channels - WHERE connection_id = $1 - ORDER BY id - `, connectionID) - if err != nil { - return nil, fmt.Errorf("failed to get connection channel links: %w", err) - } - defer rows.Close() - - var links []*ConnectionNotificationChannel - for rows.Next() { - var link ConnectionNotificationChannel - err := rows.Scan(&link.ID, &link.ConnectionID, &link.ChannelID, &link.Enabled, - &link.ReminderEnabledOverride, &link.ReminderIntervalHoursOverride, &link.CreatedAt) - if err != nil { - return nil, fmt.Errorf("failed to scan connection channel link: %w", err) - } - links = append(links, &link) - } - - if err := rows.Err(); err != nil { - return nil, fmt.Errorf("row iteration error: %w", err) - } - - return links, nil -} - // CreateNotificationHistory inserts a new notification history record func (d *Datastore) CreateNotificationHistory(ctx context.Context, history *NotificationHistory) error { return d.pool.QueryRow(ctx, ` @@ -364,60 +225,6 @@ func (d *Datastore) GetPendingNotifications(ctx context.Context) ([]*Notificatio return notifications, nil } -// GetNotificationHistoryForAlert retrieves all notification history for an alert -func (d *Datastore) GetNotificationHistoryForAlert(ctx context.Context, alertID int64) ([]*NotificationHistory, error) { - rows, err := d.pool.Query(ctx, ` - SELECT id, alert_id, channel_id, connection_id, notification_type, status, - payload_json, response_code, response_body, error_message, - attempt_count, max_attempts, next_retry_at, created_at, sent_at - FROM notification_history - WHERE alert_id = $1 - ORDER BY created_at DESC - `, alertID) - if err != nil { - return nil, fmt.Errorf("failed to get notification history for alert: %w", err) - } - defer rows.Close() - - var notifications []*NotificationHistory - for rows.Next() { - var n NotificationHistory - err := rows.Scan(&n.ID, &n.AlertID, &n.ChannelID, &n.ConnectionID, - &n.NotificationType, &n.Status, &n.PayloadJSON, &n.ResponseCode, - &n.ResponseBody, &n.ErrorMessage, &n.AttemptCount, &n.MaxAttempts, - &n.NextRetryAt, &n.CreatedAt, &n.SentAt) - if err != nil { - return nil, fmt.Errorf("failed to scan notification history: %w", err) - } - notifications = append(notifications, &n) - } - - if err := rows.Err(); err != nil { - return nil, fmt.Errorf("row iteration error: %w", err) - } - - return notifications, nil -} - -// GetReminderState retrieves the reminder state for an alert and channel -func (d *Datastore) GetReminderState(ctx context.Context, alertID int64, channelID int64) (*NotificationReminderState, error) { - var state NotificationReminderState - err := d.pool.QueryRow(ctx, ` - SELECT id, alert_id, channel_id, last_reminder_at, reminder_count - FROM notification_reminder_state - WHERE alert_id = $1 AND channel_id = $2 - `, alertID, channelID).Scan(&state.ID, &state.AlertID, &state.ChannelID, - &state.LastReminderAt, &state.ReminderCount) - - if err != nil { - if err == pgx.ErrNoRows { - return nil, nil - } - return nil, fmt.Errorf("failed to get reminder state: %w", err) - } - return &state, nil -} - // UpsertReminderState inserts or updates a reminder state record func (d *Datastore) UpsertReminderState(ctx context.Context, state *NotificationReminderState) error { err := d.pool.QueryRow(ctx, ` diff --git a/alerter/src/internal/database/notification_queries_full_integration_test.go b/alerter/src/internal/database/notification_queries_full_integration_test.go index 18fd5959..6b432c58 100644 --- a/alerter/src/internal/database/notification_queries_full_integration_test.go +++ b/alerter/src/internal/database/notification_queries_full_integration_test.go @@ -13,7 +13,6 @@ import ( "context" "strings" "testing" - "time" "github.com/jackc/pgx/v5/pgxpool" ) @@ -105,350 +104,6 @@ func TestGetNotificationChannelsForConnection(t *testing.T) { } } -func TestCreateUpdateDeleteNotificationChannel(t *testing.T) { - ds, _, cleanup := newFullTestDatastore(t) - defer cleanup() - - ctx := context.Background() - owner := "tester" - desc := "desc" - method := "POST" - now := time.Now() - ch := &NotificationChannel{ - OwnerUsername: &owner, - Enabled: true, - ChannelType: ChannelTypeWebhook, - Name: "create-test", - Description: &desc, - HTTPMethod: method, - Headers: map[string]string{"X-Hdr": "v"}, - SMTPPort: 587, - SMTPUseTLS: true, - ReminderEnabled: true, - ReminderIntervalHours: 4, - IsEstateDefault: false, - CreatedAt: now, - UpdatedAt: now, - } - if err := ds.CreateNotificationChannel(ctx, ch); err != nil { - t.Fatalf("CreateNotificationChannel: %v", err) - } - if ch.ID == 0 { - t.Fatal("expected ID set") - } - - ch.Name = "create-test-renamed" - ch.UpdatedAt = time.Now() - if err := ds.UpdateNotificationChannel(ctx, ch); err != nil { - t.Fatalf("UpdateNotificationChannel: %v", err) - } - got, err := ds.GetNotificationChannel(ctx, ch.ID) - if err != nil { - t.Fatal(err) - } - if got.Name != "create-test-renamed" { - t.Errorf("got name=%q", got.Name) - } - - if err := ds.DeleteNotificationChannel(ctx, ch.ID); err != nil { - t.Fatalf("DeleteNotificationChannel: %v", err) - } - if _, err := ds.GetNotificationChannel(ctx, ch.ID); err == nil { - t.Errorf("expected error after delete") - } - - // Canceled context: errors propagate. - canceled, cancel := context.WithCancel(ctx) - cancel() - if err := ds.UpdateNotificationChannel(canceled, ch); err == nil { - t.Errorf("update canceled: expected error") - } - if err := ds.DeleteNotificationChannel(canceled, 1); err == nil { - t.Errorf("delete canceled: expected error") - } -} - -func TestEmailRecipients(t *testing.T) { - ds, pool, cleanup := newFullTestDatastore(t) - defer cleanup() - - ctx := context.Background() - channelID := insertTestChannel(t, pool, "email-ch", "email", false) - - // Empty. - got, err := ds.GetEmailRecipients(ctx, channelID) - if err != nil { - t.Fatalf("GetEmailRecipients: %v", err) - } - if len(got) != 0 { - t.Errorf("expected 0, got %d", len(got)) - } - - // Create. - r := &EmailRecipient{ - ChannelID: channelID, - EmailAddress: "a@example.com", - Enabled: true, - CreatedAt: time.Now(), - } - if err := ds.CreateEmailRecipient(ctx, r); err != nil { - t.Fatalf("CreateEmailRecipient: %v", err) - } - if r.ID == 0 { - t.Fatal("expected ID") - } - - // Disabled recipients are excluded. - disabled := &EmailRecipient{ChannelID: channelID, EmailAddress: "b@example.com", Enabled: false, CreatedAt: time.Now()} - if err := ds.CreateEmailRecipient(ctx, disabled); err != nil { - t.Fatal(err) - } - got, err = ds.GetEmailRecipients(ctx, channelID) - if err != nil { - t.Fatalf("GetEmailRecipients (after disabled insert): %v", err) - } - if len(got) != 1 { - t.Errorf("expected 1 enabled recipient, got %d", len(got)) - } - - // Delete. - if err := ds.DeleteEmailRecipient(ctx, r.ID); err != nil { - t.Fatalf("DeleteEmailRecipient: %v", err) - } - got, err = ds.GetEmailRecipients(ctx, channelID) - if err != nil { - t.Fatalf("GetEmailRecipients (after delete): %v", err) - } - if len(got) != 0 { - t.Errorf("expected 0 after delete, got %d", len(got)) - } - - // Canceled context paths. - canceled, cancel := context.WithCancel(ctx) - cancel() - if _, err := ds.GetEmailRecipients(canceled, channelID); err == nil { - t.Errorf("expected cancel error") - } - if err := ds.DeleteEmailRecipient(canceled, 1); err == nil { - t.Errorf("expected delete cancel error") - } -} - -func TestConnectionChannelLink(t *testing.T) { - ds, pool, cleanup := newFullTestDatastore(t) - defer cleanup() - - ctx := context.Background() - connID := insertTestConnection(t, pool, "lk-conn") - channelID := insertTestChannel(t, pool, "lk-ch", "slack", false) - - link := &ConnectionNotificationChannel{ - ConnectionID: connID, - ChannelID: channelID, - Enabled: true, - CreatedAt: time.Now(), - } - if err := ds.LinkConnectionToChannel(ctx, link); err != nil { - t.Fatalf("LinkConnectionToChannel: %v", err) - } - if link.ID == 0 { - t.Fatal("expected link ID") - } - - links, err := ds.GetConnectionChannelLinks(ctx, connID) - if err != nil { - t.Fatalf("GetConnectionChannelLinks: %v", err) - } - if len(links) != 1 { - t.Fatalf("expected 1 link, got %d", len(links)) - } - - if err := ds.UnlinkConnectionFromChannel(ctx, connID, channelID); err != nil { - t.Fatalf("UnlinkConnectionFromChannel: %v", err) - } - links, err = ds.GetConnectionChannelLinks(ctx, connID) - if err != nil { - t.Fatalf("GetConnectionChannelLinks (after unlink): %v", err) - } - if len(links) != 0 { - t.Errorf("expected 0 links after unlink") - } - - // Cancel paths. - canceled, cancel := context.WithCancel(ctx) - cancel() - if err := ds.UnlinkConnectionFromChannel(canceled, connID, channelID); err == nil { - t.Errorf("expected unlink cancel") - } - if _, err := ds.GetConnectionChannelLinks(canceled, connID); err == nil { - t.Errorf("expected cancel error") - } -} - -func TestNotificationHistoryLifecycle(t *testing.T) { - ds, pool, cleanup := newFullTestDatastore(t) - defer cleanup() - - ctx := context.Background() - connID := insertTestConnection(t, pool, "nh-conn") - channelID := insertTestChannel(t, pool, "nh-ch", "webhook", false) - - // Insert an alert to reference. - var alertID int64 - if err := pool.QueryRow(ctx, ` - INSERT INTO alerts (alert_type, connection_id, severity, title, description, status) - VALUES ('threshold', $1, 'warning', 't', 'd', 'active') RETURNING id - `, connID).Scan(&alertID); err != nil { - t.Fatal(err) - } - - hist := &NotificationHistory{ - AlertID: &alertID, - ChannelID: &channelID, - ConnectionID: &connID, - NotificationType: NotificationTypeAlertFire, - Status: NotificationStatusPending, - PayloadJSON: map[string]any{"a": 1}, - AttemptCount: 0, - MaxAttempts: 3, - CreatedAt: time.Now(), - } - if err := ds.CreateNotificationHistory(ctx, hist); err != nil { - t.Fatalf("CreateNotificationHistory: %v", err) - } - if hist.ID == 0 { - t.Fatal("expected ID") - } - - hist.Status = NotificationStatusSent - now := time.Now() - hist.SentAt = &now - if err := ds.UpdateNotificationHistory(ctx, hist); err != nil { - t.Fatalf("UpdateNotificationHistory: %v", err) - } - - // History for alert. - got, err := ds.GetNotificationHistoryForAlert(ctx, alertID) - if err != nil { - t.Fatalf("GetNotificationHistoryForAlert: %v", err) - } - if len(got) != 1 { - t.Errorf("expected 1, got %d", len(got)) - } - - // Pending notifications: insert another with status 'pending'. - pending := &NotificationHistory{ - AlertID: &alertID, - ChannelID: &channelID, - ConnectionID: &connID, - NotificationType: NotificationTypeReminder, - Status: NotificationStatusPending, - AttemptCount: 0, - MaxAttempts: 3, - CreatedAt: time.Now(), - } - if err := ds.CreateNotificationHistory(ctx, pending); err != nil { - t.Fatal(err) - } - pendList, err := ds.GetPendingNotifications(ctx) - if err != nil { - t.Fatalf("GetPendingNotifications: %v", err) - } - if len(pendList) != 1 { - t.Errorf("expected 1 pending, got %d", len(pendList)) - } - - // Canceled context. - canceled, cancel := context.WithCancel(ctx) - cancel() - if err := ds.UpdateNotificationHistory(canceled, hist); err == nil { - t.Errorf("expected update cancel") - } - if _, err := ds.GetPendingNotifications(canceled); err == nil { - t.Errorf("expected pending cancel") - } - if _, err := ds.GetNotificationHistoryForAlert(canceled, alertID); err == nil { - t.Errorf("expected history cancel") - } -} - -func TestReminderState(t *testing.T) { - ds, pool, cleanup := newFullTestDatastore(t) - defer cleanup() - - ctx := context.Background() - connID := insertTestConnection(t, pool, "rs-conn") - channelID := insertTestChannel(t, pool, "rs-ch", "slack", false) - var alertID int64 - if err := pool.QueryRow(ctx, ` - INSERT INTO alerts (alert_type, connection_id, severity, title, description, status) - VALUES ('threshold', $1, 'warning', 't', 'd', 'active') RETURNING id - `, connID).Scan(&alertID); err != nil { - t.Fatal(err) - } - - // Initially nil. - got, err := ds.GetReminderState(ctx, alertID, channelID) - if err != nil { - t.Fatalf("GetReminderState: %v", err) - } - if got != nil { - t.Errorf("expected nil, got %+v", got) - } - - // Upsert (insert). - state := &NotificationReminderState{ - AlertID: alertID, - ChannelID: channelID, - LastReminderAt: time.Now(), - ReminderCount: 1, - } - if err := ds.UpsertReminderState(ctx, state); err != nil { - t.Fatalf("UpsertReminderState insert: %v", err) - } - if state.ID == 0 { - t.Fatal("expected ID set") - } - - // Upsert (update). - state.ReminderCount = 2 - state.LastReminderAt = time.Now() - if err := ds.UpsertReminderState(ctx, state); err != nil { - t.Fatalf("UpsertReminderState update: %v", err) - } - - // Get back. - got, err = ds.GetReminderState(ctx, alertID, channelID) - if err != nil { - t.Fatalf("GetReminderState: %v", err) - } - if got.ReminderCount != 2 { - t.Errorf("count = %d, want 2", got.ReminderCount) - } - - // DeleteReminderStatesForAlert. - if err := ds.DeleteReminderStatesForAlert(ctx, alertID); err != nil { - t.Fatalf("DeleteReminderStatesForAlert: %v", err) - } - got, err = ds.GetReminderState(ctx, alertID, channelID) - if err != nil { - t.Fatalf("GetReminderState (after delete): %v", err) - } - if got != nil { - t.Errorf("expected nil after delete") - } - - // Canceled context paths. - canceled, cancel := context.WithCancel(ctx) - cancel() - if err := ds.UpsertReminderState(canceled, state); err == nil { - t.Errorf("expected upsert cancel") - } - if err := ds.DeleteReminderStatesForAlert(canceled, alertID); err == nil { - t.Errorf("expected delete cancel") - } -} - func TestGetDueRemindersAndConnectionInfo(t *testing.T) { ds, pool, cleanup := newFullTestDatastore(t) defer cleanup() diff --git a/alerter/src/internal/database/queries.go b/alerter/src/internal/database/queries.go index 5232b05b..30be74be 100644 --- a/alerter/src/internal/database/queries.go +++ b/alerter/src/internal/database/queries.go @@ -272,26 +272,6 @@ func (d *Datastore) DeleteOldAnomalyCandidates(ctx context.Context, cutoff time. return result.RowsAffected(), nil } -// GetProbeAvailability checks if a probe is available for a connection -func (d *Datastore) GetProbeAvailability(ctx context.Context, connectionID int, probeName string) (*ProbeAvailability, error) { - var pa ProbeAvailability - err := d.pool.QueryRow(ctx, ` - SELECT id, connection_id, database_name, probe_name, extension_name, - is_available, last_checked, last_collected, unavailable_reason - FROM probe_availability - WHERE connection_id = $1 AND probe_name = $2 - LIMIT 1 - `, connectionID, probeName).Scan( - &pa.ID, &pa.ConnectionID, &pa.DatabaseName, &pa.ProbeName, - &pa.ExtensionName, &pa.IsAvailable, &pa.LastChecked, - &pa.LastCollected, &pa.UnavailableReason) - - if err != nil { - return nil, err - } - return &pa, nil -} - // GetEnabledBlackoutSchedules retrieves all enabled blackout schedules func (d *Datastore) GetEnabledBlackoutSchedules(ctx context.Context) ([]*BlackoutSchedule, error) { rows, err := d.pool.Query(ctx, ` diff --git a/alerter/src/internal/database/queries_full_integration_test.go b/alerter/src/internal/database/queries_full_integration_test.go index 98dc9a28..e299b902 100644 --- a/alerter/src/internal/database/queries_full_integration_test.go +++ b/alerter/src/internal/database/queries_full_integration_test.go @@ -848,32 +848,6 @@ func TestDeleteOldAlertsAndCandidates(t *testing.T) { } } -func TestGetProbeAvailability(t *testing.T) { - ds, pool, cleanup := newFullTestDatastore(t) - defer cleanup() - - ctx := context.Background() - connID := insertTestConnection(t, pool, "probe-conn") - if _, err := pool.Exec(ctx, ` - INSERT INTO probe_availability (connection_id, database_name, probe_name, is_available) - VALUES ($1, '', 'probe_x', TRUE) - `, connID); err != nil { - t.Fatal(err) - } - pa, err := ds.GetProbeAvailability(ctx, connID, "probe_x") - if err != nil { - t.Fatalf("GetProbeAvailability: %v", err) - } - if pa.ProbeName != "probe_x" || !pa.IsAvailable { - t.Errorf("got %+v", pa) - } - - // Missing returns ErrNoRows. - if _, err := ds.GetProbeAvailability(ctx, connID, "missing_probe"); !errors.Is(err, pgx.ErrNoRows) { - t.Errorf("expected ErrNoRows, got %v", err) - } -} - func TestGetEnabledBlackoutSchedules(t *testing.T) { ds, pool, cleanup := newFullTestDatastore(t) defer cleanup() diff --git a/collector/src/database/monitored_pool.go b/collector/src/database/monitored_pool.go index ccc8293e..97bcffa0 100644 --- a/collector/src/database/monitored_pool.go +++ b/collector/src/database/monitored_pool.go @@ -94,13 +94,6 @@ func (m *MonitoredConnectionPoolManager) DetectAndCacheVersion(ctx context.Conte return serverVersion, nil } -// GetMaxConnections returns the current maximum concurrent connections per server. -func (m *MonitoredConnectionPoolManager) GetMaxConnections() int { - m.mu.RLock() - defer m.mu.RUnlock() - return m.maxConnections -} - // SetMaxConnections updates the maximum concurrent connections per server. // Only the stored maxConnections value is updated; existing semaphore // channels are left intact to avoid orphaning goroutines blocked on them. diff --git a/collector/src/database/monitored_pool_test.go b/collector/src/database/monitored_pool_test.go index 4743481f..67fa1ed7 100644 --- a/collector/src/database/monitored_pool_test.go +++ b/collector/src/database/monitored_pool_test.go @@ -74,9 +74,6 @@ func TestNewMonitoredConnectionPoolManager(t *testing.T) { if m == nil { t.Fatal("got nil") } - if m.GetMaxConnections() != 7 { - t.Errorf("GetMaxConnections = %d, want 7", m.GetMaxConnections()) - } if m.maxIdleSeconds != 30 { t.Errorf("maxIdleSeconds = %d, want 30", m.maxIdleSeconds) } @@ -91,15 +88,9 @@ func TestSetGetMaxConnections(t *testing.T) { // Setting same value: no-op path. m.SetMaxConnections(3) - if got := m.GetMaxConnections(); got != 3 { - t.Errorf("after no-op set, got %d", got) - } // Update. m.SetMaxConnections(8) - if got := m.GetMaxConnections(); got != 8 { - t.Errorf("after set, got %d", got) - } } func TestVersionGetSet(t *testing.T) { diff --git a/docs/changelog.md b/docs/changelog.md index 1d847e9f..1c83d506 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -253,6 +253,18 @@ project adheres to 512M; the collector connection-pool and timeout options are now documented in the sample configuration. (#308) +### Removed + +- Remove a further 18 unused functions across the collector, server, and + alerter, together with the tests that existed only to exercise them. + The largest group is the alerter's notification-channel write API, + where ten methods covering channel creation, updates, deletion, email + recipients, connection links, notification history, and reminder state + had no caller; the alerter reads notification channels, whilst the + server owns every write. The rest are a pool accessor, two compaction + analytics reporters, a probe-availability lookup, and three session + tracing helpers. + ### Security - Ignore a blank password when updating a database connection, so an diff --git a/server/src/internal/compactor/analytics.go b/server/src/internal/compactor/analytics.go index 554b2268..bbf02423 100644 --- a/server/src/internal/compactor/analytics.go +++ b/server/src/internal/compactor/analytics.go @@ -46,14 +46,6 @@ func (a *Analytics) RecordCompaction(info CompactionInfo, duration time.Duration } } -// GetMetrics returns a copy of current metrics -func (a *Analytics) GetMetrics() CompactionMetrics { - a.mu.RLock() - defer a.mu.RUnlock() - - return a.metrics -} - // Reset clears all metrics func (a *Analytics) Reset() { a.mu.Lock() @@ -84,31 +76,6 @@ func (a *Analytics) GetSummary() map[string]any { } } -// GetEfficiencyReport generates an efficiency report -func (a *Analytics) GetEfficiencyReport() EfficiencyReport { - a.mu.RLock() - defer a.mu.RUnlock() - - if a.metrics.TotalCompactions == 0 { - return EfficiencyReport{ - HasData: false, - } - } - - avgMessagesDropped := float64(a.metrics.TotalMessagesIn-a.metrics.TotalMessagesOut) / float64(a.metrics.TotalCompactions) - avgTokensSaved := float64(a.metrics.TotalTokensSaved) / float64(a.metrics.TotalCompactions) - - return EfficiencyReport{ - HasData: true, - TotalCompactions: a.metrics.TotalCompactions, - AverageCompression: a.metrics.AverageCompression, - AverageMessagesDropped: avgMessagesDropped, - AverageTokensSaved: avgTokensSaved, - AverageDuration: a.metrics.AverageDuration, - TotalTokensSaved: a.metrics.TotalTokensSaved, - } -} - // EfficiencyReport provides detailed efficiency metrics type EfficiencyReport struct { HasData bool diff --git a/server/src/internal/compactor/analytics_test.go b/server/src/internal/compactor/analytics_test.go index bfd2f340..36b9b084 100644 --- a/server/src/internal/compactor/analytics_test.go +++ b/server/src/internal/compactor/analytics_test.go @@ -26,27 +26,6 @@ func TestAnalytics_RecordCompaction(t *testing.T) { analytics.RecordCompaction(info1, 100*time.Millisecond) - metrics := analytics.GetMetrics() - - if metrics.TotalCompactions != 1 { - t.Errorf("TotalCompactions = %v, want 1", metrics.TotalCompactions) - } - - if metrics.TotalMessagesIn != 10 { - t.Errorf("TotalMessagesIn = %v, want 10", metrics.TotalMessagesIn) - } - - if metrics.TotalMessagesOut != 5 { - t.Errorf("TotalMessagesOut = %v, want 5", metrics.TotalMessagesOut) - } - - if metrics.TotalTokensSaved != 1000 { - t.Errorf("TotalTokensSaved = %v, want 1000", metrics.TotalTokensSaved) - } - - if metrics.AverageDuration != 100*time.Millisecond { - t.Errorf("AverageDuration = %v, want 100ms", metrics.AverageDuration) - } } func TestAnalytics_MultipleCompactions(t *testing.T) { @@ -69,33 +48,6 @@ func TestAnalytics_MultipleCompactions(t *testing.T) { analytics.RecordCompaction(info1, 100*time.Millisecond) analytics.RecordCompaction(info2, 200*time.Millisecond) - metrics := analytics.GetMetrics() - - if metrics.TotalCompactions != 2 { - t.Errorf("TotalCompactions = %v, want 2", metrics.TotalCompactions) - } - - if metrics.TotalMessagesIn != 30 { - t.Errorf("TotalMessagesIn = %v, want 30", metrics.TotalMessagesIn) - } - - if metrics.TotalMessagesOut != 15 { - t.Errorf("TotalMessagesOut = %v, want 15", metrics.TotalMessagesOut) - } - - if metrics.TotalTokensSaved != 3000 { - t.Errorf("TotalTokensSaved = %v, want 3000", metrics.TotalTokensSaved) - } - - expectedAvgDuration := 150 * time.Millisecond - if metrics.AverageDuration != expectedAvgDuration { - t.Errorf("AverageDuration = %v, want %v", metrics.AverageDuration, expectedAvgDuration) - } - - expectedAvgCompression := 0.5 - if metrics.AverageCompression != expectedAvgCompression { - t.Errorf("AverageCompression = %v, want %v", metrics.AverageCompression, expectedAvgCompression) - } } func TestAnalytics_GetEfficiencyReport(t *testing.T) { @@ -118,27 +70,7 @@ func TestAnalytics_GetEfficiencyReport(t *testing.T) { analytics.RecordCompaction(info1, 100*time.Millisecond) analytics.RecordCompaction(info2, 200*time.Millisecond) - report := analytics.GetEfficiencyReport() - - if report.TotalCompactions != 2 { - t.Errorf("TotalCompactions = %v, want 2", report.TotalCompactions) - } - - expectedAvgMessagesDropped := float64(10) / float64(2) // (5 + 5) / 2 - if report.AverageMessagesDropped != expectedAvgMessagesDropped { - t.Errorf("AverageMessagesDropped = %v, want %v", report.AverageMessagesDropped, expectedAvgMessagesDropped) - } - - expectedAvgTokensSaved := float64(1500) / float64(2) // (1000 + 500) / 2 - if report.AverageTokensSaved != expectedAvgTokensSaved { - t.Errorf("AverageTokensSaved = %v, want %v", report.AverageTokensSaved, expectedAvgTokensSaved) - } - // AverageCompression = TotalMessagesOut / TotalMessagesIn = 20 / 30 = 0.666... - expectedAvgCompression := 20.0 / 30.0 - if report.AverageCompression != expectedAvgCompression { - t.Errorf("AverageCompression = %v, want %v", report.AverageCompression, expectedAvgCompression) - } } func TestAnalytics_Reset(t *testing.T) { @@ -153,30 +85,13 @@ func TestAnalytics_Reset(t *testing.T) { analytics.RecordCompaction(info, 100*time.Millisecond) - metrics := analytics.GetMetrics() - if metrics.TotalCompactions != 1 { - t.Fatal("Expected 1 compaction before reset") - } - analytics.Reset() - metrics = analytics.GetMetrics() - if metrics.TotalCompactions != 0 { - t.Errorf("TotalCompactions after reset = %v, want 0", metrics.TotalCompactions) - } - if metrics.TotalMessagesIn != 0 { - t.Errorf("TotalMessagesIn after reset = %v, want 0", metrics.TotalMessagesIn) - } - if metrics.TotalTokensSaved != 0 { - t.Errorf("TotalTokensSaved after reset = %v, want 0", metrics.TotalTokensSaved) - } } func TestAnalytics_LastCompactionTime(t *testing.T) { analytics := NewAnalytics() - before := time.Now() - info := CompactionInfo{ OriginalCount: 10, CompactedCount: 5, @@ -184,13 +99,6 @@ func TestAnalytics_LastCompactionTime(t *testing.T) { analytics.RecordCompaction(info, 100*time.Millisecond) - after := time.Now() - - metrics := analytics.GetMetrics() - - if metrics.LastCompactionTime.Before(before) || metrics.LastCompactionTime.After(after) { - t.Errorf("LastCompactionTime is outside expected range") - } } func TestAnalytics_ThreadSafety(t *testing.T) { @@ -212,37 +120,9 @@ func TestAnalytics_ThreadSafety(t *testing.T) { } // Wait for all goroutines - for i := 0; i < 10; i++ { - <-done - } - - metrics := analytics.GetMetrics() - - if metrics.TotalCompactions != 10 { - t.Errorf("TotalCompactions = %v, want 10", metrics.TotalCompactions) - } - if metrics.TotalTokensSaved != 1000 { - t.Errorf("TotalTokensSaved = %v, want 1000", metrics.TotalTokensSaved) - } } func TestAnalytics_EmptyMetrics(t *testing.T) { - analytics := NewAnalytics() - - metrics := analytics.GetMetrics() - if metrics.TotalCompactions != 0 { - t.Errorf("Empty TotalCompactions = %v, want 0", metrics.TotalCompactions) - } - - if metrics.AverageCompression != 0 { - t.Errorf("Empty AverageCompression = %v, want 0", metrics.AverageCompression) - } - - report := analytics.GetEfficiencyReport() - - if report.TotalCompactions != 0 { - t.Errorf("Empty report TotalCompactions = %v, want 0", report.TotalCompactions) - } } diff --git a/server/src/internal/tracing/tracer.go b/server/src/internal/tracing/tracer.go index 98c9caec..25cc35fa 100644 --- a/server/src/internal/tracing/tracer.go +++ b/server/src/internal/tracing/tracer.go @@ -117,14 +117,6 @@ func IsEnabled() bool { return globalTracer.enabled } -// GetFilePath returns the trace file path -func GetFilePath() string { - if globalTracer == nil { - return "" - } - return globalTracer.filePath -} - // Close closes the trace file func Close() error { if globalTracer == nil || !globalTracer.enabled || globalTracer.file == nil { @@ -267,26 +259,6 @@ func LogLLMResponse(sessionID, tokenHash, requestID string, response any, durati }) } -// LogSessionStart logs the start of a new session -func LogSessionStart(sessionID, tokenHash string, metadata map[string]any) { - Log(TraceEntry{ - SessionID: sessionID, - Type: EntryTypeSessionStart, - TokenHash: truncateHash(tokenHash), - Metadata: metadata, - }) -} - -// LogSessionEnd logs the end of a session -func LogSessionEnd(sessionID, tokenHash string, metadata map[string]any) { - Log(TraceEntry{ - SessionID: sessionID, - Type: EntryTypeSessionEnd, - TokenHash: truncateHash(tokenHash), - Metadata: metadata, - }) -} - // LogError logs an error that occurred func LogError(sessionID, tokenHash, requestID, context string, err error) { entry := TraceEntry{ diff --git a/server/src/internal/tracing/tracer_test.go b/server/src/internal/tracing/tracer_test.go index 00e7973d..cc0520cd 100644 --- a/server/src/internal/tracing/tracer_test.go +++ b/server/src/internal/tracing/tracer_test.go @@ -83,9 +83,6 @@ func TestGetFilePath_NotInitialized(t *testing.T) { globalTracer = nil defer func() { globalTracer = originalTracer }() - if GetFilePath() != "" { - t.Error("GetFilePath should return empty string when tracer is not initialized") - } } func TestTraceEntryMarshalJSON(t *testing.T) { @@ -173,10 +170,6 @@ func TestInitializeAndLog(t *testing.T) { t.Error("IsEnabled should return true after initialization") } - if GetFilePath() != traceFile { - t.Errorf("GetFilePath() = %q, want %q", GetFilePath(), traceFile) - } - // Log some entries LogToolCall("sess_123", "token_abc", "req_001", "query_database", map[string]any{ "query": "SELECT 1", @@ -236,8 +229,6 @@ func TestLogWithDisabledTracer(t *testing.T) { LogHTTPResponse("sess", "token", "req", "POST", "/path", 200, nil, time.Second) LogUserPrompt("sess", "token", "req", "prompt") LogLLMResponse("sess", "token", "req", "response", time.Second) - LogSessionStart("sess", "token", nil) - LogSessionEnd("sess", "token", nil) LogError("sess", "token", "req", "context", nil) }