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

Instrument filters in the pipeline #45

Merged
merged 5 commits into from
Mar 29, 2013

Conversation

mtodd
Copy link
Contributor

@mtodd mtodd commented Mar 28, 2013

This makes HTML::Pipeline compatible with ActiveSupport::Notifications instrumentation.

Shuffles things around a bit but adds instrumenting to every filter call.

@jch I know we talked about putting the instrumentation on the individual filters but I decided to keep it here since I wanted to make it easy to make the instrumentation service object available. The pipeline felt like the right place to put that, and the individual filter wasn't aware of the pipeline object as far as I could see.

@jch
Copy link
Contributor

jch commented Mar 28, 2013

How about adding an instrumentation_service class attribute in addition to the instance attribute? If you have several pipelines defined, it'd be nice to default the instance attribute from the class one.

HTML::Pipeline.default_instrumentation_service = SomeService.new

@mtodd
Copy link
Contributor Author

mtodd commented Mar 28, 2013

@jch 👓

@jch
Copy link
Contributor

jch commented Mar 29, 2013

Hawt. Add a blurb about this in the readme?

@mtodd
Copy link
Contributor Author

mtodd commented Mar 29, 2013

@jch now that I write the README, does it make sense to instrument the whole pipeline call, too? That would fire off a call_pipeline.html_pipeline event after all of the filters have finished.

@mtodd
Copy link
Contributor Author

mtodd commented Mar 29, 2013

@jch
Copy link
Contributor

jch commented Mar 29, 2013

👍 adding an instrument to the pipeline call. I like that it gives you a quick total for all the filters. Thanks for the extra readme cleanup too.

@brianmario
Copy link
Contributor

niiice

@mtodd
Copy link
Contributor Author

mtodd commented Mar 29, 2013

@jch boom. Whatcha think?

jch added a commit that referenced this pull request Mar 29, 2013
Instrument filters in the pipeline
@jch jch merged commit a99e0b3 into gjtorikian:master Mar 29, 2013
@jch
Copy link
Contributor

jch commented Mar 29, 2013

🤘 perfect sir. I'll cut a release after ci passes

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.

3 participants