Add TaskLogger dependency
#25
Conversation
src/docket/dependencies.py
Outdated
| "task.key": execution.key, | ||
| "task.attempt": execution.attempt, |
There was a problem hiding this comment.
task.*? execution.*? task.execution.*?
There was a problem hiding this comment.
In the OTEL stuff I used execution.key, and execution.attempt because I thought the task could refer to the function itself and the execution could refer to this one instance of running the function
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #25 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 1160 1182 +22
Branches 51 51
=========================================
+ Hits 1160 1182 +22 ☔ View full report in Codecov by Sentry. |
src/docket/dependencies.py
Outdated
| def __call__( | ||
| self, docket: Docket, worker: Worker, execution: Execution | ||
| ) -> logging.LoggerAdapter: | ||
| logger = logging.getLogger(f"docket.task.{execution.key}") |
There was a problem hiding this comment.
We could make this configurable but I kind of feel like we always want the logger to be the task identifier?
There was a problem hiding this comment.
I feel like this should be "docket.task.{execution.function.__name__}"? The key is unique to each time a task runs, so if we made that the logger name, it might be pretty hard to filter it in logging tools? These also form like a hierarchical tree inside of the logging module so it might get bogged down if there are tons of unique ones (not totally sure about that)
chrisguidry
left a comment
chrisguidry
left a comment
There was a problem hiding this comment.
Love it! I feel like we should do something really sparkly with logging in Docket but I don't know what it is yet :D #23 is aspirational
src/docket/dependencies.py
Outdated
| def __call__( | ||
| self, docket: Docket, worker: Worker, execution: Execution | ||
| ) -> logging.LoggerAdapter: | ||
| logger = logging.getLogger(f"docket.task.{execution.key}") |
There was a problem hiding this comment.
I feel like this should be "docket.task.{execution.function.__name__}"? The key is unique to each time a task runs, so if we made that the logger name, it might be pretty hard to filter it in logging tools? These also form like a hierarchical tree inside of the logging module so it might get bogged down if there are tons of unique ones (not totally sure about that)
src/docket/dependencies.py
Outdated
| "task.key": execution.key, | ||
| "task.attempt": execution.attempt, |
There was a problem hiding this comment.
In the OTEL stuff I used execution.key, and execution.attempt because I thought the task could refer to the function itself and the execution could refer to this one instance of running the function
tests/test_fundamentals.py
Outdated
| assert called | ||
| assert "Task is running" in caplog.text | ||
| assert "docket.task.my-cool-task:123" in caplog.text | ||
| print(caplog.text) |
Closes #9
This PR adds a new dependency
TaskLogger. I'm not exactly sure if this is what was intended so happy to make adjustments!