🚀 Que integration #361

tmiller · 2017-11-01T00:56:20Z

Description

Add instrumentation and exception reporting for Que.

Instrumentation is added by using meta programming around the Que::Job#_run.
Error reporting is implemented by passing an error handler to Que#error_handler since Que already handles exceptions.

tmiller · 2017-11-01T02:03:00Z

I don't believe the exception reporting is working just yet, so please hold off on pulling this.

tmiller · 2017-11-01T02:36:46Z

It seems to be working. I was running into problems because I was running the job synchronously, which doesn't not call _run but instead calls run directly so you can debug exceptions in the rails server or console.

tombruijn · 2017-11-01T11:10:27Z

Hi @tmiller ,

Thanks for the PR!
Looks like it's using some outdated test suite setup. We're slowly trying to rewrite everything in issue #252 . I would like to rewrite the specs to use that before merging.

Also, do you have an example app you can share with us? Something we can add to this repo: https://github.com/appsignal/appsignal-examples

tmiller · 2017-11-03T21:40:37Z

@tombruijn I've made the changes, can you please review the changes to the tests? I am also working on setting up an example application as requested.

tombruijn · 2017-11-07T12:49:25Z

@tmiller That's great! Thanks for already updating the specs to use Appsignal::Transaction#to_h!
I'll review it and let you know if any further changes are needed.

tombruijn

@tmiller I have some minor comments about the integration. Otherwise it looks really good! I'll see if I can set up an example app today, unless you already have one?

tombruijn · 2017-11-07T12:53:57Z

lib/appsignal/hooks/que.rb

+      def install
+        require "appsignal/integrations/que"
+        ::Que::Job.send(:include, Appsignal::Integrations::QuePlugin) unless
+          ::Que::Job.included_modules.include?(Appsignal::Integrations::QuePlugin)


we only install the integration once so a check to see if the integration is already present shouldn't be necessary here.

tombruijn · 2017-11-07T12:56:24Z

lib/appsignal/integrations/que.rb

+              transaction.set_error(error)
+              raise error
+            ensure
+              transaction.set_http_or_background_action(request.env)


if possible we want to minimize relying on this method. If we already know the class and method name combination in the integration itself, we can use Appsignal::Transaction#set_action instead.

In this case:

transaction.set_action "#{cls}#run"

Then the class and method name wouldn't have to be added to the request env on lines 11 and 12. Since this is not really a request, but a background job.

tombruijn · 2017-11-07T12:58:28Z

spec/lib/appsignal/integrations/que_spec.rb

@@ -0,0 +1,172 @@
+if DependencyHelper.que_present?
+  describe "Que integration" do


Can you use the integration class name here?

Appsignal::Integrations::QuePlugin

tmiller · 2017-11-08T14:51:43Z

Here you go https://github.com/ProctorU/appsignal-examples/tree/rails-5+que

tombruijn · 2017-11-08T15:07:05Z

@tmiller That's amazing! I was just starting to set up an example app 😁

I'll try it out, test the integration you wrote and merge it into the examples repo

Let me know if you still have time to fix the comments I had earlier. If you don't have time, I'll push the fixes to this PR. After that I'll submit it to review for the rest of the team.

tmiller · 2017-11-08T15:15:18Z

Thanks I have time, they will be done in a few minutes :)

Que catches all exceptions instead of rethrowing them to the caller. This causes exceptions not to be logged in app signal. In addition to adding the QuePlugin integration to Que, also set the error notification handler so exceptions are logged to Appsignal.

QuePlugin is using Appsignal::monitor_transaction which setups up exception handling, but Que already handles exceptions and has a notifier that can be used to send excpetion to Appsignal. This refactor removes the exception handling and the logic checking for HTTP requests.

When using Transaction#set_action, we shouldn't need to set the class and method in the request since this isn't a web request.

tombruijn · 2017-11-08T16:23:56Z

@tmiller upon reviewing it against the example I noticed that ActiveJob is not tested in the test suite? I was trying something myself in this branch: https://github.com/appsignal/appsignal-ruby/tree/aj-que-integration
but so far have had no luck with it. Do you have any experience with testing it directly like so?

If this doesn't work we may have to revise the tests to only test the plugin itself without being included in the Que::Job class. I wouldn't want to have to add the postgresql dependency either.

tmiller · 2017-11-08T16:39:07Z

We don't use ActiveJob. I think that was left over from the code before I picked it up. I haven't tried testing it, and I won't be able to work on adding tests and correct support for ActiveJob until Friday. I'm ok with removing the ActiveJob parts if you are?

tombruijn · 2017-11-09T10:28:21Z

