Skip to content

Conversation

@khvn26
Copy link
Member

@khvn26 khvn26 commented Mar 27, 2025

This PR adds the following Task Processor metrics:

  • task_processor_enqueued_tasks_total: Counter labeled with task_identifier. Represents all tasks created via .delay(). Exposed both in API and Task Processor mode.
  • task_processor_finished_tasks_total: Counter labeled with task_identifier and result ("success", "failure"). Exposed only in Task Processor mode.
  • task_processor_task_duration_seconds: Histogram labeled with task_identifier and result ("success", "failure"). Exposed only in Task Processor mode.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds Prometheus metrics to track task processing in both API and Task Processor modes. It includes new metrics definitions, instrumentation in task execution and task enqueuing, test updates for metric assertions, and README documentation updates.

  • Added new counters and histogram in src/task_processor/metrics.py.
  • Instrumented task processing in src/task_processor/processor.py and task enqueuing in src/task_processor/decorators.py.
  • Updated unit tests and documentation to validate the new metrics.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/unit/task_processor/test_unit_task_processor_processor.py Added a test to verify finished task metrics and task duration metrics.
tests/unit/task_processor/test_unit_task_processor_decorators.py Added a test to verify enqueued task metrics on delay.
src/task_processor/processor.py Updated the task runner to record task duration and finished task metrics.
src/task_processor/metrics.py Added metric definitions for enqueued tasks, finished tasks, and task duration.
src/task_processor/decorators.py Updated logging and triggered enqueued task metrics.
settings/dev.py Enabled Task Processor mode.
README.md Documented the newly added Task Processor metrics.

@khvn26 khvn26 requested a review from Copilot March 27, 2025 14:39
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds Task Processor metrics to monitor enqueued tasks, finished tasks, and task durations, along with test coverage and documentation updates.

  • Added new tests to assert that the relevant metrics are recorded correctly.
  • Integrated metric collection points in the task processor and decorators.
  • Updated settings and documentation to reflect the new metrics.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/unit/task_processor/test_unit_task_processor_processor.py Added tests for task run metrics.
tests/unit/task_processor/test_unit_task_processor_decorators.py Added tests for metrics on task creation via decorators.
src/task_processor/processor.py Instrumented task execution with timing and finish metrics.
src/task_processor/metrics.py Introduced new Prometheus counters and a histogram for task metrics.
src/task_processor/decorators.py Updated to increment enqueued task metrics in the delay method.
src/common/test_tools/plugin.py Added registry reset logic for metric collectors.
settings/dev.py Enabled TASK_PROCESSOR_MODE for development.
README.md Documented the new Task Processor metrics.
Comments suppressed due to low confidence (1)

src/task_processor/processor.py:111

  • [nitpick] Consider simplifying the timer usage by directly invoking the timer as a context manager with labels, for example using a with-statement on metrics.task_processor_task_duration_seconds.labels(...).time(). This may simplify context management and reduce potential errors with timer closure.
ctx = ExitStack()

Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

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

These metrics look great. I'm interested to know how we seed the data.

If we start the task processor in an environment that already has a backlog of tasks, how does it handle that?

@khvn26 khvn26 force-pushed the feat/task-processor-metrics branch from 6584113 to 338d7f8 Compare March 27, 2025 17:10
@khvn26
Copy link
Member Author

khvn26 commented Mar 27, 2025

I'm interested to know how we seed the data.

I don't think we should:

  1. The counters are increment-only and not supposed to represent the shared state.
  2. They are going to be used to calculate rates and compared against each other rather for getting the absolute values.

Let me know if you have a stronger opinion, @matthewelwell. CC @rolodato in case you want to chime in here.

@khvn26 khvn26 requested a review from matthewelwell March 27, 2025 17:25
@khvn26 khvn26 force-pushed the feat/task-processor-metrics branch from 338d7f8 to 6648586 Compare March 27, 2025 17:36
@khvn26 khvn26 changed the base branch from feat/task-processor to main March 27, 2025 19:07
@khvn26 khvn26 requested a review from a team as a code owner March 27, 2025 19:07
@khvn26 khvn26 requested review from gagantrivedi and removed request for a team March 27, 2025 19:07
@khvn26 khvn26 force-pushed the feat/task-processor-metrics branch 4 times, most recently from b7713cf to 2c76e44 Compare April 1, 2025 09:36
Copy link
Contributor

@rolodato rolodato left a comment

Choose a reason for hiding this comment

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

LGTM, left some suggestions but no blockers 👍

khvn26 and others added 8 commits April 3, 2025 15:54
- consistent usage of `settings.TASK_PROCESSOR_MODE`
- test for failure running tasks when `TASK_PROCESSOR_MODE` is `False`
- add `task_processor_mode` pytest marker
- consistent usage of `django_db` marker
@khvn26 khvn26 force-pushed the feat/task-processor-metrics branch from 2c76e44 to 80943c0 Compare April 3, 2025 14:55
@khvn26 khvn26 merged commit d0ea561 into main Apr 4, 2025
4 checks passed
This was referenced Jul 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants