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.

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

Make reporting more metrics for tasks possible #3343

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make reporting more metrics for tasks possible #3343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions luigi/metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@
def handle_task_done(self, task):
pass

def handle_task_statistics(self, task, statistics):
pass

Check warning on line 70 in luigi/metrics.py

View check run for this annotation

Codecov / codecov/patch

luigi/metrics.py#L70 

Added line #L70 was not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a common implementation that can be introduced here? Or within the Datadog and/or Prometheus implementations of MetricsCollector?

If not, and this method needs to remain as an optionally-implemented method, i would ask at a minimum for there to be appropriately informative and discoverable documentation written surrounding its intended use. (I don't believe there are currently any docs/ content around metrics collectors).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is entirely optional. I had not provided any documentation and tests yet, because I was hoping to gather some feedback in #3342 as you suggested in my earlier pull request (#3333 (comment)).

I will gladly add tests and documentation if you are convinced this change is small enough.


def generate_latest(self):
return

Expand Down
6 changes: 6 additions & 0 deletions luigi/scheduler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1646,3 +1646,9 @@
@rpc_method()
def update_metrics_task_started(self, task):
self._state._metrics_collector.handle_task_started(task)

@rpc_method()
def report_task_statistics(self, task_id, statistics):
if self._state.has_task(task_id):
task = self._state.get_task(task_id)
self._state._metrics_collector.handle_task_statistics(task, statistics)

Check warning on line 1654 in luigi/scheduler.py

View check run for this annotation

Codecov / codecov/patch

luigi/scheduler.py#L1652-L1654 

Added lines #L1652 - L1654 were not covered by tests
3 changes: 3 additions & 0 deletions luigi/worker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,9 @@
def decrease_running_resources(self, decrease_resources):
self._scheduler.decrease_running_task_resources(self._task_id, decrease_resources)

def report_task_statistics(self, statistics):
self._scheduler.report_task_statistics(self._task_id, statistics)

Check warning on line 343 in luigi/worker.py

View check run for this annotation

Codecov / codecov/patch

luigi/worker.py#L343 

Added line #L343 was not covered by tests


class SchedulerMessage:
"""
Expand Down
Loading