Merge lp:~james-w/python-oops-celery/hooks-based into lp:python-oops-celery

[James Westby]

Switch to a hooks-based approach for populating the report.

Hi,

I switched to be more like python-oops-wsgi and use hooks for
populating the report.

This both makes the code a bit simpler, and allows more flexibility
for people to re-use only parts of the code.

Thanks,

James

[lp:~james-w/python-oops-celery/hooks-based]

6. By James Westby on 2012-01-26

Merged oops-reporter into hooks-based.

[Sidnei da Silva (sidnei)]

Looks great, +1!

review: Approve

=== modified file 'oops_celery/oops_reporter.py'
--- oops_celery/oops_reporter.py	2012-01-26 19:14:26 +0000
+++ oops_celery/oops_reporter.py	2012-01-26 19:14:27 +0000
@@ -1,26 +1,28 @@
 from celery import signals
 
 
+def attach_celery_info(report, context):
+    for key, value in context.items():
+        if key.startswith('celery.'):
+            report[key] = value
+
+
 class TaskOopsReporter(object):
 
     def __init__(self, config):
         self.config = config
 
-    def on_failure(self, signal=None, sender=None, task_id=None, exception=None,
-                   args=None, kwargs=None, einfo=None, traceback=None):
+    def on_failure(self, einfo=None, **kwargs):
         context = dict()
         if einfo is not None:
             context['exc_info'] = einfo.exc_info
+        for key, value in kwargs.items():
+            context['celery.' + key] = value
         report = self.config.create(context)
-        report.update({
-            'celery.task_id': task_id,
-            'celery.task_args': args,
-            'celery.task_kwargs': kwargs,
-            'celery.sender': sender,
-        })
         self.config.publish(report)
 
 
 def setup_oops_reporter(config):
+    config.on_create.append(attach_celery_info)
     reporter = TaskOopsReporter(config)
     signals.task_failure.connect(reporter.on_failure, weak=False)
 
=== modified file 'oops_celery/tests/test_oops_reporter.py'
--- oops_celery/tests/test_oops_reporter.py	2012-01-26 19:14:26 +0000
+++ oops_celery/tests/test_oops_reporter.py	2012-01-26 19:14:27 +0000
@@ -6,7 +6,32 @@
 import oops
 from testtools import TestCase
 
-from oops_celery.oops_reporter import setup_oops_reporter, TaskOopsReporter
+from oops_celery.oops_reporter import (
+    attach_celery_info,
+    setup_oops_reporter,
+    TaskOopsReporter,
+    )
+
+
+class AttachCeleryInfoTests(TestCase):
+
+    def test_adds_all_celery_keys(self):
+        context = {
+            'celery.foo': 'foo',
+            'celery.bar': 'bar',
+        }
+        report = {}
+        attach_celery_info(report, context)
+        self.assertEqual(context, report)
+
+    def test_doesnt_add_non_celery_keys(self):
+        context = {
+            'celery': 'foo',
+            'bar': 'bar',
+            }
+        report = {}
+        attach_celery_info(report, context)
+        self.assertEqual({}, report)
 
 
 class TaskOopsReporterTests(TestCase):
@@ -27,11 +52,15 @@
     def get_oops_config(self):
         config = oops.Config()
         config.publishers.append(self.store_oops)
+        config.on_create.append(self.collect_context)
         return config
 
     def store_oops(self, oops):
         self.oopses.append(oops)
 
+    def collect_context(self, report, context):
+        self.context = context
+
     def test_on_failure_publishes_oops(self):
         einfo = self.get_einfo()
         self.recorder.on_failure()
@@ -41,19 +70,19 @@
         einfo = self.get_einfo()
         task_id = 'atask'
         self.recorder.on_failure(task_id=task_id)
-        self.assertEqual(task_id, self.oopses[0]['celery.task_id'])
+        self.assertEqual(task_id, self.context['celery.task_id'])
 
     def test_on_failure_includes_args(self):
         einfo = self.get_einfo()
         args = [8, 9]
         self.recorder.on_failure(args=args)
-        self.assertEqual(args, self.oopses[0]['celery.task_args'])
+        self.assertEqual(args, self.context['celery.args'])
 
     def test_on_failure_includes_kwargs(self):
         einfo = self.get_einfo()
         kwargs = dict(a=1, b=2)
         self.recorder.on_failure(kwargs=kwargs)
-        self.assertEqual(kwargs, self.oopses[0]['celery.task_kwargs'])
+        self.assertEqual(kwargs, self.context['celery.kwargs'])
 
     def test_on_failure_includes_tb_text(self):
         einfo = self.get_einfo()
@@ -73,19 +102,22 @@
     def test_on_failure_includes_sender(self):
         sender = object()
         self.recorder.on_failure(sender=sender)
-        self.assertEqual(sender, self.oopses[0]['celery.sender'])
-
-    def test_on_failure_accepts_signal_kwarg(self):
-        # Test there's no error if celery passes the signal kwarg
-        self.recorder.on_failure(signal=object())
-
-    def test_on_failure_accepts_exception_kwarg(self):
-        # Test there's no error if celery passes the exception kwarg
-        self.recorder.on_failure(exception=object())
-
-    def test_on_failure_accepts_traceback_kwarg(self):
-        # Test there's no error if celery passes the traceback kwarg
-        self.recorder.on_failure(traceback=object())
+        self.assertEqual(sender, self.context['celery.sender'])
+
+    def test_on_failure_includes_signal(self):
+        signal = object()
+        self.recorder.on_failure(signal=signal)
+        self.assertEqual(signal, self.context['celery.signal'])
+
+    def test_on_failure_includes_exception(self):
+        exception = object()
+        self.recorder.on_failure(exception=exception)
+        self.assertEqual(exception, self.context['celery.exception'])
+
+    def test_on_failure_includes_traceback(self):
+        traceback = object()
+        self.recorder.on_failure(traceback=traceback)
+        self.assertEqual(traceback, self.context['celery.traceback'])
 
 
 class SetupOopsReporterTests(TestCase):
@@ -119,6 +151,11 @@
         receiver = signals.task_failure.receivers[0][1]
         self.assertEqual(config, receiver.im_self.config)
 
+    def test_inserts_hooks(self):
+        config = oops.Config()
+        self.setup_oops_reporter(config=config)
+        self.assertIn(attach_celery_info, config.on_create)
+
     def test_generates_oops(self):
         config = oops.Config()
         oopses = []