-
Notifications
You must be signed in to change notification settings - Fork 166
Create a ruby worker job for RISC notifications (LG-4972) #5333
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
Changes from all commits
394136c
4136127
2499f29
9ec645e
03a2240
4cb7a0f
5693abb
551ae09
403373b
03917be
de7c040
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| class RiscDeliveryJob < ApplicationJob | ||
| queue_as :low | ||
|
|
||
| retry_on Faraday::TimeoutError, Faraday::ConnectionFailed, wait: :exponentially_longer | ||
|
|
||
| def perform( | ||
| push_notification_url:, | ||
| jwt:, | ||
| event_type:, | ||
| issuer: | ||
| ) | ||
| response = faraday.post( | ||
| push_notification_url, | ||
| jwt, | ||
| 'Accept' => 'application/json', | ||
| 'Content-Type' => 'application/secevent+jwt', | ||
| ) do |req| | ||
| req.options.context = { | ||
| service_name: inline? ? 'risc_http_push_direct' : 'risc_http_push_async', | ||
| } | ||
| end | ||
|
|
||
| unless response.success? | ||
| Rails.logger.warn( | ||
| { | ||
| event: 'http_push_error', | ||
| transport: inline? ? 'direct' : 'async', | ||
| event_type: event_type, | ||
| service_provider: issuer, | ||
| status: response.status, | ||
| }.to_json, | ||
| ) | ||
| end | ||
| rescue Faraday::TimeoutError, Faraday::ConnectionFailed => err | ||
| raise err if !inline? | ||
|
|
||
| Rails.logger.warn( | ||
| { | ||
| event: 'http_push_error', | ||
| transport: 'direct', | ||
| event_type: event_type, | ||
| service_provider: issuer, | ||
| error: err.message, | ||
| }.to_json, | ||
| ) | ||
| end | ||
|
|
||
| def faraday | ||
| Faraday.new do |f| | ||
| f.request :instrumentation, name: 'request_log.faraday' | ||
| f.adapter :net_http | ||
| f.options.timeout = 3 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do you think of making these configurable?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm open to it! Would we want all Faraday instances to share the same timeout configs, or would we want it different in different contexts?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh I missed this was merged 😛 it's probably fine for all RISC events to have the same timeout, yeah. we have some independent faraday configs for timeouts like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed https://cm-jira.usa.gov/browse/LG-5044 to follow up
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| f.options.read_timeout = 3 | ||
| f.options.open_timeout = 3 | ||
| f.options.write_timeout = 3 | ||
| end | ||
| end | ||
|
|
||
| def inline? | ||
| queue_adapter.is_a?(ActiveJob::QueueAdapters::InlineAdapter) | ||
| end | ||
| end | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| require 'rails_helper' | ||
|
|
||
| RSpec.describe RiscDeliveryJob do | ||
| describe '#perform' do | ||
| let(:push_notification_url) { 'https://push.example.gov' } | ||
| let(:jwt) { JWT.encode({ foo: 'bar' }, 'a') } | ||
| let(:event_type) { PushNotification::IdentifierRecycledEvent::EVENT_TYPE } | ||
| let(:issuer) { 'issuer1' } | ||
| let(:transport) { 'ruby_worker' } | ||
|
|
||
| let(:job) { RiscDeliveryJob.new } | ||
| subject(:perform) do | ||
| job.perform( | ||
| push_notification_url: push_notification_url, | ||
| jwt: jwt, | ||
| event_type: event_type, | ||
| issuer: issuer, | ||
| ) | ||
| end | ||
|
|
||
| it 'POSTs the jwt to the given URL' do | ||
| req = stub_request(:post, push_notification_url). | ||
| with( | ||
| body: jwt, | ||
| headers: { | ||
| 'Content-Type' => 'application/secevent+jwt', | ||
| 'Accept' => 'application/json', | ||
| }, | ||
| ) | ||
|
|
||
| perform | ||
|
|
||
| expect(req).to have_been_requested | ||
| end | ||
|
|
||
| context 'network errors' do | ||
| before do | ||
| stub_request(:post, push_notification_url).to_timeout | ||
| end | ||
|
|
||
| context 'when performed inline' do | ||
| it 'warns on timeouts' do | ||
| expect(Rails.logger).to receive(:warn) do |msg| | ||
| payload = JSON.parse(msg, symbolize_names: true) | ||
|
|
||
| expect(payload[:event]).to eq('http_push_error') | ||
| expect(payload[:transport]).to eq('direct') | ||
| end | ||
|
|
||
| expect { perform }.to_not raise_error | ||
| end | ||
| end | ||
|
|
||
| context 'when performed in a worker' do | ||
| before do | ||
| allow(job).to receive(:queue_adapter). | ||
| and_return(ActiveJob::QueueAdapters::GoodJobAdapter.new) | ||
| end | ||
|
|
||
| it 'raises on timeouts (and retries via ActiveJob)' do | ||
| expect(Rails.logger).to_not receive(:warn) | ||
|
|
||
| expect { perform }.to raise_error(Faraday::ConnectionFailed) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://edgeapi.rubyonrails.org/classes/ActiveJob/Exceptions/ClassMethods.html#method-i-retry_on
The defaults here are 5 retries, and with this exponential config will go from about 3s delay to a few minutes
I think this OK for now? open to suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although the default retries is still zero (which I think is fine behavior for now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attempts: 5is the default if I read that doc correctly?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohhh, I see. I misunderstood. How does it behave with the inline job adapter? Since we won't want it retrying for minutes quite yet 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry I wasn't clear. Async worker jobs get 5x retries. Inline preserves the current behavior of logging a warning and not retrying
https://github.com/18F/identity-idp/pull/5333/files#diff-0136d3b83a32c6efebf036634bad2428cef4f659538ae4d564b7b97019880795R34-R37