bind log-weasel-id as a thread var #2

Merged
merged 3 commits into from Aug 31, 2012

Conversation

Projects
None yet
3 participants
@kenchong

bind log-weasel-id as a thread var for logging in sync

src/resque_clojure/resque.clj
- (let [{:keys [class args]} (json/read-json (:data msg))]
- (assoc msg :func class :args args)))))
+ (let [{:keys [class args context]} (json/read-json (:data msg))]
+ (assoc msg :func class :args args :context context)))))

This comment has been minimized.

Show comment Hide comment
@z00b

z00b Aug 30, 2012

Member

so upon further reflection, i'm thinking the best way to keep this general is by just replacing :data in the map with the json-parsed version. ie:

(let [{:keys [class args] :as data} (json/read-json (:data msg))]
  (assoc msg :func class :args args :data data)))))

should also save you the pain of your dynamic binding in tests

@z00b

z00b Aug 30, 2012

Member

so upon further reflection, i'm thinking the best way to keep this general is by just replacing :data in the map with the json-parsed version. ie:

(let [{:keys [class args] :as data} (json/read-json (:data msg))]
  (assoc msg :func class :args args :data data)))))

should also save you the pain of your dynamic binding in tests

This comment has been minimized.

Show comment Hide comment
@travis

travis Aug 30, 2012

I think that's right, but I think we still also want to bind the weasel id to a thread local. we don't want to need to pass this data hash to every function that might conceivably want to log...

@travis

travis Aug 30, 2012

I think that's right, but I think we still also want to bind the weasel id to a thread local. we don't want to need to pass this data hash to every function that might conceivably want to log...

This comment has been minimized.

Show comment Hide comment
@z00b

z00b Aug 30, 2012

Member

agree, but we should do that in mendocino, right?

if it were python, i'd write a decorator. is there an analogous approach in clojure?

@z00b

z00b Aug 30, 2012

Member

agree, but we should do that in mendocino, right?

if it were python, i'd write a decorator. is there an analogous approach in clojure?

src/resque_clojure/worker.clj
@@ -4,6 +4,8 @@
(def config (atom {}))
+(def ^:dynamic log-weasel-id)

This comment has been minimized.

Show comment Hide comment
@travis

travis Aug 30, 2012

yea, think rob mentioned this, but this guy needs earmuffs

@travis

travis Aug 30, 2012

yea, think rob mentioned this, but this guy needs earmuffs

This comment has been minimized.

Show comment Hide comment
@travis

travis Aug 30, 2012

+1

@kenchong

This comment has been minimized.

Show comment Hide comment
@kenchong

kenchong Aug 30, 2012

updated

updated

src/resque_clojure/worker.clj
- {:result :pass :job job :queue queue}
- (catch Exception e
- {:result :error :exception e :job job :queue queue}))))
+ (binding [*log-weasel-id* (:log_weasel_id (:context data))]

This comment has been minimized.

Show comment Hide comment
@z00b

z00b Aug 30, 2012

Member

i was trying to achieve log-weasel-id not being in this lib. am i crazy @travis ? can we just bind job-data or something?

@z00b

z00b Aug 30, 2012

Member

i was trying to achieve log-weasel-id not being in this lib. am i crazy @travis ? can we just bind job-data or something?

This comment has been minimized.

Show comment Hide comment
@z00b

z00b Aug 30, 2012

Member

maybe that's a waste of time...

@z00b

z00b Aug 30, 2012

Member

maybe that's a waste of time...

This comment has been minimized.

Show comment Hide comment
@travis

travis Aug 30, 2012

oh hi, sorry, was looking at this earlier and got distracted. it feels a little weird to bind all of the job data. probably better to bind this in the job definitions. write it out a few times in the job definitions themselves and then we can write a macro to hide the binding. really simple macro, a good test piece to advance understanding of them.

@travis

travis Aug 30, 2012

oh hi, sorry, was looking at this earlier and got distracted. it feels a little weird to bind all of the job data. probably better to bind this in the job definitions. write it out a few times in the job definitions themselves and then we can write a macro to hide the binding. really simple macro, a good test piece to advance understanding of them.

This comment has been minimized.

Show comment Hide comment
@z00b

z00b Aug 30, 2012

Member

so how would you get the map into the job? i'm trying to work around the fact that this is a generalized lib and the log-weasel stuff is very specific. but i'm about to stop caring before ken yells at me again. he's very temperamental.

@z00b

z00b Aug 30, 2012

Member

so how would you get the map into the job? i'm trying to work around the fact that this is a generalized lib and the log-weasel stuff is very specific. but i'm about to stop caring before ken yells at me again. he's very temperamental.

@kenchong

This comment has been minimized.

Show comment Hide comment
@kenchong

kenchong Aug 31, 2012

updated

updated

@z00b

This comment has been minimized.

Show comment Hide comment
@z00b

z00b Aug 31, 2012

Member

+1. needs versioning help, but i'm willing to do that.

Member

z00b commented Aug 31, 2012

+1. needs versioning help, but i'm willing to do that.

@z00b

This comment has been minimized.

Show comment Hide comment
@z00b

z00b Aug 31, 2012

Member

actually @travis do you have a plan for how we version these libs for copious-specific builds?

Member

z00b commented Aug 31, 2012

actually @travis do you have a plan for how we version these libs for copious-specific builds?

@travis

This comment has been minimized.

Show comment Hide comment
@travis

travis Aug 31, 2012

are we going to submit this change upstream? if so, I'd merge these changes to master and submit a PR and then create a new branch in which we change the group name and version number.

we can probably even just submit this to clojars so that other people can use these changes easily if they want them...

travis commented Aug 31, 2012

are we going to submit this change upstream? if so, I'd merge these changes to master and submit a PR and then create a new branch in which we change the group name and version number.

we can probably even just submit this to clojars so that other people can use these changes easily if they want them...

@travis

This comment has been minimized.

Show comment Hide comment
@travis

travis Aug 31, 2012

ping me today and we can work through any of this stuff, I waved some hands for sure...

travis commented Aug 31, 2012

ping me today and we can work through any of this stuff, I waved some hands for sure...

@z00b

This comment has been minimized.

Show comment Hide comment
@z00b

z00b Aug 31, 2012

Member

yeah, i guess we intentionally made the changes upstream-useful, so we should just do that. but need a pipeline going in the meantime, so let's work that out. maybe after the 10am.

Member

z00b commented Aug 31, 2012

yeah, i guess we intentionally made the changes upstream-useful, so we should just do that. but need a pipeline going in the meantime, so let's work that out. maybe after the 10am.

@travis

This comment has been minimized.

Show comment Hide comment
@travis

travis Aug 31, 2012

yea, nothing stopping that. I think I know how I want this to work, lemme jump on this now...

travis commented Aug 31, 2012

yea, nothing stopping that. I think I know how I want this to work, lemme jump on this now...

travis added a commit that referenced this pull request Aug 31, 2012

Merge pull request #2 from kenchong/bind-log-weasel-id
bind log-weasel-id as a thread var

@travis travis merged commit 4ad95c3 into utahstreetlabs:master Aug 31, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment