Allow status tracking if you enqueue the job from another app?

[hwo411]

Hi everyone.

I noticed this in our environment when pushed a job from one app that's handled by another app. The sidekiq-status middleware was enabled in both apps.

What I see in sidekiq ui is basically this:

As I understand from this code https://github.com/kenaniah/sidekiq-status/blob/main/lib/sidekiq-status/client_middleware.rb#L22-L43
the middleware searches for a class and doesn't find it, therefore does not store the status.

Is this behavior mandatory? Or can it be changed to support pushes from other apps with sidekiq status?

I understand that with this code you can reliably determine whether it's sidekiq job or not. Are there any cases where it doesn't work well if you still store the info if the class does not exist?

[kenaniah]

At face value, this use case sounds like a potential anti-pattern. Ideally, the job class should be available to both the client (invoker) and server (executor) middleware. But as I might just simply be unfamiliar, would you be able to outline a scenario in which the job class being accessible to both middlewares is not preferrable?

[kenaniah]

@hwo411 closing for now, but feel free to re-open if you have an answer to the question posted above.

[hwo411]

Oh, sorry @kenaniah , I missed your reply.

So what's our case. We push some jobs from one our app to be executed by other apps (e.g., with one that sends emails). Ideally, I'd set up RabiitMQ or another pub-sub and send the events there so that email app can just respond to them and so that one app isn't aware of the structure of another app, but at this moment we don't want to add it to our stack (but will do it later). So we went with a simple solution where we push a sidekiq job that is then executed by another app. Our decision was based on that maintaining a separate codebase is more important for us than temporary exposure of a kind of private interface.

Also, I wouldn't consider this an avoid-at-all-cost anti-pattern, because a similar example is even mentioned in sidekiq wiki/FAQ: https://github.com/sidekiq/sidekiq/wiki/FAQ#how-do-i-push-a-job-to-sidekiq-without-ruby
It shows how to do it without sidekiq at all, we still do it with Sidekiq client, just without the job on the sender side.

At the same time, I see that allowing the job that's not available on the client to have status will have the limitations with your middleware, since some parameters of the job are not available. Though I think it'd still be useful to allow the limited status instead of no status.

WDYT?

UPD: I don't seem to have an access to re-open the issue, so please reopen it after you read the message if you think this is something that you may implement in the future.

[tomasmiguez]

+1
We are having the same problem. With a similar use case.

[tomasmiguez]

@hwo411 in the meanwhile we are doing something kind of ugly but that works fine. It might be helpful for you too, but we didn't test it extensively so it may not work in some cases.

  class JobEnqueuer
    include Singleton

    def enqueue(job:, args:)
      JobStub.name = job
      Sidekiq::Client.push('class' => JobStub,
                           'args' => args)
    end

    ##
    # Currently Sidekiq::Status does not support enqueuing with an string as
    # the class parameter. We can't use the real class as it's not declared on this
    # project. Sidekiq::Status only needs the class passed as parameter to check if
    # Sidekiq::Status::Worker is an ancestor
    # (https://github.com/kenaniah/sidekiq-status/blob/main/lib/sidekiq-status/client_middleware.rb#L22-L43).
    #
    # We also need to take into account what vanilla Sidekiq expects of the
    # class passed as parameter, and it actually just calls to_s on it to get an
    # string representation of the class name, so we stub that method and make
    # it return a class instance variable that can be set to whatever we want.
    #
    # This is a known issue and we should remove this if an alternative is
    # implemented:
    #
    # https://github.com/kenaniah/sidekiq-status/issues/29
    class JobStub
      include Sidekiq::Worker
      include Sidekiq::Status::Worker

      @name = 'JobStub'

      def self.name=(name)
        raise 'Invalid name' unless name.is_a?(String)

        @name = name
      end

      def self.to_s
        @name
      end
    end
  end

[hwo411]

That's an interesting approach, thank you! Might work as a quick temporary workaround for us.

[kenaniah]

I'd be happy to consider a pull request to handle this sort of use case, as long as there is still a clean way to denote which jobs should have status info tracked.

[tomasmiguez]

I don't know how that could be achieved without modifying the gem API :/

[kenaniah]

I'll convert this to a discussion then, as someone may be able to present a workable solution down the road.

[hwo411]

Speaking about the general solution to this problem, I'd consider the following:

1. We need a config parameter whether to allow the status to unknown jobs or not. I believe there could be some projects that want to preserve this behavior.

2. If it's allowed, we have to consider 3 situations:

2.a) String was passed, class does not exist
2.b) String was passed, class exists, but not descendant of the Sidekiq::Status::Worker
2.c) Class was passed, but is not descendant of the Sidekiq::Status::Worker

In case of 2.a we probably have to use the string as name right away, in 2.b and 2.c probably use class name as name (though not sure about 2.b, it might be a coincidence that a class exists).

As for expiration, we probably have to use the default one in all 3 cases.

In addition to that, we should probably support obtaining additional data from msg argument, since one can use it to push expiration or maybe other options (though I still need to check Sidekiq::Client.push code to see if it can work like this).

I think with this solution the use case will be covered, and one still can turn on old behavior if there are any problems with the new one (e.g., class name collision).

WDYT?