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

Make it easy to get a logger from a RunTracker instance. #6771

Merged
merged 4 commits into from Nov 16, 2018

Conversation

Projects
None yet
3 participants
@benjyw
Contributor

benjyw commented Nov 14, 2018

This allows anything with access to subsystems to get a logger,
with RunTracker.global_instance().logger.

Eventually we can leverage this to stop having to plumb loggers
in via the task context all over the place (or not logging
because we can't be bothered to do the plumbing). Logging is a
crosscutting concern, so it makes total sense for a logger to
be available in this manner.

@stuhood

Thanks.

Borja will be starting #6004 soon and I'd appreciate any feedback you have there. But this change should be orthogonal.

@benjyw

This comment has been minimized.

Contributor

benjyw commented Nov 15, 2018

Cool - this did make me wonder how logging is/will/should work in the new engine.

@jsirois

The middleman comment is not a blocker.

@@ -47,6 +48,9 @@ def set_outcome(self, outcome):
class DummyRunTracker(object):
"""A runtracker stand-in that does no actual tracking."""
def __init__(self):

This comment has been minimized.

@jsirois

jsirois Nov 15, 2018

Member

Needs a blank line above to separate from doc string.

@@ -124,6 +124,9 @@ def __init__(self, *args, **kwargs):
# Note that multiple threads may share a name (e.g., all the threads in a pool).
self._threadlocal = threading.local()
# A logger facade that logs into this RunTracker.
self._logger = RunTrackerLogger(self)

This comment has been minimized.

@jsirois

jsirois Nov 15, 2018

Member

This has the RunTracker acting as middle-man so that folks who just need a logger now need to instead get a runtracker and pull on its logger. It seems a bit better to me to:

  1. short term: prefer those folks construct a RunTrackerLogger for which they depend on a RunTracker subsystem
  2. cleanup: have those folks depend on a Logger subsystem directly since it's what they actually need. That thin susbsystem would depend on a RunTracker (for now).

This comment has been minimized.

@benjyw

benjyw Nov 15, 2018

Contributor

We could do 2. straight away, I think. All uses of this logger today are via Context.log. But before I mess with that any further I'd like to understand how v2 tasks handle logging.

benjyw added some commits Nov 14, 2018

Make it easy to get a logger from a RunTracker instance.
This allows anything with access to subsystems to get a logger,
with RunTracker.global_instance().logger.

Eventually we can leverage this to stop having to plumb loggers
in via the task context all over the place (or not logging
because we can't be bothered to do the plumbing). Logging is a
crosscutting concern, so it makes total sense for a logger to
be available in this manner.

@benjyw benjyw force-pushed the benjyw:run_tracker_logger branch from bf50c9f to 726206a Nov 16, 2018

@benjyw benjyw merged commit 3756af9 into pantsbuild:master Nov 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:run_tracker_logger branch Nov 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment