v6.0.1 completely broke sidekiq-unique-jobs

[mhenrixon]

Ruby version:
Sidekiq 6.0.1:

https://travis-ci.com/mhenrixon/sidekiq-unique-jobs/jobs/241465466 broke when you released 6.0.1.

I also had the following issue reported: mhenrixon/sidekiq-unique-jobs#427 which matches perfectly with the broken test suite.

Any ideas what the broken change might be? Seems like you should have made that bump to version 7 not 6.0.1 :)

[mhenrixon]

I'm really not trying to be sarcastic (just funny). Did you make sidekiq too fast perhaps? 🤣💣

[mperham]

The only change that I can think that might have changed some middleware semantics accidentally is #4303.

[mhenrixon]

Hmm, that does indeed look like a potential problem! Let me check if reverting that fixes the issue

[mhenrixon]

Yup @mperham when I revert the commit in https://github.com/mhenrixon/sidekiq test pass just fine.

[mperham]

I would like to understand the problem with the change. Can you send me a script or unit test which reproduces the behavior?

[mhenrixon]

It seems (in test suite also) that since 6.0.1 no jobs get actually pushed to redis anymore. This is of course because of the middelware handling or something.

By closer examination it doesn't seem like the marshaling/performance PR.

[mhenrixon]

class BuildDistributorWorker
  include Sidekiq::Worker

  sidekiq_options lock: :until_executing, log_duplicate_payload: true, queue: :build_distributors

  def perform(id); end
end

That type of lock should always allow to be pushed, it only locks during processing (when the server processor is picking it off the queue). My guess is that some part of the code that either deals with the push or the middleware changed in 6.0.1.

[mhenrixon]

Pretty sure this broken the build: daabec1

[fatkodima]

It seems like changes in logger context handling (one pr after this) broke this.
This code from your gem

  def with_context(context, &block)
    if logger.respond_to?(:with_context)
      logger.with_context(context, &block)
    elsif defined?(Sidekiq::Logging)
      Sidekiq::Logging.with_context(context, &block)
    end
  end

not recognizes new changes (Sidekiq::Context.with), so not working as expected.

[mhenrixon]

Too late @fatkodima I already pointed that out :)

[fatkodima]

That's because I was typing this here slowly 😄

[mhenrixon]

That code is naive anyway but gosh it is getting annoying to maintain all the various logging contexts from sidekiq 🤣

Can you just decide on a way to do this and stick to it? 🥳🤨

[mhenrixon]

Ok, I'll add some tests for it. My problem is that I want to piggy-back on the sidekiq logger to provide some sensitive extra values to the logger. Maybe I should just scrap that and build my own instead. Been thinking about it for some time but it always seemed so unnecessary.

[mhenrixon]

Closing this, thanks for the quick feedback everyone!

PS. I love this last refactoring you did on Sidekiq::Context @mperham. The Sidekiq::Logging or whatever used to be pretty hairy if you ask me!

Ever thought of using something like twp/logging which kind of does a lot of that stuff already?