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

Allow message_attributes to be set from ActiveJob clients #635

Merged
merged 3 commits into from
Jan 24, 2021

Conversation

timbreitkreutz
Copy link
Contributor

We'd like to be able to access the SQS message attributes from the Shoryuken client code and middleware. Previously, a memoized copy of the needed attributes were used which interfered with middleware when jobs were nested (i.e. ActiveJob clients calling other Active Jobs). In addition, we had to work implement workarounds for metrics and tracing attributes at that level.

The suggestion here is to allow these to be passed in as options which then get used as the starting point. The necessary active job attributes are then overwritten on top of any that are passed in to be sure they function the same as before.

We are open to any suggestions on style, thanks!

Co-authored by @tamouse

@timbreitkreutz timbreitkreutz changed the title Allow message_attributes in ActiveJobs Allow message_attributes to be set from ActiveJob clients Nov 2, 2020
@timbreitkreutz
Copy link
Contributor Author

timbreitkreutz commented Nov 2, 2020

@phstc Here's the nicer version of the PR (#633) we deleted a couple weeks ago. Is the utility function in code climate a blocker? Is there anything else you'd like us to change or adjust on this one?

@timbreitkreutz
Copy link
Contributor Author

Also how do we get Travis running?

@timbreitkreutz timbreitkreutz marked this pull request as ready for review November 3, 2020 17:37
@timbreitkreutz
Copy link
Contributor Author

Update: We have been running this version in our main app for a couple weeks now and have no reported issues.

@phstc
Copy link
Collaborator

phstc commented Nov 16, 2020

Hi @timbreitkreutz

I can't really review this now. But I added a form for "recruiting" Shoryuken maintainers. Let me know if you are interested 4a03b64

msg = {
message_body: body,
message_attributes: attributes.merge(MESSAGE_ATTRIBUTES)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@timbreitkreutz Sorry for taking long to get back on this.

Is it being used when enqueuing a job? MyJob.perform_later(params)

I do miss the ability to send Shoryuken specific params (for example, FIFO params) when using Active Job is this change adding this support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry was on vacation. Will try to get an answer soon :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly I don't think there is a way to inject this via perform_later--unless I'm missing something. This PR was actually aimed more at the middleware layer.

Copy link
Collaborator

@cjlarose cjlarose Jan 24, 2021

Choose a reason for hiding this comment

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

@phstc I was able to test this locally. The proposed changes do not permit specification of message_attributes when using perform_later (I've documented that as a separate issue in #646).

Instead, the following can be used to enqueue a message with additional message_attributes, without overriding the shoryuken_class attribute.

class AddJob < ActiveJob::Base
  def perform(a, b)
    puts a + b
  end
end

job = AddJob.new 1, 2
AddJob.queue_adapter.enqueue(job, message_attributes: { 'attr' => { string_value: 'myval', data_type: 'String' } })

Everything looks good from a testing perspective, so I think we're good to merge it. Although it doesn't solve #646, it doesn't hinder it and might actually be an important prerequisite.

@timbreitkreutz Thanks so much for your patience on this one!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@timbreitkreutz If you could rebase on master, I'd appreciate it. Travis should run correctly after pushing up the rebased version. 🤞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @cjlarose !

@timbreitkreutz
Copy link
Contributor Author

Fork master has been updated. Merge with squash I believe will result in a single commit to shoryuken repo?

@cjlarose
Copy link
Collaborator

Thanks! Tests are green. I'll squash merge.

@cjlarose cjlarose merged commit f0dd5b7 into ruby-shoryuken:master Jan 24, 2021
@cjlarose
Copy link
Collaborator

Whoops. I said I'd squash merge and then immediately forgot. The master branch is protected from force-pushes, so I think I'll just leave it. An extra merge commit won't hurt anyone.

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