Skip to content

replace log with slog#140

Merged
brandur merged 4 commits intoriverqueue:masterfrom
pmenglund:pme/slog
Jan 6, 2024
Merged

replace log with slog#140
brandur merged 4 commits intoriverqueue:masterfrom
pmenglund:pme/slog

Conversation

@pmenglund
Copy link
Contributor

found a few more places where log is used instead of slog

return
}
log.Printf("error establishing connection from pool: %v", err)
n.logger.Error("error establishing connection from pool", "err", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay this one's a bit tricky in that it's unfortunately failing example tests when changed to slog because especially in CI tests can finish before the notifier's able to connect, and by the time it's done trying to, it may produce a connection error (rather than a context cancelled error which is handled above), which shows up in test output and fails example tests.

Change this line to this:

		// Log at a lower verbosity level in case an error is received when the
		// context is already done (probably because the client is stopping).
		// Example tests can finish before the notifier connects and starts
		// listening, and on client stop may produce a connection error that
		// would otherwise pollute output and fail the test.
		select {
		case <-ctx.Done():
			n.logger.Info("error establishing connection from pool", "err", err)
		default:
			n.logger.Error("error establishing connection from pool", "err", err)
		}

It's a little hacky, but it seems to get the tests passing again.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we have a better way to "fix" this, although it's really only an issue for example tests so it may not warrant much effort 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, there probably is, but I couldn't think of a particularly easy alternative and was thinking that it might be better just to patch it for now, and try to think of something else.

LMK, if you have any ideas. FWIW, here's what a failing build looks like with a more detailed error:

https://github.com/riverqueue/river/actions/runs/7380506104/job/20077953005

error establishing connection from pool: failed to connect to host=127.0.0.1 user=postgres database=river_testdb_example: failed SASL auth (write failed: write tcp 127.0.0.1:42510->127.0.0.1:5432: i/o timeout)

Unfortunately, it's a fairly broad error and it wasn't super obvious to me how to handle it while being sure it's a stop-related error as opposed to something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can make use of context.WithCancelCause to cancel the context with a specific error type during shutdown? Then we could check if the context was cancelled and the cause was a shutdown before logging the error here.

I did something similar in #141.

Copy link
Contributor

Choose a reason for hiding this comment

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

K good idea on this one. I think this rabbit hole may be a little deep to ask for in what should've been a fairly simple third party contribution like this one, but I put it on my list of things to investigate further.

@brandur brandur merged commit ae7cd43 into riverqueue:master Jan 6, 2024
brandur added a commit that referenced this pull request Jan 6, 2024
A few short changelog entries for the next release to include:

* #137
* #138
* #140
brandur added a commit that referenced this pull request Jan 6, 2024
A few short changelog entries for the next release to include:

* #137
* #138
* #140
brandur added a commit that referenced this pull request Jan 6, 2024
A few short changelog entries for the next release to include:

* #137
* #138
* #140
brandur added a commit that referenced this pull request Jan 27, 2024
…or logging

Follow up to a discussion [1] in which an additional error log was
causing example tests to fail after uses of `log` were replaced with
`slog` and no longer suppressed.

Here, use `WithCancelCause` to send a specific cancellation error on
shutdown, which can be handled specially for instances where we'd only
want to log an error under unusual circumstances.

[1] #140 (comment)
brandur added a commit that referenced this pull request Jan 27, 2024
…or logging

Follow up to a discussion [1] in which an additional error log was
causing example tests to fail after uses of `log` were replaced with
`slog` and no longer suppressed.

Here, use `WithCancelCause` to send a specific cancellation error on
shutdown, which can be handled specially for instances where we'd only
want to log an error under unusual circumstances.

[1] #140 (comment)
brandur added a commit that referenced this pull request Jan 27, 2024
…or logging

Follow up to a discussion [1] in which an additional error log was
causing example tests to fail after uses of `log` were replaced with
`slog` and no longer suppressed.

Here, use `WithCancelCause` to send a specific cancellation error on
shutdown, which can be handled specially for instances where we'd only
want to log an error under unusual circumstances.

[1] #140 (comment)
brandur added a commit that referenced this pull request Jan 27, 2024
…or logging

Follow up to a discussion [1] in which an additional error log was
causing example tests to fail after uses of `log` were replaced with
`slog` and no longer suppressed.

Here, use `WithCancelCause` to send a specific cancellation error on
shutdown, which can be handled specially for instances where we'd only
want to log an error under unusual circumstances.

[1] #140 (comment)
brandur added a commit that referenced this pull request Jan 28, 2024
…or logging (#179)

Follow up to a discussion [1] in which an additional error log was
causing example tests to fail after uses of `log` were replaced with
`slog` and no longer suppressed.

Here, use `WithCancelCause` to send a specific cancellation error on
shutdown, which can be handled specially for instances where we'd only
want to log an error under unusual circumstances.

[1] #140 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants