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

Don't log enqueuing details when the job wasn't enqueued #49515

Merged
merged 1 commit into from Oct 11, 2023

Conversation

dustinbrownman
Copy link
Contributor

Motivation / Background

This Pull Request was created to fix a small bug introduced in #48066.

Detail

This changes the logged message when a job is started. Before, the logged message always attempted to include the enqueued_at info. Unfortunately, for jobs that are run with perform_now, they are not enqueued and instead executed immediately. This left enqueued_at as nil which in turn caused a NoMethodError to be raised when attempting to call utc on the nil value.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the activejob label Oct 6, 2023
@zzak
Copy link
Member

zzak commented Oct 6, 2023

Thanks for the PR!

Could you please squash your commits? 🙇

@dustinbrownman
Copy link
Contributor Author

@zzak Thanks for taking a look! Apologies for not squashing before opening the PR. That's been done now 👍🏻

Copy link
Member

@zzak zzak left a comment

Choose a reason for hiding this comment

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

LGTM, @bensheldon wdyt?

Copy link
Contributor

@bensheldon bensheldon left a comment

Choose a reason for hiding this comment

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

I think the change looks good in the context of avoiding the error and is a definite improvement.

In terms of "is it the ideal error message for perform_now?" it also makes me think: the log message should also exclude the queue name. But that looks harder to do and that could be looked into separately.

👍

@dustinbrownman
Copy link
Contributor Author

dustinbrownman commented Oct 7, 2023

@bensheldon That's totally fair, and something I was considering when making this change. I agree that it would be a bigger change to do this correctly. With this change I'm inferring that the job wasn't enqueued with perform_later since enqueued_at isn't set, which happens during serialization. If we want to change the log message to be something like "Performing MyJob (Job ID: 123) immediately", I'd want to be more sure that was the case, perhaps with a flag that is set during perform_now or something.

I'd love to explore that more, but I think that doing so separately is probably the way to go.

@matthewd matthewd merged commit 42efd02 into rails:main Oct 11, 2023
4 checks passed
matthewd added a commit that referenced this pull request Oct 11, 2023
Don't log enqueuing details when the job wasn't enqueued
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants