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 methods for including start events with notified actions #676

Closed
lighthouse-import opened this issue May 16, 2011 · 14 comments
Closed

Comments

@lighthouse-import
Copy link

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/4594
Created by Justin George - 2010-05-13 22:37:30 UTC

The use case here is essentially for callback-esque use cases - we want to note when a sql transaction has begun, and when it has ended, as seperate events.

A use case for this is to notice when sql queries are hanging, or for noticing which order queries are starting in without reference to their final length.

ActiveSupport::Notifications.subscribe(/sql.active_record/) do |name, *args|
  @events << name
end

ActiveSupport::Notifications.instrument_with_start('sql.active_record', {:some => 'payload'}) do
  # query logic
end

@events.inspect #=> ['sql.active_record.start', 'sql.active_record']

Open to suggestions on what to call it, and how it should work.

@lighthouse-import
Copy link
Author

Imported from Lighthouse.
Comment by Justin George - 2010-05-13 22:40:15 UTC

Patch attached

@lighthouse-import
Copy link
Author

Imported from Lighthouse.
Comment by Rizwan Reza - 2010-05-17 19:08:11 UTC

+1 Verified. This patch applies cleanly and all tests pass.

@lighthouse-import
Copy link
Author

Imported from Lighthouse.
Comment by Jeremy Kemper - 2010-05-17 19:47:27 UTC

Justin, we should always fire start events. Could you merge this with instrument rather than introduce a new API?

@lighthouse-import
Copy link
Author

Imported from Lighthouse.
Comment by Justin George - 2010-05-17 20:40:39 UTC

There's a couple situations where you don't want it, !render_template.action_view being the main one where 1000s of calls may be mode.

I will rename the old instrument method to be instrument_without_start, or something like that, yes? Or make it take an optional parameter? I'm not sure which is the proper way.

def instrument(event, payload={})
  ...
end

def instrument_without_start(event, payload={})
  ...
end

versus

def instrument(event, payload={}, start=true)
  @notifier.publish(...) if start
  ...
end

Thanks for the feedback, I was less sure about this change.

@lighthouse-import
Copy link
Author

Imported from Lighthouse.
Comment by Repository - 2010-05-21 19:43:23 UTC

(from [f3abc8a]) Use multibyte proxy class on 1.9, refactor Unicode.

Makes String#mb_chars on Ruby 1.9 return an instance of ActiveSupport::Multibyte::Chars to work around 1.9's lack of Unicode case folding.

Refactors class methods from ActiveSupport::Multibyte::Chars into new Unicode module, adding other related functionality for consistency.

[#4594 state:resolved]

Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/f3abc8ac36055afed9fcc902c33ee146e066d17a

@lighthouse-import
Copy link
Author

Imported from Lighthouse.
Comment by Jeremy Kemper - 2010-05-25 21:19:38 UTC

Incorrectly resolved.

@lighthouse-import
Copy link
Author

Imported from Lighthouse.
Comment by Justin George - 2010-05-26 00:19:05 UTC

Okay, here we are, after a brief hiatus, a proper patch for this.

I think that the case I was worrying about before (!render_template) is not that big a deal - if it's a problem we can fix it later.

@lighthouse-import
Copy link
Author

Imported from Lighthouse.
Comment by Rohit Arondekar - 2010-06-17 06:16:18 UTC

Justin, this patch doesn't apply any more to Rails master. Can you fix it?

@lighthouse-import
Copy link
Author

Imported from Lighthouse.
Comment by Jeremy Kemper - 2010-08-30 03:10:32 UTC

[bulk edit]

@lighthouse-import
Copy link
Author

Imported from Lighthouse.
Comment by Jeremy Kemper - 2010-10-15 22:01:53 UTC

[bulk edit]

@lighthouse-import
Copy link
Author

Imported from Lighthouse.
Comment by Santiago Pastorino - 2010-11-15 21:15:12 UTC

[bulk edit]

@lighthouse-import
Copy link
Author

Imported from Lighthouse.
Comment by Roger Leite - 2011-01-04 19:08:46 UTC

Hi, I made a fix to this patch.
Can you please check if it's ok ?

@lighthouse-import
Copy link
Author

Imported from Lighthouse.
Comment by Dan Pickett - 2011-02-04 22:43:59 UTC

Robert's patch currently applies cleanly to master (despite a trailing whitespace warning), with all tests passing

@lighthouse-import
Copy link
Author

Attachments saved to Gist: http://gist.github.com/971669

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

No branches or pull requests

2 participants