fix: Sidekiq spans crashing when enqueued by Exq #1796
Conversation
Exq doesn't seem to pass "created_at" with its messages. [Current Sidekiq API](https://github.com/sidekiq/sidekiq/blob/f9e0a026543c93ae49c1d1d41c6592d2877d318a/lib/sidekiq/api.rb#L456) defaults `created_at` to `enqueued_at` or `0` if it's not available, so we can mimic that method for safely defaulting thee values.
xathien
requested review from
a team,
ahayworth,
arielvalentin,
dazuma,
ericmustin,
fbogsany,
kaylareopelle,
mwear,
plantfansam,
robbkidd,
robertlaurin,
simi and
xuan-cao-swi
as code owners
| created_at = time_from_timestamp(msg['created_at'] || msg['enqueued_at'] || 0) | ||
| enqueued_at = time_from_timestamp(msg['enqueued_at'] || 0) |
There was a problem hiding this comment.
Won't this end up loosing meaning? Would it not be better to omit the span events in these cases?
There was a problem hiding this comment.
Probably! I'll admit I'm not an opentelemetry expert; just trying to fix the crash. 🙂
Would it be more advisable to let time_from_timestamp handle/return nil and skip the add_event in those cases?
I think letting the created_at default to enqueued_at when not specified can still be useful, but maybe not so much the 0 default (which I only selected since Sidekiq's API does that).
There was a problem hiding this comment.
IMHO, If the value is not set, we shouldn't have any event. Not provide misleading data.
There was a problem hiding this comment.
@xathien think then that it's best to check if the values are present and only add the events if that's the case. I don't think the timestamp helper should tolerate nil values at this time.
There was a problem hiding this comment.
Sounds good to me! I've updated the change accordingly.
dmathieu
left a comment
dmathieu
left a comment
There was a problem hiding this comment.
This will need a changelog entry
| _(job_span.attributes['messaging.operation']).must_equal 'process' | ||
| end | ||
|
|
||
| it 'traces when enqueued through another system' do |
There was a problem hiding this comment.
I would name this as "when enqueued with minimal data"
| end | ||
|
|
||
| it 'traces when enqueued through another system' do | ||
| payload = { 'queue' => 'default', 'args' => [], 'class' => SimpleJob, 'enqueued_at' => Time.current, 'jid' => '4' } |
There was a problem hiding this comment.
Shouldn't enqueued_at be removed here too?
There was a problem hiding this comment.
Exq supplies this one, but not created_at from what I can tell. In addition, Sidekiq::Client#raw_push adds enqueued_at on its own. I could try doing some shenanigans to push the job directly to Redis, but I'm not sure that effort would be worth the slight test improvement.
There was a problem hiding this comment.
IMHO, it's not about Exq. It's about what any custom library could do. If it's possible to enqueue a job and have it processed by sidekiq without an `enqueued_at, then we should handle it.
If sidekiq would auto-add it, or reject processing it then, we're good.
@dmathieu our release tooling auto-generates the changelog based on the title of the PR/merge commit so there isn't a need to create a manual entry. |
3 participants
Exq doesn't seem to pass "created_at" with its messages. Current Sidekiq API defaults
created_attoenqueued_ator0if it's not available, so we can mimic that method for safely defaulting these values.Fixes #1795