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 the generation of Action Mailer messages #12556

Merged
merged 1 commit into from Oct 20, 2013

Conversation

@dasch
Copy link
Contributor

@dasch dasch commented Oct 16, 2013

Currently, only the time spent sending a mail is instrumented in Action Mailer. The time it takes to render the views and building the mail object itself is not instrumented at all.

This PR adds very basic instrumentation. More data, such as view rendering time, database time, and really anything, can be added later.

@josevalim
josevalim reviewed Oct 16, 2013
View changes
actionmailer/lib/action_mailer/base.rb Outdated
@@ -512,10 +512,14 @@ def initialize(method_name=nil, *args)
end

def process(*args) #:nodoc:

This comment has been minimized.

@josevalim

josevalim Oct 16, 2013
Contributor

I am wondering if it is worthy breaking ActionMailer::Base in two modules now. One with all the delivery stuff and another one on top that would add the instrumentation, following ActionController pattern.

This comment has been minimized.

@dasch

dasch Oct 16, 2013
Author Contributor

I was thinking that - I actually had a branch based off the first one that did this, but obviously it didn't work since it hooked into deliver_mail, which only happens after the mail has been generated.

Would it be okay to get this in first and then work on a refactoring later? It would probably require moving the existing #process code into a module (Implementation) and then including that and a separate Instrumentation module. It could easily obfuscate the content of this PR.

This comment has been minimized.

@dasch

dasch Oct 16, 2013
Author Contributor

I also want to use the same mechanism for including additional messages in the log line, e.g. Active Record includes a module that implements an add_info_to_payload method.

@dasch
Copy link
Contributor Author

@dasch dasch commented Oct 16, 2013

Just a few questions:

  • Is "generate" the right word to use here?
  • Do you want the log to be in a different format?
@josevalim
josevalim reviewed Oct 16, 2013
View changes
actionmailer/lib/action_mailer/base.rb Outdated

super
@_message = NullMail.new unless @_mail_was_called
ActiveSupport::Notifications.instrument("generate.action_mailer", payload) do

This comment has been minimized.

@josevalim

josevalim Oct 16, 2013
Contributor

maybe process.action_mailer to mimic action controller as well?

@josevalim
josevalim reviewed Oct 16, 2013
View changes
actionmailer/lib/action_mailer/log_subscriber.rb Outdated
def generate(event)
return unless logger.info?
mailer = event.payload[:mailer]
info("\n#{mailer}: generated mail in #{event.duration.round(1)}ms")

This comment has been minimized.

@josevalim

josevalim Oct 16, 2013
Contributor

debug maybe? If we have lived all this time without it, it feels like debug would suffice. :)

This comment has been minimized.

@dasch

dasch Oct 16, 2013
Author Contributor

Okay :-)

@josevalim
Copy link
Contributor

@josevalim josevalim commented Oct 16, 2013

The logged message is fine, we can always adjust it later. :)

@dasch
Copy link
Contributor Author

@dasch dasch commented Oct 16, 2013

@josevalim should I include the mailer method in the instrumentation? Right now only the name of the mailer itself is included.

@dasch
Copy link
Contributor Author

@dasch dasch commented Oct 16, 2013

Okay, I've included the action - seems to be prudent. Do you want me to rebase?

@josevalim
Copy link
Contributor

@josevalim josevalim commented Oct 16, 2013

Yes, please rebase. I have also added a message about the event name. IMO it should be called process instead of generate. Any reason why not to?

@dasch
Copy link
Contributor Author

@dasch dasch commented Oct 16, 2013

Nah, I'll change it to "process".

@dasch
Copy link
Contributor Author

@dasch dasch commented Oct 16, 2013

Actually, "process" makes it sound like we handle an incoming mail, "an email was processed".

@dasch
Copy link
Contributor Author

@dasch dasch commented Oct 16, 2013

Naming is hard... what about "render"? Although actual rendering happens multiple times :-/

@dasch
Copy link
Contributor Author

@dasch dasch commented Oct 16, 2013

Okay, I've renamed it to "process" and squashed. I guess we'll find out if the name is confusing or not. If you merge this I'll start work on refactoring and adding support for view rendering time and db time.

@pftg
pftg reviewed Oct 16, 2013
View changes
actionmailer/lib/action_mailer/base.rb Outdated
def process(method_name, *args) #:nodoc:
payload = {
:mailer => self.class.name.to_s,
:action => method_name

This comment has been minimized.

@pftg

pftg Oct 16, 2013
Contributor

May you use 1.9 hash style?

This comment has been minimized.

@dasch

dasch Oct 16, 2013
Author Contributor

Is that the convention in Rails nowadays?

@pftg
pftg reviewed Oct 16, 2013
View changes
actionmailer/lib/action_mailer/base.rb Outdated
lookup_context.skip_default_locale!
def process(method_name, *args) #:nodoc:
payload = {
:mailer => self.class.name.to_s,

This comment has been minimized.

@pftg

pftg Oct 16, 2013
Contributor

Are there any needs to call to_s?

@dasch
Copy link
Contributor Author

@dasch dasch commented Oct 17, 2013

The build is failing due to some other thing :-/

The processing of outbound mail is instrumented with the key
`process.action_mailer`. The payload includes the mailer name as well as
the mailer method.
@robin850
Copy link
Member

@robin850 robin850 commented Oct 20, 2013

@dasch : Yes, this is not your fault ; this failure is present in lots of pull requests.

@josevalim : Could you please review and merge this one if everything is ok ? :-)

josevalim added a commit that referenced this pull request Oct 20, 2013
Instrument the generation of Action Mailer messages
@josevalim josevalim merged commit fdfc967 into rails:master Oct 20, 2013
1 check failed
1 check failed
default The Travis CI build failed
Details
@dasch
Copy link
Contributor Author

@dasch dasch commented Oct 21, 2013

Thanks! I'll get started on more granular metrics as soon as I have the time.

@robin850
Copy link
Member

@robin850 robin850 commented Oct 21, 2013

Thank you guys!

@dasch dasch deleted the dasch:dasch/nstrument-am-processing branch Apr 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.