Rails Ruby on Rails → 3.0.2

Comments and changes to this ticket

• You flagged this item as spam.
  

  Justin George April 29th, 2010 @ 11:00 PM

  • Tag changed from “notifications” to “feature, notifications”

  Here's a patch that implements my suggestions

  • namespace_switch.diff 16.6 KB

• You flagged this item as spam.
  

  Justin George April 29th, 2010 @ 11:00 PM

  • Tag changed from “feature, notifications” to “feature, notifications, patch”
• 

  Jeremy Kemper April 29th, 2010 @ 11:47 PM

  • Milestone cleared.
  • State changed from “new” to “open”
  • Assigned user set to “José Valim”
• 

  José Valim April 30th, 2010 @ 09:15 AM

  • State changed from “open” to “wontfix”

  Sometimes you want to match all SQL queries, but sometimes you want to match all events that happens inside a framework.

  This is exactly the reason why you can give a regular expression to subscriber:

  ActiveSupport::Notifications.subscribe /^(\w+).sql$/

• You flagged this item as spam.
  

  Justin George April 30th, 2010 @ 07:42 PM

  José Valim:

  Maybe you can help me figure out a better namespacing system then:

  I need to add start events to most of the current notifications so I can build a stack of events for transaction tracing.

  This is important because most events do not occur within Rails' notification framework - an example being Net::HTTP calls or raw Memcached reads.

  I (New Relic) do this currently by method chaining - I think this is less than ideal, and would prefer to use notifications.

  However, it's important that these start events don't show up in the standard notifications, or you end up getting double-logging and the like.

  My thought was that a reversed namespace would allow me to exclude things listening to just sql.

• 

  José Valim April 30th, 2010 @ 08:46 PM

  • State changed from “wontfix” to “incomplete”

  Hey Justin,

  Jeremy and Yehuda convinced me about this patch so let's apply it!

  I would like to ask you to just change one thing: action_view.render_template! should be become !render_template.action_view.

  Currently, all notifications that ends with ! means that you should not be listening to them. Putting them in the middle, as you did in your patch, makes them hard to catch with a regexp. So it seems that adding them at the beginning would be inline with your patch. What do you think?

• You flagged this item as spam.
  

  Justin George April 30th, 2010 @ 09:47 PM

  Sounds great, fixed patch attached.

  Thanks for your feedback, I appreciate it.

  Justin

  • namespace_switch_2.diff 16.6 KB

• You flagged this item as spam.
  

  Justin George April 30th, 2010 @ 09:50 PM

  Whoops, missed the test cases on this - yet another patch attached.

  Thinking about this more, in the future we may want to decompose these method names (i.e. have render.template.action_view) but I will file separate tickets when I figure that stuff out.

  Justin

  • namespace_switch_3.diff 16.6 KB
• 

  Repository May 2nd, 2010 @ 09:51 PM

  • State changed from “incomplete” to “resolved”

  (from [731d4392e478ff5526b595074d9caa999da8bd0c]) Change event namespace ordering to most-significant first [#4504 state:resolved]

  More work still needs to be done on some of these names
  (render_template.action_view and render_template!.action_view particularly) but this allows (for example) /^sql/ to subscribe to all
  the various ORMs without further modification

  Signed-off-by: José Valim jose.valim@gmail.com
  http://github.com/rails/rails/commit/731d4392e478ff5526b595074d9caa...

• 

  Jeremy Kemper October 15th, 2010 @ 11:01 PM

  • Milestone set to “3.0.2”
  • Importance changed from “” to “Low”

  [bulk edit]