Skip to content

TaskAdmin+fields(created_at,updated_at), ideomatic logger, TaskManager TaskManager __str__ try #118

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.

Sign up for GitHub

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

TaskAdmin+fields(created_at,updated_at), ideomatic logger, TaskManager TaskManager __str__ try #118

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
30261b0
TaskAdmin +readonly_fields "created_at", "updated_at"; fix setup name
ikvk Jan 11, 2022
8831f1d
TaskAdmin +readonly_fields "created_at", "updated_at"; fix setup name…
ikvk Feb 6, 2023
d402b4d
TaskAdmin +readonly_fields "created_at", "updated_at"; fix setup name…
ikvk Feb 6, 2023
0e7a745
del /.idea/
ikvk Apr 26, 2024
01fb783
flake
ikvk Apr 26, 2024
69c482b
logging to idiomatic usage; add logger.exception - Failed to display …
ikvk Apr 27, 2024
e56726c
Update django_dramatiq/admin.py
ikvk Dec 28, 2024
2a01a5b
Merge branch 'master' into minor-improves
ikvk Aug 21, 2025
4ec1563
name="django_dramatiq" back (will make issue)
ikvk Aug 21, 2025
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
1 change: 1 addition & 0 deletions CONTRIBUTORS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@ This file lists the contributors to the `django_dramatiq` project.
| [@OrazioPirataDelloSpazio](https://github.com/OrazioPirataDelloSpazio) | Lorenzo |
| [@timdrijvers](https://github.com/timdrijvers) | Tim Drijvers |
| [@magraeber](https://github.com/magraeber) | magraeber |
| [@ikvk](https://github.com/ikvk) | Vladimir Kaukin |
18 changes: 4 additions & 14 deletions django_dramatiq/admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,14 @@
@admin.register(Task)
class TaskAdmin(admin.ModelAdmin):
exclude = ("message_data",)
readonly_fields = ("message_details", "traceback", "status", "queue_name", "actor_name")
list_display = (
"__str__",
"status",
"eta",
"created_at",
"updated_at",
"queue_name",
"actor_name",
)
readonly_fields = ("message_details", "traceback", "status", "queue_name", "actor_name", "created_at", "updated_at")
list_display = ("__str__", "status", "eta", "created_at", "updated_at", "queue_name", "actor_name",)
list_filter = ("status", "created_at", "queue_name", "actor_name")
search_fields = ("actor_name",)

def eta(self, instance):
timestamp = (
instance.message.options.get("eta", instance.message.message_timestamp) / 1000
)

"""Estimated time of arrival"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

What would be the value of this docstring? It just deciphers a common-known abbreviation, which the function named after.

Copy link
Author

@ikvk ikvk Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

First time I took a few moments to decipher it, as I didn't meet her very often. I think it definitely won't hurt

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"""Estimated time of arrival"""

I would agree with @amureki on this. Lets remove it before merging.

Copy link
Author

Choose a reason for hiding this comment

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

Are you serious? How can the documentation line interfere?

Copy link
Author

Choose a reason for hiding this comment

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

I conducted a survey among 10 of my colleagues: "I know exactly what the abbreviation ETA stands for." Most of them are engineers. The result: 40% know for sure.

I didn't think I'd have to defend the docstring either.

Copy link
Author

Choose a reason for hiding this comment

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

One more argument:
Explicit is better than implicit.
:D

By the way, is "Estimated time of arrival" the correct decoding of ETA?

timestamp = (instance.message.options.get("eta", instance.message.message_timestamp) / 1000)
# Django expects a timezone-aware datetime if USE_TZ is True, and a naive datetime in localtime otherwise.
tz = timezone.utc if settings.USE_TZ else None
return datetime.fromtimestamp(timestamp, tz=tz)
Expand Down
8 changes: 4 additions & 4 deletions django_dramatiq/middleware.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from django import db
from dramatiq.middleware import Middleware

LOGGER = logging.getLogger("django_dramatiq.AdminMiddleware")
logger = logging.getLogger(__name__)
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue: Any reason for this change? ie. this is potentially a breaking change for people relying on the namespacing of this logger.

Copy link
Author

Choose a reason for hiding this comment

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

Logger naming for this place is not ideomatic, there is no reasons to do this like here.
https://docs.python.org/3/library/logging.html

Anyway the log path will be the same here.

A potential problem may look like this: they changed the name of the class and forgot to change it in the logger.

This approach would be logical if you specified a single name for the entire package, for example: "django_dramatiq_all_pain" however, it is obvious from this place that there was no such goal.



class AdminMiddleware(Middleware):
Expand All @@ -13,7 +13,7 @@ class AdminMiddleware(Middleware):
def after_enqueue(self, broker, message, delay):
from .models import Task

LOGGER.debug("Creating Task from message %r.", message.message_id)
logger.debug("Creating Task from message %r.", message.message_id)
status = Task.STATUS_ENQUEUED
if delay:
status = Task.STATUS_DELAYED
Expand All @@ -28,7 +28,7 @@ def after_enqueue(self, broker, message, delay):
def before_process_message(self, broker, message):
from .models import Task

LOGGER.debug("Updating Task from message %r.", message.message_id)
logger.debug("Updating Task from message %r.", message.message_id)
Task.tasks.create_or_update_from_message(
message,
status=Task.STATUS_RUNNING,
Expand All @@ -49,7 +49,7 @@ def after_process_message(self, broker, message, *, result=None, exception=None,
elif status is None:
status = Task.STATUS_DONE

LOGGER.debug("Updating Task from message %r.", message.message_id)
logger.debug("Updating Task from message %r.", message.message_id)
Task.tasks.create_or_update_from_message(
message,
status=status,
Expand Down
9 changes: 8 additions & 1 deletion django_dramatiq/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import logging
from datetime import timedelta

from django.db import models
Expand All @@ -10,6 +11,8 @@
#: The database label to use when storing task metadata.
DATABASE_LABEL = DjangoDramatiqConfig.tasks_database()

logger = logging.getLogger(__name__)
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue: Same issue as above. 😉

Copy link
Author

Choose a reason for hiding this comment

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

*above



class TaskManager(models.Manager):
def create_or_update_from_message(self, message, **extra_fields):
Expand Down Expand Up @@ -63,4 +66,8 @@ def message(self):
return Message.decode(bytes(self.message_data))

def __str__(self):
return str(self.message)
try:
return str(self.message)
except Exception as e:
logger.exception(f'Failed to display Task {self.id}')
return f'Failed to display Task: {e}'
Comment on lines +69 to +73
Copy link
Collaborator

Choose a reason for hiding this comment

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

Under what condition would this raise?

Copy link
Author

Choose a reason for hiding this comment

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

As I told above

The key feature here is that the database cannot guarantee us the encodability of a BinaryField data.
Any byte error will result in 500 at admin web page, no matter how it got into the database.

And I finally found the trace!
I then changed some dramatic settings in the project, but I didn’t log which ones :D

2022-03-10T16:01:44.770354+05:00 job1.stage.int.<mywork>.ru django: Failed to decode message using encoder .
Traceback (most recent call last):
File "/home/int/.pyenv/versions/3.9.7/envs/int-3.9.7/lib/python3.9/site-packages/dramatiq/message.py", line 102, in decode
return cls(**global_encoder.decode(data))
_pickle.UnpicklingError: invalid load key, '_'.

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
File "/home/int/.pyenv/versions/3.9.7/envs/int-3.9.7/lib/python3.9/site-packages/dramatiq/brokers/rabbitmq.py", line 511, in __next__
message = Message.decode(body)
File "/home/int/.pyenv/versions/3.9.7/envs/int-3.9.7/lib/python3.9/site-packages/dramatiq/message.py", line 104, in decode
raise DecodeError("Failed to decode message.", data, e) from e
dramatiq.errors.DecodeError: Failed to decode message.

2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def rel(*xs):


setup(
name="django_dramatiq",
name="django-dramatiq",
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue: Any reason to change the name of the project?? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Yes,
Let github.com to show "Used by" block at main repo page (setup.name, on my own experience):
For my lib it works, I found this decision somewhere at web.
ikvk/imap_tools@f1b2614#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7R16

Here is rules:
https://packaging.python.org/en/latest/specifications/name-normalization/

This is not killer fature, but useful.

If I saw such a MR for my library, I would also double-check everything 100 times.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let github.com to show "Used by" block at main repo page

I dont think this is the case, and its most likely due to the number of public repo dependents being less than 100..

Copy link
Author

Choose a reason for hiding this comment

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

Maybe so, I suggest you try, and if anything, you can roll back the changes in the minor release immediately after the release.
In my old screenshot, the number of uses was 500, there is a chance of success.
*The block is not always visible immediately after opening the repository page, sometimes you need to wait a few seconds and refresh the page.

version=version,
description="A Django app for Dramatiq.",
long_description="Visit https://github.com/Bogdanp/django_dramatiq for more information.",
Expand Down