proletarian icon indicating copy to clipboard operation
proletarian copied to clipboard

Passing job-id to worker handler-fn

Open tangrammer opened this issue 2 years ago • 7 comments

Hello!

is there any reason of not passing job-id besides job-type to the worker handler-fn? I think it could be useful to identify the job execution context with this uuid (thinking for example in monitoring)

Are you interested on this change so i could try to tackle it in a PR? i suspect that this change should need a change in the create-queue-worker API :thinking: ... but as far as it's data oriented we could add something like handler-fn-v2 alike

what do you think?

thanks in advance for this great library!

tangrammer avatar Apr 11 '23 06:04 tangrammer

Hi, and thanks for your interest in Proletarian!

I've gone back and forth in my mind about exposing job-id to the job handling function. In the end I've decided that it's an implementation detail that should not leak out to consumers. I can see that I'm not consistent here, as enqueue! returns the job-id as it stands. You should think about that value more as a diagnostic value, not to be used in application code.

However, your use case remains valid. I'd suggest using the payload to store metadata (not in the Clojure sense) about your jobs. We're doing this at work to propagate OpenTelemetry tracing data (using https://github.com/steffan-westcott/clj-otel). Here's more or less how we do it, using a pair of wrapper functions that read and write from the payload, but in a way that is transparent to the rest of the application code:

(ns my-app.infrastructure.worker-jobs
  (:require
    [proletarian.job :as job]
    [proletarian.protocols]
    [steffan-westcott.clj-otel.api.trace.span :as otel.span]
    [steffan-westcott.clj-otel.context :as otel.context]))

(defn with-trace-data
  [payload]
  (let [trace-context (otel.context/->headers)]
    (cond-> payload
      (seq trace-context) (assoc ::trace-context trace-context))))

(defn enqueue!
  [conn queue job-type payload]
  (assert (map? payload) "Worker job payload must be a map")
  (otel.span/with-span! {:name       (str `enqueue!)
                         :span-kind  :producer
                         :attributes {:queue    queue
                                      :job-type job-type
                                      :payload  payload}}
    (job/enqueue! conn job-type (with-trace-data payload) {:proletarian/queue queue})))

(defmulti handle-job!
  (fn [job-type _payload] job-type))

(defn handle-job-wrapper!
  [job-type payload]
  (let [trace-context (some-> (::trace-context payload) (otel.context/headers->merged-context))
        payload       (dissoc payload ::trace-context)]
    (otel.span/with-span! {:name       (str job-type)
                           :parent     trace-context
                           :attributes {:payload payload}
                           :span-kind  :consumer}
      (handle-job! job-type payload))))

handle-job-wrapper! would be the function that you pass to create-queue-worker. And you'd use enqueue! in your application code instead of using proletarian.job/enqueue! directly.

How would this approach work for you?

msolli avatar Apr 11 '23 11:04 msolli

Hi Martin! and sorry for the big delay on your detailed answer :grimacing:

after thinking a lot about this sentece

In the end I've decided that it's an implementation detail that should not leak out to consumers.

I really don't get the "deep" reasons of not having something so useful as an (U)ID ... I fully respect that you've decided it but can't imagine why you consider an uuid as detail implementation instead of just identity data

Thinking aloud now :sweat_smile: ... couldn't be considered Identity something universal?

thanks again!

tangrammer avatar Aug 28 '23 18:08 tangrammer

I think in the end I don't want to prematurely commit to a signature for the handler-fn that has too much information in it. I haven't seen any use for the id in the arguments to the handler-fn, but I could be wrong. What would you use it for?

msolli avatar Aug 31 '23 06:08 msolli