log stats even when filter chain gets halted

[jordan-brough]

I believe this commit will help ensure memory stats get logged even on requests that render/redirect via another filter (which could very well be requests causing memory issues).

I was tempted to put in something so that the logger would trigger even in the presence of an exception (e.g. put an 'ensure' right after the 'yield' on line 41) but I thought that might get complicated if the exception is bad enough that oink also fails, yet we wouldn't want to suppress all oink errors... Any thoughts on the subject would be appreciated. Perhaps that scenario is best left for a separate commit anyway?

[jordan-brough]

oh, i suppose it'd be good to make a similar update to instance_type_counter.rb. if the above looks ok let me know and i'll update the patch to propagate the changes to instance_type_counter.rb also.

[noahd1]

Hi Jordan -

Sorry for the long reply on this. I merged that commit in. I had to sort of convince myself that it works as you described by adding an around filter myself. I don't tend to use a lot of them. if you want to add another for instance_type_counter I'll merge it as well. Might also be great to get a test for this condition, but it would require a real controller instead of the fake one that we are using now.

I'm not sure about handling error conditions ... my initial reaction is that is a bit outside the scope of oink, that an application should get its house in order and fix errors before dealing with memory bloat, but you're right a more complete system would log those as well.

[jordan-brough]

Awesome, thanks. Yeah, I had to convince myself of around_filter also :)

I haven't needed to use instance_type_counter in production yet so I'm hesitant to make a commit there until I've really tested it out but I'm sure I'll get there if no one else does it first. One potential question there -- there don't appear to be tests that would hit that condition in instance_type_counter_spec.rb, would abstracting out the 'ApplicationController' mock from memory_usage_logger_spec.rb and adding similar tests sound like the right way to start?

Yeah, I couldn't think of a great way to test the around filter catching redirects given that we're using a mock controller in the first place.

Agreed on error conditions, doesn't seem critical.

[noahd1]

One other note about around filters in relation to oink -- this does mean that any object instantiation/memory that happens in after filters will not count towards totals. In light of the tradeoff I think this is ok, but others may not. We'll have to see and come up with a solution that meets both criteria if it's a problem.

re: a mock ApplicationController, I think that's an ok start and the easiest thing. Ultimately we'll probably have to go down the road of real controller specs and/or integration tests, but that's a lot more work and wouldn't want that to prevent this kind of contribution.

[noahd1]

hmm, you know what? the solution is to make oink rack middleware instead of using filters. Should be on the roadmap to rails3 support too.