riverqueue / river

Fast and reliable background jobs in Go
https://riverqueue.com
Mozilla Public License 2.0
3.59k stars 94 forks source link

Make jobCancelError and jobSnoozeError public #625

Closed jace-ys closed 3 weeks ago

jace-ys commented 2 months ago

Hey 👋🏻 I'm not entirely sure why jobCancelError and jobSnoozeError are not exported as part of the public API, especially because jobCancelError also implements errors.Is and errors.Unwrap.

What I'm trying to achieve is to wrap my actual Work functions with an instrumented Work helper that logs/traces job errors. Right now the only way to check if the actual Work returned river.JobCancel or river.JobSnooze is to do a string prefix match, which isn't ideal.

For example:

    err = w.worker.Work(ctx, job)
    if err != nil {
        var errReason string
        switch {
        case strings.HasPrefix(err.Error(), "jobCancelError:"):
            errReason = "job cancelled"
            span.SetAttributes(attribute.Bool("job.cancelled", true))

        case strings.HasPrefix(err.Error(), "jobSnoozeError:"):
            slog.Info(ctx, "job snoozed")
            span.SetAttributes(attribute.Bool("job.snoozed", true))
            return err

        case job.Attempt == job.MaxAttempts:
            errReason = "job failed, discarded due to max attempts exceeded"
            span.SetAttributes(attribute.Bool("job.discarded", true))

        default:
            errReason = "job failed"
            span.SetAttributes(attribute.Bool("job.failed", true))
        }

        slog.Error(ctx, errReason, err)
        span.SetStatus(codes.Error, errReason)
        span.SetAttributes(attribute.String("error", err.Error()))

        return err
    }

If the error types were made public, I will be able to simplify this using errors.Is(err, &river.JobCancelError{}).

Pascal-Delange commented 1 month ago

Hi, I was going to make the same ticket, independently. This seems like a low-pain change that makes implementing a logging middleware easier.

bgentry commented 3 weeks ago

Good call. There was no specific reason for these to not be exported other than our general hesitation to export anything without a good reason to do so. The need to use these in testing is a pretty good justification IMO!

Fixed in #665 and going out in the next release.

Pascal-Delange commented 3 weeks ago

thanks!