-
Notifications
You must be signed in to change notification settings - Fork 887
feat: add logging to pgPubsub #11953
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
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @spikecurtis and the rest of your teammates on |
f214239
to
65cc427
Compare
case pq.ListenerEventConnected: | ||
logger.Info(ctx, "pubsub connected to postgres") | ||
case pq.ListenerEventDisconnected: | ||
logger.Error(ctx, "pubsub disconnected from postgres", slog.Error(err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say this is more of an info or warning than an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not a "normal" disconnection when we asked it to disconnect. It means the connection to postgres dropped due to postgres going down or a network error, and we're going to have to try to reconnect. In the meantime, we could lose some messages sent during the time we are disconnected. It's bad news and should be an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, that makes sense. I didn't think about us potentially losing messages. The disconnect/reconnect I see as expected in any scenario where we're dealing with networking, data/state loss not so much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This got me thinking, how do we propagate this "potential loss of state"? If someone is viewing the dashboard for instance, and the disconnect happens to coincide with a workspace state update, we need a way to propagate that once connection is re-established. A "simple" re-fetch of the relevant information and send it to the user seems sensible, a diff/change detection if we want to be extreme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the Pubsub interface to include a SubscribeWithErr
function that informs the subscriber about potential data loss, so that it can resync or punt the error up to a higher layer to retry. Not everywhere uses it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that it is mainly for debugging purposes.
bc6fe9e
to
c97b89d
Compare
c97b89d
to
1657e5a
Compare
Merge activity
|
Should be helpful for #11950
Adds a logger to pgPubsub and logs various events, most especially connection and disconnection from postgres.