AP-513 improving order of operations

[jason-raitz]

• co-authored with @danschmidt5189
• some logs were still not getting request_id logged
• we believe there an issue with when the two around_performs were called
• this commit should eliminate that problem

[danschmidt5189]

Curious about the test failures.

[danschmidt5189]

Since it came up on our call, I'll just note you could use find to make this more robust to argument order:

@request_id = args.select { |arg| arg.is_a? Hash }
  .find(-> {{}}) { |arg| arg.key?(:request_id) }
  .fetch(:request_id)

I'm sure there's a more syntactically sugary way to write that, too, but you get the gist. (And yes, it's inefficient to select -> find, I've just done that for clarity.)

[jason-raitz]

I meant to post this as a reply to your comment.

[jason-raitz]

I went with a destructering and reassignment.

[danschmidt5189]

FWIW I found it hard to parse the intention here, so I'd recommend refactoring for clarity. There's also a subtle bug in that if r_id_hash.any? is false, then @request_id remains undefined.

Off the dome I came up with:

def request_id
  if !defined? @request_id
    request_id_arg, other_args = arguments.partition { |arg| arg.is_a?(Hash) && arg.key?(:request_id) }
    @request_id = request_id_arg.first[:request_id] rescue nil
    self.arguments = other_args if other_args.any?
  end
  @request_id
end

This retains the usual if/end/return pattern and should fix the nil assignment bug.

[danschmidt5189]

Also maybe this is more along the lines of what @awilfox suggested, but I wonder if we need to use arguments here at all? We should be able to just set a request_id attribute directly on the job object using a callback. Only question there would be making sure it gets serialized/deserialized properly.

CurrentAttributes should be of help here.

[awilfox]

This is good, but I am a bit concerned that the mutation of arguments is done in a method. If we change how or when (or if) that method is called, suddenly the arguments will change too. I think it's better to do this in a dedicated callback, i.e. before_perform :set_request_id or something.

[anarchivist]

looks good. one clarificatory question about your docs in comments, but otherwise r+.

edit: this might be related to @awilfox's concern, too.

[anarchivist]

does this comment still apply with the restructuring you did?

[jason-raitz]

yeah. I'll fix things to go with something more like @awilfox's suggestion.

[jason-raitz]

This is good, but I am a bit concerned that the mutation of arguments is done in a method. If we change how or when (or if) that method is called, suddenly the arguments will change too. I think it's better to do this in a dedicated callback, i.e. before_perform :set_request_id or something.

I think you're right. I'll redo it. I think the reason some of the jobs are not getting the request_id right now is possibly because of multiple before_performs and around_performs making it a race condition? I'll try something different in a bit.

[danschmidt5189]

FWIW I found it hard to parse the intention here, so I'd recommend refactoring for clarity. There's also a subtle bug in that if r_id_hash.any? is false, then @request_id remains undefined.

Off the dome I came up with:

def request_id
  if !defined? @request_id
    request_id_arg, other_args = arguments.partition { |arg| arg.is_a?(Hash) && arg.key?(:request_id) }
    @request_id = request_id_arg.first[:request_id] rescue nil
    self.arguments = other_args if other_args.any?
  end
  @request_id
end

This retains the usual if/end/return pattern and should fix the nil assignment bug.

[anarchivist]

👏 i like the direction this took! r+.

[danschmidt5189]

Looks good, I like how this turned out, very clean / Rails-y.

[awilfox]

Looks great. Thanks for cleaning it up so well! r+