@tmiller we've discussed it here and we're okay with merging it without the ActiveJob support. I'd rather we take the ActiveJob logic out entirely and have another look at it in issue #330

tmiller · 2017-11-09T20:55:30Z

Thanks! I've removed the code, anything else you need?

tombruijn · 2017-11-10T09:26:18Z

@tmiller no, I think that's it! I've requested reviews from the team. Let's see if anyone else has any comments :)

tombruijn · 2017-11-13T09:35:55Z

Thanks again @tmiller for the PR!
We'll try to release this in the upcoming 2.4.1 release. Which we hope will be sometime this week. It's waiting for PR #366 to be merged and tested.

tombruijn · 2017-11-13T15:58:42Z

@tmiller this is now released in AppSignal gem v2.4.1 🎉

tmiller · 2017-11-13T19:24:21Z

Thanks, for all the help!

tmiller force-pushed the que-integration branch from 2731cb2 to 4f82055 Compare November 1, 2017 00:59

tombruijn added the feature label Nov 1, 2017

tombruijn changed the title ~~Que integration~~ 🚀 Que integration Nov 1, 2017

tmiller force-pushed the que-integration branch 2 times, most recently from 0e04893 to b69f77b Compare November 3, 2017 21:39

tombruijn self-assigned this Nov 6, 2017

tombruijn reviewed Nov 8, 2017

View reviewed changes

wvengen and others added 11 commits November 8, 2017 10:10

Add Que integration

fc40c3c

Support Ruby 1.9.3 again with Que integration

c8d4a92

Show ActiveJob class with Que

92f7224

Add que gemfile

c253f9e

Add que to tests

7850c0c

Remove unecessary check in QueHook#install

876f3c0

Use set_action over set_http_or_background_action

4662719

When using Transaction#set_action, we shouldn't need to set the class and method in the request since this isn't a web request.

Describe the class being tested

6909375

Remove unecessary rubocop disable

f2c1ff8

tmiller force-pushed the que-integration branch from 6220982 to f2c1ff8 Compare November 8, 2017 16:10

Remove ActiveJob support in QuePlugin

68b0600

tombruijn requested review from thijsc and jeffkreeftmeijer November 10, 2017 09:25

thijsc approved these changes Nov 10, 2017

View reviewed changes

jeffkreeftmeijer approved these changes Nov 10, 2017

View reviewed changes

tombruijn merged commit 6ef8dcf into appsignal:master Nov 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚀 Que integration #361

🚀 Que integration #361

tmiller commented Nov 1, 2017

tmiller commented Nov 1, 2017 •

edited

Loading

tmiller commented Nov 1, 2017

tombruijn commented Nov 1, 2017

tmiller commented Nov 3, 2017

tombruijn commented Nov 7, 2017

tombruijn left a comment

tombruijn Nov 7, 2017

tombruijn Nov 7, 2017

tombruijn Nov 7, 2017

tmiller commented Nov 8, 2017

tombruijn commented Nov 8, 2017

tmiller commented Nov 8, 2017 •

edited

Loading

tombruijn commented Nov 8, 2017

tmiller commented Nov 8, 2017

tombruijn commented Nov 9, 2017

tmiller commented Nov 9, 2017

tombruijn commented Nov 10, 2017

tombruijn commented Nov 13, 2017

tombruijn commented Nov 13, 2017

tmiller commented Nov 13, 2017

		@@ -0,0 +1,172 @@
		if DependencyHelper.que_present?
		describe "Que integration" do

🚀 Que integration #361

🚀 Que integration #361

Conversation

tmiller commented Nov 1, 2017

Description

tmiller commented Nov 1, 2017 • edited Loading

tmiller commented Nov 1, 2017

tombruijn commented Nov 1, 2017

tmiller commented Nov 3, 2017

tombruijn commented Nov 7, 2017

tombruijn left a comment

Choose a reason for hiding this comment

tombruijn Nov 7, 2017

Choose a reason for hiding this comment

tombruijn Nov 7, 2017

Choose a reason for hiding this comment

tombruijn Nov 7, 2017

Choose a reason for hiding this comment

tmiller commented Nov 8, 2017

tombruijn commented Nov 8, 2017

tmiller commented Nov 8, 2017 • edited Loading

tombruijn commented Nov 8, 2017

tmiller commented Nov 8, 2017

tombruijn commented Nov 9, 2017

tmiller commented Nov 9, 2017

tombruijn commented Nov 10, 2017

tombruijn commented Nov 13, 2017

tombruijn commented Nov 13, 2017

tmiller commented Nov 13, 2017

tmiller commented Nov 1, 2017 •

edited

Loading

tmiller commented Nov 8, 2017 •

edited

Loading