basic alerting, for internal use primarily #778
Conversation
heavycrystal
requested review from
serprex,
iskakaushik,
iamKunalGupta and
Amogh-Bharadwaj
flow/connectors/postgres/postgres.go
Outdated
| @@ -79,6 +81,11 @@ func NewPostgresConnector(ctx context.Context, pgConfig *protos.PostgresConfig) | |||
| metadataSchema = *pgConfig.MetadataSchema | |||
| } | |||
|
|
|||
| vigil, err := evervigil.NewVigil() | |||
There was a problem hiding this comment.
should NewVigil take ctx given it's calling Context.Background()?
There was a problem hiding this comment.
I thought having it be context.Background() would allow alerts to be sent even in cases where context cancellation happens, not sure if it'll work out in practice.
There was a problem hiding this comment.
No, if you want that level of isolation you'll want the monitor to be a separate container
heavycrystal
force-pushed
the
alerting-v1-evervigil
branch
from
79da726
to
8b2c0f0
Compare
|
@heavycrystal could you limit this PR to just implement the alerting service for slot growth. Lets split out the temporal activity failure alerts. |
heavycrystal
force-pushed
the
alerting-v1-evervigil
branch
from
90a9d43
to
0ad9858
Compare
|
|
||
| if int64(slotInfo[0].LagInMb) >= slotLagInMBThreshold { | ||
| a.Vigil.AlertIf(fmt.Sprintf("%s-slot-size-exceeded", peerName), | ||
| fmt.Sprintf("Slot %s on peer %s has exceeded threshold size of %dMB, currently at %fMB!", |
There was a problem hiding this comment.
| fmt.Sprintf("Slot %s on peer %s has exceeded threshold size of %dMB, currently at %fMB!", | |
| fmt.Sprintf("Slot %s on peer %s has exceeded threshold size of %dMB, currently at %.2fMB!", |
Connection pools are meant to be shared, so remove pool from monitoring Also go one step further: cache connection pool from env.go into global This requires never closing pool returned by GetCatalogConnectionPoolFromEnv Run migrations for CI because `monitoring` logic is now non optional Opens up reusing pool for #778
heavycrystal
force-pushed
the
alerting-v1-evervigil
branch
from
523c80b
to
10dd323
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
heavycrystal
force-pushed
the
alerting-v1-evervigil
branch
from
10dd323
to
95bb209
Compare
| if ev.catalogPool != nil { | ||
| ev.catalogPool.Close() | ||
| } |
There was a problem hiding this comment.
Catalog pool should never be closed
| @@ -43,9 +43,6 @@ type CDCPullConnector interface { | |||
| // PullFlowCleanup drops both the Postgres publication and replication slot, as a part of DROP MIRROR | |||
| PullFlowCleanup(jobName string) error | |||
|
|
|||
| // SendWALHeartbeat allows for activity to progress restart_lsn on postgres. | |||
There was a problem hiding this comment.
Is this being removed because it's unused? Should be split out to another PR
|
please rename EverVigil to something like |
|
reworked in #866 |
For now, an alerting function logs an entry in an alerts table in catalog, and also dispatches notifications to one or more slack channels. Events are staggered to be 15 minutes apart at least, as long as the same "alert key" is used.
Only logs alerts for slot lag increasing a threshold, configured via the environment variable
PEERDB_SLOT_LAG_MB_ALERT_THRESHOLD