Create a ruby worker job for RISC notifications (LG-4972)#5333
Create a ruby worker job for RISC notifications (LG-4972)#5333zachmargolis merged 11 commits intomainfrom
Conversation
- Also clean up old eventbridge code (LG-4974)
zachmargolis
requested review from
jmhooper,
mitchellhenke,
pauldoomgov and
stevegsa
| class RiscDeliveryJob < ApplicationJob | ||
| queue_as :low | ||
|
|
||
| retry_on Faraday::TimeoutError, Faraday::ConnectionFailed, wait: :exponentially_longer |
There was a problem hiding this comment.
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.
although the default retries is still zero (which I think is fine behavior for now)
There was a problem hiding this comment.
attempts: 5 is the default if I read that doc correctly?
There was a problem hiding this comment.
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.
Ah sorry I wasn't clear. Async worker jobs get 5x retries. Inline preserves the current behavior of logging a warning and not retrying
| jwt: jwt(service_provider), | ||
| event_type: event.event_type, | ||
| issuer: service_provider.issuer, | ||
| transport: 'ruby_worker', |
There was a problem hiding this comment.
if there was a way to infer if a job was performed inline vs in a thread, I'd be happy to use that instead of passing a string like this, I could not find one
There was a problem hiding this comment.
we can call queue_adapter within the job
| @@ -1,4 +0,0 @@ | |||
| module PushNotification | |||
| class PushNotificationError < StandardError | |||
There was a problem hiding this comment.
I forgot why we had this, it's unused, so it's gone now
**Why**: retry_on exceptions are unhandled and caused errors
app/jobs/risc_delivery_job.rb
Outdated
| 'Accept' => 'application/json', | ||
| 'Content-Type' => 'application/secevent+jwt', | ||
| ) do |req| | ||
| req.options.context = { service_name: 'http_push_direct' } |
There was a problem hiding this comment.
@mitchellhenke do you think this should be logged differently for workers vs inline?
There was a problem hiding this comment.
yeah, I'd make it something specific to RISC push notifications like risc_http_push_async, and the other one should probably be renamed too to match risc_http_push_direct. I kind of regret the original name being that ambiguous
| Faraday.new do |f| | ||
| f.request :instrumentation, name: 'request_log.faraday' | ||
| f.adapter :net_http | ||
| f.options.timeout = 3 |
There was a problem hiding this comment.
what do you think of making these configurable?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 outbound_connection_check_timeout, acuant_timeout, lexisnexis_timeout, etc. so we could follow that pattern?
There was a problem hiding this comment.
Filed https://cm-jira.usa.gov/browse/LG-5044 to follow up
3 participants
Does not include rate limiting by destination, that's tracked by LG-4973