Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add separate logging for jobs and events #85

Merged
merged 1 commit into from
Jun 4, 2015
Merged

Conversation

jcredding
Copy link
Member

This updates the PayloadHandler to do custom logging for
different types of messages. Events and jobs will now do different
logging to help differentiate what the message being processed is.
This is to avoid confusion and be explicit about what Qs background
process is doing.

This also changes the Payload to have a type_method_name method
that converts payload types to method names. This is to validate
that payload types, which can come from the redis DB, do not
execute arbitrary code via send. This was previously being done
only for the Payload module but the same process is needed for
the PayloadHandler. This formalizes the conversion so that the
PayloadHandler can make use of it.

@kellyredding - Ready for review.

This updates the `PayloadHandler` to do custom logging for
different types of messages. Events and jobs will now do different
logging to help differentiate what the message being processed is.
This is to avoid confusion and be explicit about what Qs background
process is doing.

This also changes the `Payload` to have a `type_method_name` method
that converts payload types to method names. This is to validate
that payload types, which can come from the redis DB, do not
execute arbitrary code via `send`. This was previously being done
only for the `Payload` module but the same process is needed for
the `PayloadHandler`. This formalizes the conversion so that the
`PayloadHandler` can make use of it.
Adding 10000 Jobs Time: 1.6360s
Running 10000 Jobs Time: 11.1720s
Adding 10000 Jobs Time: 1.5895s
Running 10000 Jobs Time: 11.3055s
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kellyredding - My computer does seem to be going slower today when running jobs, but I think there is about a 0.05 - 0.1s slowdown overall with this change.

I don't know why adding jobs was faster, but there is a variance 0.05s (+ or -) with these benchmarks in general.

@kellyredding
Copy link
Member

@jcredding this turned out really nice man. I love this! 👎

jcredding added a commit that referenced this pull request Jun 4, 2015
Add separate logging for jobs and events
@jcredding jcredding merged commit 60c57b9 into master Jun 4, 2015
@jcredding jcredding deleted the jcr-job-event-logging branch June 4, 2015 15:12
jcredding added a commit that referenced this pull request Aug 4, 2015
* Add `Event` (#67)
* Add `EventHandler` mixin (#68)
* Hotfix: Updating benchmark report to set new baseline (a3a88f5)
* Add `dispatcher_queue` to Qs, setup for publishing events (#69)
* Add payload type to jobs (#70)
* Update `Event` to set a custom payload type (#73)
* Add `Qs.publish` (#74)
* Add `event` to `Queue` (#75)
* Split `TestHelpers` (#72)
* Store queues as event subscribers in redis (#76)
* Add `Payload` module for serializing and deserializing (#78)
* Add `Message`, setup `Event` being a kind of `Message` (#79)
* Add `MessageHandler` (#80)
* Update runners to expect a message (#81)
* Update `Event` to be a kind of `Message` (#82)
* Add optional publisher for events (#77)
* Rename `RedisItem` to `QueueItem` (#83)
* Pass params via options when building jobs and events (#84)
* Add separate logging for jobs and events (#85)
* Add `DispatchJobHandler` (#86)
* Update dispatcher queue (#87)
* Benchmark publishing and running events (#89)
* Add daemon event handling system tests (#88)
* Add subscription system tests, fix sync (#90)

/cc @kellyredding
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants