xinminlabs / newrelic-rake

NewRelic instrument for rake task
MIT License
29 stars 11 forks source link

Remove at_exit shutdown hook. #13

Closed wireframe closed 10 years ago

wireframe commented 10 years ago

Newrelic agent already handles at_exit flushing and installing extra at_exit handler causes bugs with ruby's exit code handling. https://bugs.ruby-lang.org/issues/5218

This issue can be reproduced by:

http://www.davekonopka.com/2013/rspec-exit-code.html

wireframe commented 10 years ago

thanks @flyerhzm for merging! would you mind releasing an updated gem with this change?

flyerhzm commented 10 years ago

@wireframe just released 1.4.1 version, thanks for contributing.

jonchay commented 10 years ago

What is your situation for removing NewRelic::Agent.shutdown? Ours stopped reporting to NewRelic when we upgrade newrelic-rake to 1.4.1. We had to rollback to use 1.4.0.

flyerhzm commented 10 years ago

@jonchay could you try upgrading to latest newrelic gem?

jonchay commented 10 years ago

@flyerhzm According to NewRelic it wasn't a gem issue, it was Sinatra. Please review the response NewRelic gave to me:

"Generally speaking, the agent will automatically register an at_exit hook to send data upon shutdown, however there are a few exceptions to this rule. The one that's probably most relevant here is that the agent won't install the at_exit hook when it detects that the Sinatra::Application constant is defined. Looking at the Gemfile.lock for your application, it looks like you do have Sinatra present, which is probably why the agent is not installing its at_exit hook."

Looks like newrelic-rpm does have a check like this: https://github.com/newrelic/rpm/blob/7f7e9f4a3dee08f0eaf884bd26d48869e57a9583/lib/new_relic/agent/agent.rb#L359-L365. We may have to borrow this check before executing the deleted at_exit { } block.

flyerhzm commented 10 years ago

@jonchay as newrelic has already installed exit handler by itself, I don't think newrelic-rake should repeat it again, I just updated README to mention that if there is sinatra gem in Gemfile.lock, user has to manually install exit handler.

jonchay commented 10 years ago

Sounds good. Thanks!