Skip to content

Conversation

@brandur
Copy link
Contributor

@brandur brandur commented Nov 23, 2025

Here, add a "safe" variant of riverlog.Logger: rigerlog.LoggerSafely.

While it'd be rare to not know whether you had a logger or not from
inside a Work function, Work functions might call into other Go
modules in your project, those modules might be reused in non-Work
contexts, and it might not always be clear whether a logger should be
available or not.

This function lets users do a conditional check before logging:

if logger, ok := riverlog.LoggerSafely(ctx); ok {
    logger.InfoContext(ctx, "Hello from logger")
}

I went with the "safely" naming according to our conventions elsewhere,
but one thing I'm not sure about is that ClientFromContextSafely which
is a similar function, returns an error instead of ok boolean. Here we
break with that convention somewhat, but I think the boolean version is
more ergonomic. Another option might be to give this a slightly
alternative naming convention like LoggerOK or LoggerMaybe.

Fixes #1092.

@brandur
Copy link
Contributor Author

brandur commented Nov 23, 2025

The other option is that we could just get rid of the panic and have Logger return a simple nil in case it wasn't available in context. Not sure which version is better, although that one would be a little more risky in causing nil pointer exceptions.

Here, add a "safe" variant of `riverlog.Logger`: `rigerlog.LoggerSafely`.

While it'd be rare to not know whether you had a logger or not from
inside a `Work` function, `Work` functions might call into other Go
modules in your project, those modules might be reused in non-`Work`
contexts, and it might not always be clear whether a logger should be
available or not.

This function lets users do a conditional check before logging:

    if logger, ok := riverlog.LoggerSafely(ctx); ok {
        logger.InfoContext(ctx, "Hello from logger")
    }

I went with the "safely" naming according to our conventions elsewhere,
but one thing I'm not sure about is that `ClientFromContextSafely` which
is a similar function, returns an error instead of `ok` boolean. Here we
break with that convention somewhat, but I think the boolean version is
more ergonomic. Another option might be to give this a slightly
alternative naming convention like `LoggerOK` or `LoggerMaybe`.

Fixes #1092.
@brandur brandur force-pushed the brandur-logger-safely branch from 5c44ad6 to bc9872f Compare November 23, 2025 11:13
@brandur brandur mentioned this pull request Nov 23, 2025
@brandur brandur requested a review from bgentry November 23, 2025 11:14
Copy link
Contributor

@bgentry bgentry left a comment

Choose a reason for hiding this comment

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

This seems like a good solution 👍 Thanks for getting up a fix quickly 🙏

@brandur
Copy link
Contributor Author

brandur commented Nov 24, 2025

Woot. Thx!

@brandur brandur merged commit 317cf8f into master Nov 24, 2025
14 checks passed
@brandur brandur deleted the brandur-logger-safely branch November 24, 2025 05:37
brandur added a commit that referenced this pull request Nov 24, 2025
This one just includes #1093. It may seem a bit early for a new release,
but as discussed, we may want to put #1084 and #1085 into an RC, so I'm
just thinking it might a good idea to capture all other changes in a
release before we do that.
@brandur brandur mentioned this pull request Nov 24, 2025
brandur added a commit that referenced this pull request Nov 24, 2025
This one just includes #1093. It may seem a bit early for a new release,
but as discussed, we may want to put #1084 and #1085 into an RC, so I'm
just thinking it might a good idea to capture all other changes in a
release before we do that.

[skip ci]
brandur added a commit that referenced this pull request Nov 24, 2025
This one just includes #1093. It may seem a bit early for a new release,
but as discussed, we may want to put #1084 and #1085 into an RC, so I'm
just thinking it might a good idea to capture all other changes in a
release before we do that.

[skip ci]
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.

Safe logger

3 participants