From 79379b444c6d05c0c86c33700ec282f38c6443dd Mon Sep 17 00:00:00 2001 From: Erik Kalkoken Date: Wed, 9 Mar 2022 10:04:13 +0000 Subject: [PATCH] Improve task statistics --- .../task_statistics/counters.py | 40 +++++++ .../task_statistics/event_series.py | 72 ++--------- .../authentication/task_statistics/signals.py | 28 +++-- .../task_statistics/tests/test_counters.py | 51 ++++++++ .../tests/test_event_series.py | 113 ++---------------- .../task_statistics/tests/test_signals.py | 58 ++++----- .../admin-status/celery_bar_partial.html | 3 +- .../allianceauth/admin-status/overview.html | 11 +- allianceauth/templatetags/admin_status.py | 2 +- 9 files changed, 169 insertions(+), 209 deletions(-) create mode 100644 allianceauth/authentication/task_statistics/counters.py create mode 100644 allianceauth/authentication/task_statistics/tests/test_counters.py diff --git a/allianceauth/authentication/task_statistics/counters.py b/allianceauth/authentication/task_statistics/counters.py new file mode 100644 index 00000000..bd9eaabd --- /dev/null +++ b/allianceauth/authentication/task_statistics/counters.py @@ -0,0 +1,40 @@ +from collections import namedtuple +import datetime as dt + +from .event_series import EventSeries + + +"""Global series for counting task events.""" +succeeded_tasks = EventSeries("SUCCEEDED_TASKS") +retried_tasks = EventSeries("RETRIED_TASKS") +failed_tasks = EventSeries("FAILED_TASKS") + + +_TaskCounts = namedtuple( + "_TaskCounts", ["succeeded", "retried", "failed", "total", "earliest_task", "hours"] +) + + +def dashboard_results(hours: int) -> _TaskCounts: + """Counts of all task events within the given timeframe.""" + + def earliest_if_exists(events: EventSeries, earliest: dt.datetime) -> list: + my_earliest = events.first_event(earliest=earliest) + return [my_earliest] if my_earliest else [] + + earliest = dt.datetime.utcnow() - dt.timedelta(hours=hours) + earliest_events = list() + succeeded_count = succeeded_tasks.count(earliest=earliest) + earliest_events += earliest_if_exists(succeeded_tasks, earliest) + retried_count = retried_tasks.count(earliest=earliest) + earliest_events += earliest_if_exists(retried_tasks, earliest) + failed_count = failed_tasks.count(earliest=earliest) + earliest_events += earliest_if_exists(failed_tasks, earliest) + return _TaskCounts( + succeeded=succeeded_count, + retried=retried_count, + failed=failed_count, + total=succeeded_count + retried_count + failed_count, + earliest_task=min(earliest_events) if earliest_events else None, + hours=hours, + ) diff --git a/allianceauth/authentication/task_statistics/event_series.py b/allianceauth/authentication/task_statistics/event_series.py index 5799189c..f8591fbc 100644 --- a/allianceauth/authentication/task_statistics/event_series.py +++ b/allianceauth/authentication/task_statistics/event_series.py @@ -1,5 +1,4 @@ import datetime as dt -from collections import namedtuple from typing import Optional, List from redis import Redis @@ -7,68 +6,29 @@ from pytz import utc from django.core.cache import cache -_TaskCounts = namedtuple( - "_TaskCounts", ["succeeded", "retried", "failed", "total", "earliest_task", "hours"] -) - - -def dashboard_results(hours: int) -> _TaskCounts: - """Counts of all task events within the given timeframe.""" - def earliest_if_exists(events: EventSeries, earliest: dt.datetime) -> list: - my_earliest = events.first_event(earliest=earliest) - return [my_earliest] if my_earliest else [] - - earliest = dt.datetime.utcnow() - dt.timedelta(hours=hours) - earliest_events = list() - succeeded = SucceededTaskSeries() - succeeded_count = succeeded.count(earliest=earliest) - earliest_events += earliest_if_exists(succeeded, earliest) - retried = RetriedTaskSeries() - retried_count = retried.count(earliest=earliest) - earliest_events += earliest_if_exists(retried, earliest) - failed = FailedTaskSeries() - failed_count = failed.count(earliest=earliest) - earliest_events += earliest_if_exists(failed, earliest) - return _TaskCounts( - succeeded=succeeded_count, - retried=retried_count, - failed=failed_count, - total=succeeded_count + retried_count + failed_count, - earliest_task=min(earliest_events) if earliest_events else None, - hours=hours, - ) - class EventSeries: - """Base class for recording and analysing a series of events. + """API for recording and analysing a series of events.""" - This class must be inherited from and the child class must define KEY_ID. - """ + _ROOT_KEY = "ALLIANCEAUTH_EVENT_SERIES" - _ROOT_KEY = "ALLIANCEAUTH_TASK_SERIES" - - def __init__( - self, - redis: Redis = None, - ) -> None: - if type(self) == EventSeries: - raise TypeError("Can not instantiate base class.") - if not hasattr(self, "KEY_ID"): - raise ValueError("KEY_ID not defined") + def __init__(self, key_id: str, redis: Redis = None) -> None: self._redis = cache.get_master_client() if not redis else redis if not isinstance(self._redis, Redis): raise TypeError( "This class requires a Redis client, but none was provided " "and the default Django cache backend is not Redis either." ) + self._key_id = str(key_id) + self.clear() @property def _key_counter(self): - return f"{self._ROOT_KEY}_{self.KEY_ID}_COUNTER" + return f"{self._ROOT_KEY}_{self._key_id}_COUNTER" @property def _key_sorted_set(self): - return f"{self._ROOT_KEY}_{self.KEY_ID}_SORTED_SET" + return f"{self._ROOT_KEY}_{self._key_id}_SORTED_SET" def add(self, event_time: dt.datetime = None) -> None: """Add event. @@ -133,21 +93,3 @@ class EventSeries: @staticmethod def _cast_scores_to_dt(score) -> dt.datetime: return dt.datetime.fromtimestamp(float(score), tz=utc) - - -class SucceededTaskSeries(EventSeries): - """A task has succeeded.""" - - KEY_ID = "SUCCEEDED" - - -class RetriedTaskSeries(EventSeries): - """A task has been retried.""" - - KEY_ID = "RETRIED" - - -class FailedTaskSeries(EventSeries): - """A task has failed.""" - - KEY_ID = "FAILED" diff --git a/allianceauth/authentication/task_statistics/signals.py b/allianceauth/authentication/task_statistics/signals.py index 87dc4f5e..9c54520a 100644 --- a/allianceauth/authentication/task_statistics/signals.py +++ b/allianceauth/authentication/task_statistics/signals.py @@ -1,15 +1,21 @@ -from celery.signals import task_failure, task_retry, task_success, worker_ready +from celery.signals import ( + task_failure, + task_internal_error, + task_retry, + task_success, + worker_ready +) from django.conf import settings -from .event_series import FailedTaskSeries, RetriedTaskSeries, SucceededTaskSeries +from .counters import failed_tasks, retried_tasks, succeeded_tasks def reset_counters(): """Reset all counters for the celery status.""" - SucceededTaskSeries().clear() - FailedTaskSeries().clear() - RetriedTaskSeries().clear() + succeeded_tasks.clear() + failed_tasks.clear() + retried_tasks.clear() def is_enabled() -> bool: @@ -27,16 +33,22 @@ def reset_counters_when_celery_restarted(*args, **kwargs): @task_success.connect def record_task_succeeded(*args, **kwargs): if is_enabled(): - SucceededTaskSeries().add() + succeeded_tasks.add() @task_retry.connect def record_task_retried(*args, **kwargs): if is_enabled(): - RetriedTaskSeries().add() + retried_tasks.add() @task_failure.connect def record_task_failed(*args, **kwargs): if is_enabled(): - FailedTaskSeries().add() + failed_tasks.add() + + +@task_internal_error.connect +def record_task_internal_error(*args, **kwargs): + if is_enabled(): + failed_tasks.add() diff --git a/allianceauth/authentication/task_statistics/tests/test_counters.py b/allianceauth/authentication/task_statistics/tests/test_counters.py new file mode 100644 index 00000000..2625d49d --- /dev/null +++ b/allianceauth/authentication/task_statistics/tests/test_counters.py @@ -0,0 +1,51 @@ +import datetime as dt + +from django.test import TestCase +from django.utils.timezone import now + +from allianceauth.authentication.task_statistics.counters import ( + dashboard_results, + succeeded_tasks, + retried_tasks, + failed_tasks, +) + + +class TestDashboardResults(TestCase): + def test_should_return_counts_for_given_timeframe_only(self): + # given + earliest_task = now() - dt.timedelta(minutes=15) + succeeded_tasks.clear() + succeeded_tasks.add(now() - dt.timedelta(hours=1, seconds=1)) + succeeded_tasks.add(earliest_task) + succeeded_tasks.add() + succeeded_tasks.add() + retried_tasks.clear() + retried_tasks.add(now() - dt.timedelta(hours=1, seconds=1)) + retried_tasks.add(now() - dt.timedelta(seconds=30)) + retried_tasks.add() + failed_tasks.clear() + failed_tasks.add(now() - dt.timedelta(hours=1, seconds=1)) + failed_tasks.add() + # when + results = dashboard_results(hours=1) + # then + self.assertEqual(results.succeeded, 3) + self.assertEqual(results.retried, 2) + self.assertEqual(results.failed, 1) + self.assertEqual(results.total, 6) + self.assertEqual(results.earliest_task, earliest_task) + + def test_should_work_with_no_data(self): + # given + succeeded_tasks.clear() + retried_tasks.clear() + failed_tasks.clear() + # when + results = dashboard_results(hours=1) + # then + self.assertEqual(results.succeeded, 0) + self.assertEqual(results.retried, 0) + self.assertEqual(results.failed, 0) + self.assertEqual(results.total, 0) + self.assertIsNone(results.earliest_task) diff --git a/allianceauth/authentication/task_statistics/tests/test_event_series.py b/allianceauth/authentication/task_statistics/tests/test_event_series.py index 87dea9e6..079c4721 100644 --- a/allianceauth/authentication/task_statistics/tests/test_event_series.py +++ b/allianceauth/authentication/task_statistics/tests/test_event_series.py @@ -4,46 +4,13 @@ from pytz import utc from django.test import TestCase from django.utils.timezone import now -from allianceauth.authentication.task_statistics.event_series import ( - EventSeries, - FailedTaskSeries, - RetriedTaskSeries, - SucceededTaskSeries, - dashboard_results, -) +from allianceauth.authentication.task_statistics.event_series import EventSeries class TestEventSeries(TestCase): - """Testing EventSeries class.""" - - class IncompleteEvents(EventSeries): - """Child class without KEY ID""" - - class MyEventSeries(EventSeries): - KEY_ID = "TEST" - - def test_should_create_object(self): - # when - events = self.MyEventSeries() - # then - self.assertIsInstance(events, self.MyEventSeries) - - def test_should_abort_when_redis_client_invalid(self): - with self.assertRaises(TypeError): - self.MyEventSeries(redis="invalid") - - def test_should_not_allow_instantiation_of_base_class(self): - with self.assertRaises(TypeError): - EventSeries() - - def test_should_not_allow_creating_child_class_without_key_id(self): - with self.assertRaises(ValueError): - self.IncompleteEvents() - def test_should_add_event(self): # given - events = self.MyEventSeries() - events.clear() + events = EventSeries("dummy") # when events.add() # then @@ -53,8 +20,7 @@ class TestEventSeries(TestCase): def test_should_add_event_with_specified_time(self): # given - events = self.MyEventSeries() - events.clear() + events = EventSeries("dummy") my_time = dt.datetime(2021, 11, 1, 12, 15, tzinfo=utc) # when events.add(my_time) @@ -65,8 +31,7 @@ class TestEventSeries(TestCase): def test_should_count_events(self): # given - events = self.MyEventSeries() - events.clear() + events = EventSeries("dummy") events.add() events.add() # when @@ -76,8 +41,7 @@ class TestEventSeries(TestCase): def test_should_count_zero(self): # given - events = self.MyEventSeries() - events.clear() + events = EventSeries("dummy") # when result = events.count() # then @@ -85,8 +49,7 @@ class TestEventSeries(TestCase): def test_should_count_events_within_timeframe_1(self): # given - events = self.MyEventSeries() - events.clear() + events = EventSeries("dummy") events.add(dt.datetime(2021, 12, 1, 12, 0, tzinfo=utc)) events.add(dt.datetime(2021, 12, 1, 12, 10, tzinfo=utc)) events.add(dt.datetime(2021, 12, 1, 12, 15, tzinfo=utc)) @@ -101,8 +64,7 @@ class TestEventSeries(TestCase): def test_should_count_events_within_timeframe_2(self): # given - events = self.MyEventSeries() - events.clear() + events = EventSeries("dummy") events.add(dt.datetime(2021, 12, 1, 12, 0, tzinfo=utc)) events.add(dt.datetime(2021, 12, 1, 12, 10, tzinfo=utc)) events.add(dt.datetime(2021, 12, 1, 12, 15, tzinfo=utc)) @@ -114,8 +76,7 @@ class TestEventSeries(TestCase): def test_should_count_events_within_timeframe_3(self): # given - events = self.MyEventSeries() - events.clear() + events = EventSeries("dummy") events.add(dt.datetime(2021, 12, 1, 12, 0, tzinfo=utc)) events.add(dt.datetime(2021, 12, 1, 12, 10, tzinfo=utc)) events.add(dt.datetime(2021, 12, 1, 12, 15, tzinfo=utc)) @@ -127,8 +88,7 @@ class TestEventSeries(TestCase): def test_should_clear_events(self): # given - events = self.MyEventSeries() - events.clear() + events = EventSeries("dummy") events.add() events.add() # when @@ -138,8 +98,7 @@ class TestEventSeries(TestCase): def test_should_return_date_of_first_event(self): # given - events = self.MyEventSeries() - events.clear() + events = EventSeries("dummy") events.add(dt.datetime(2021, 12, 1, 12, 0, tzinfo=utc)) events.add(dt.datetime(2021, 12, 1, 12, 10, tzinfo=utc)) events.add(dt.datetime(2021, 12, 1, 12, 15, tzinfo=utc)) @@ -151,8 +110,7 @@ class TestEventSeries(TestCase): def test_should_return_date_of_first_event_with_range(self): # given - events = self.MyEventSeries() - events.clear() + events = EventSeries("dummy") events.add(dt.datetime(2021, 12, 1, 12, 0, tzinfo=utc)) events.add(dt.datetime(2021, 12, 1, 12, 10, tzinfo=utc)) events.add(dt.datetime(2021, 12, 1, 12, 15, tzinfo=utc)) @@ -166,57 +124,10 @@ class TestEventSeries(TestCase): def test_should_return_all_events(self): # given - events = self.MyEventSeries() - events.clear() + events = EventSeries("dummy") events.add() events.add() # when results = events.all() # then self.assertEqual(len(results), 2) - - -class TestDashboardResults(TestCase): - def test_should_return_counts_for_given_timeframe_only(self): - # given - earliest_task = now() - dt.timedelta(minutes=15) - succeeded = SucceededTaskSeries() - succeeded.clear() - succeeded.add(now() - dt.timedelta(hours=1, seconds=1)) - succeeded.add(earliest_task) - succeeded.add() - succeeded.add() - retried = RetriedTaskSeries() - retried.clear() - retried.add(now() - dt.timedelta(hours=1, seconds=1)) - retried.add(now() - dt.timedelta(seconds=30)) - retried.add() - failed = FailedTaskSeries() - failed.clear() - failed.add(now() - dt.timedelta(hours=1, seconds=1)) - failed.add() - # when - results = dashboard_results(hours=1) - # then - self.assertEqual(results.succeeded, 3) - self.assertEqual(results.retried, 2) - self.assertEqual(results.failed, 1) - self.assertEqual(results.total, 6) - self.assertEqual(results.earliest_task, earliest_task) - - def test_should_work_with_no_data(self): - # given - succeeded = SucceededTaskSeries() - succeeded.clear() - retried = RetriedTaskSeries() - retried.clear() - failed = FailedTaskSeries() - failed.clear() - # when - results = dashboard_results(hours=1) - # then - self.assertEqual(results.succeeded, 0) - self.assertEqual(results.retried, 0) - self.assertEqual(results.failed, 0) - self.assertEqual(results.total, 0) - self.assertIsNone(results.earliest_task) diff --git a/allianceauth/authentication/task_statistics/tests/test_signals.py b/allianceauth/authentication/task_statistics/tests/test_signals.py index 7b3da396..aeeb8d60 100644 --- a/allianceauth/authentication/task_statistics/tests/test_signals.py +++ b/allianceauth/authentication/task_statistics/tests/test_signals.py @@ -4,10 +4,10 @@ from celery.exceptions import Retry from django.test import TestCase, override_settings -from allianceauth.authentication.task_statistics.event_series import ( - FailedTaskSeries, - RetriedTaskSeries, - SucceededTaskSeries, +from allianceauth.authentication.task_statistics.counters import ( + failed_tasks, + retried_tasks, + succeeded_tasks, ) from allianceauth.authentication.task_statistics.signals import ( reset_counters, @@ -17,15 +17,16 @@ from allianceauth.eveonline.tasks import update_character @override_settings( - CELERY_ALWAYS_EAGER=True, ALLIANCEAUTH_DASHBOARD_TASK_STATISTICS_DISABLED=False + CELERY_ALWAYS_EAGER=True,ALLIANCEAUTH_DASHBOARD_TASK_STATISTICS_DISABLED=False ) class TestTaskSignals(TestCase): fixtures = ["disable_analytics"] def test_should_record_successful_task(self): # given - events = SucceededTaskSeries() - events.clear() + succeeded_tasks.clear() + retried_tasks.clear() + failed_tasks.clear() # when with patch( "allianceauth.eveonline.tasks.EveCharacter.objects.update_character" @@ -33,12 +34,15 @@ class TestTaskSignals(TestCase): mock_update.return_value = None update_character.delay(1) # then - self.assertEqual(events.count(), 1) + self.assertEqual(succeeded_tasks.count(), 1) + self.assertEqual(retried_tasks.count(), 0) + self.assertEqual(failed_tasks.count(), 0) def test_should_record_retried_task(self): # given - events = RetriedTaskSeries() - events.clear() + succeeded_tasks.clear() + retried_tasks.clear() + failed_tasks.clear() # when with patch( "allianceauth.eveonline.tasks.EveCharacter.objects.update_character" @@ -46,12 +50,15 @@ class TestTaskSignals(TestCase): mock_update.side_effect = Retry update_character.delay(1) # then - self.assertEqual(events.count(), 1) + self.assertEqual(succeeded_tasks.count(), 0) + self.assertEqual(failed_tasks.count(), 0) + self.assertEqual(retried_tasks.count(), 1) def test_should_record_failed_task(self): # given - events = FailedTaskSeries() - events.clear() + succeeded_tasks.clear() + retried_tasks.clear() + failed_tasks.clear() # when with patch( "allianceauth.eveonline.tasks.EveCharacter.objects.update_character" @@ -59,28 +66,21 @@ class TestTaskSignals(TestCase): mock_update.side_effect = RuntimeError update_character.delay(1) # then - self.assertEqual(events.count(), 1) + self.assertEqual(succeeded_tasks.count(), 0) + self.assertEqual(retried_tasks.count(), 0) + self.assertEqual(failed_tasks.count(), 1) - -@override_settings(ALLIANCEAUTH_DASHBOARD_TASK_STATISTICS_DISABLED=False) -class TestResetCounters(TestCase): def test_should_reset_counters(self): # given - succeeded = SucceededTaskSeries() - succeeded.clear() - succeeded.add() - retried = RetriedTaskSeries() - retried.clear() - retried.add() - failed = FailedTaskSeries() - failed.clear() - failed.add() + succeeded_tasks.add() + retried_tasks.add() + failed_tasks.add() # when reset_counters() # then - self.assertEqual(succeeded.count(), 0) - self.assertEqual(retried.count(), 0) - self.assertEqual(failed.count(), 0) + self.assertEqual(succeeded_tasks.count(), 0) + self.assertEqual(retried_tasks.count(), 0) + self.assertEqual(failed_tasks.count(), 0) class TestIsEnabled(TestCase): diff --git a/allianceauth/templates/allianceauth/admin-status/celery_bar_partial.html b/allianceauth/templates/allianceauth/admin-status/celery_bar_partial.html index 135f5eb0..8939d8b3 100644 --- a/allianceauth/templates/allianceauth/admin-status/celery_bar_partial.html +++ b/allianceauth/templates/allianceauth/admin-status/celery_bar_partial.html @@ -6,7 +6,6 @@ aria-valuenow="{% widthratio tasks_count tasks_total 100 %}" aria-valuemin="0" aria-valuemax="100" - style="width: {% widthratio tasks_count tasks_total 100 %}%;" - title="{{ tasks_count|intcomma }} {{ label }}"> + style="width: {% widthratio tasks_count tasks_total 100 %}%;"> {% widthratio tasks_count tasks_total 100 %}% diff --git a/allianceauth/templates/allianceauth/admin-status/overview.html b/allianceauth/templates/allianceauth/admin-status/overview.html index 5a2392e8..8bf61183 100644 --- a/allianceauth/templates/allianceauth/admin-status/overview.html +++ b/allianceauth/templates/allianceauth/admin-status/overview.html @@ -78,10 +78,15 @@

{% translate "Task Queue" %}

- {% blocktranslate with total=tasks_total|intcomma latest=earliest_task|timesince|default_if_none:"?" %} - Status of {{ total }} processed tasks • last {{ latest }}

+ {% blocktranslate with total=tasks_total|intcomma latest=earliest_task|timesince|default:"?" %} + Status of {{ total }} processed tasks • last {{ latest }} {% endblocktranslate %} -
+

+
{% include "allianceauth/admin-status/celery_bar_partial.html" with label="suceeded" level="success" tasks_count=tasks_succeeded %} {% include "allianceauth/admin-status/celery_bar_partial.html" with label="retried" level="info" tasks_count=tasks_retried %} {% include "allianceauth/admin-status/celery_bar_partial.html" with label="failed" level="danger" tasks_count=tasks_failed %} diff --git a/allianceauth/templatetags/admin_status.py b/allianceauth/templatetags/admin_status.py index 1316a269..dd610630 100644 --- a/allianceauth/templatetags/admin_status.py +++ b/allianceauth/templatetags/admin_status.py @@ -13,7 +13,7 @@ from django.core.cache import cache from allianceauth import __version__ -from ..authentication.task_statistics.event_series import dashboard_results +from ..authentication.task_statistics.counters import dashboard_results register = template.Library()