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

Add task group to metrics #399

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions tasks/base.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import logging
from datetime import datetime
from typing import Optional

from celery.exceptions import SoftTimeLimitExceeded
from celery.worker.request import Request
Expand Down Expand Up @@ -35,16 +36,22 @@
)


def _get_task_group_from_name(task_name: Optional[str]) -> Optional[str]:
if task_name is None or ("." not in task_name):
return None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] do we want this to return None or "unknown_group"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I thought "unknown_group" but after checking the existing dashboards for the tasks I noticed that it's possible to have the task_name there are None... so just being consistent with returning None as well

# We checked that '.' is part of task_name
# So there will be at least 2 items in the array
return task_name.split(".")[-2]


class BaseCodecovRequest(Request):
@property
def metrics_prefix(self):
return f"worker.task.{self.name}"

def on_timeout(self, soft: bool, timeout: int):
res = super().on_timeout(soft, timeout)
task_group = (
self.name.split(".")[-2] if self.name is not None else "unknown_group"
)
task_group = _get_task_group_from_name(self.name)
if not soft:
REQUEST_HARD_TIMEOUT_COUNTER.labels(
task=self.name, task_group=task_group
Expand Down Expand Up @@ -126,7 +133,7 @@ class BaseCodecovTask(celery_app.Task):
def __init_subclass__(cls, name="unknown_task"):
cls.name = name
# All task names follow the format `app.[cron|task].<task_group>.<task>`
task_group = name.split(".")[-2] if name != "unknown_task" else "unknown_group"
task_group = _get_task_group_from_name(name)
cls.task_group = task_group

cls.metrics_prefix = f"worker.task.{name}"
Expand Down
18 changes: 16 additions & 2 deletions tasks/tests/unit/test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,27 @@
)

from database.tests.factories.core import OwnerFactory, RepositoryFactory
from tasks.base import BaseCodecovRequest, BaseCodecovTask
from tasks.base import BaseCodecovRequest, BaseCodecovTask, _get_task_group_from_name
from tasks.base import celery_app as base_celery_app

here = Path(__file__)


@pytest.mark.parametrize(
"task_name,output",
[
(None, None),
("task_not_following_expected_pattern", None),
("task_group.task_name", "task_group"),
("app.task.task_group.task_name", "task_group"),
("app.cron.daily.task_name", "daily"),
(f"app.tasks.test_results.TestResultsProcessor", "test_results"),
],
)
def test__get_task_group_from_name(task_name, output):
assert _get_task_group_from_name(task_name) == output


class MockDateTime(datetime):
"""
`@pytest.mark.freeze_time()` is convenient but will freeze time for
Expand Down Expand Up @@ -422,7 +437,6 @@ def test_sample_task_retry(self, celery_app, mocker):


class TestBaseCodecovRequest(object):

"""
All in all, this is a really weird class

Expand Down
Loading