Conversation
mitchellhenke
force-pushed
the
mitchellhenke/active-job-metrics-lg-4369
branch
2 times, most recently
from
b53b419 to
bb9a4a1
Compare
app/jobs/address_proofing_job.rb
Outdated
There was a problem hiding this comment.
we can probably drop these soon right?
There was a problem hiding this comment.
yeah, we can and should, I think it's safe to pass nil by default until we remove it from the functions themselves. We could keep trace_id if we wanted since it probably does exist, but I don't know how useful it would be.
lib/identity_job_log_subscriber.rb
Outdated
There was a problem hiding this comment.
WDYT of adding job_id to default_attributes?
There was a problem hiding this comment.
good call, moved it to default. It initially didn't seem like job_id existed in the enqueue event based on how Rails used it, but it is there.
mitchellhenke
force-pushed
the
mitchellhenke/active-job-metrics-lg-4369
branch
from
4e613c3 to
3526374
Compare
|
I think this is good to go with the exception of codeclimate being unhappy with the untested parts of the subscriber we unfortunately don't/can't use in the test environment. |
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM, lmk if you need admin merge override
|
@zachmargolis yes, please! 🙁 |
2 participants
The formatting and properties for the events are mostly taken from https://github.com/rails/rails/blob/v6.1.3.1/activejob/lib/active_job/log_subscriber.rb
I included all of the events, though I don't think we'll see some of them due to our limited use right now (like retries or discards)