Skip to content
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

Make jobCancelError and jobSnoozeError public #625

Closed
jace-ys opened this issue Sep 27, 2024 · 3 comments · Fixed by #665
Closed

Make jobCancelError and jobSnoozeError public #625

jace-ys opened this issue Sep 27, 2024 · 3 comments · Fixed by #665

Comments

@jace-ys
Copy link

jace-ys commented Sep 27, 2024

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
Copy link

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 added a commit that referenced this issue Nov 3, 2024
These were initially kept as unexported types, but it turns out they're
useful for testing. Still, they should not be initialized directly (i.e.
from within workers) and should be used through the `JobCancel` and
`JobSnooze` top level functions.

Fixes #625.
bgentry added a commit that referenced this issue Nov 3, 2024
These were initially kept as unexported types, but it turns out they're
useful for testing. Still, they should not be initialized directly (i.e.
from within workers) and should be used through the `JobCancel` and
`JobSnooze` top level functions.

Fixes #625.
@bgentry
Copy link
Contributor

bgentry commented Nov 3, 2024

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
Copy link

thanks!

tigrato pushed a commit to gravitational/river that referenced this issue Dec 18, 2024
These were initially kept as unexported types, but it turns out they're
useful for testing. Still, they should not be initialized directly (i.e.
from within workers) and should be used through the `JobCancel` and
`JobSnooze` top level functions.

Fixes riverqueue#625.
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 a pull request may close this issue.

3 participants