Ticket #2447 (inTicketWork new functionality)

Opened 6 years ago

Last modified 6 years ago

Enable logging via the event broker

Reported by: ktl Owned by: ktl
Priority: normal Milestone:
Component: pipe_base Keywords:
Cc: srp, daues, price, ktl, rhl, rowen Blocked By:
Blocking: Project: LSST
Version Number:
How to repeat:

not applicable

Description

Tasks can currently log via the default logger (a ScreenLog) to std::clog (which is equivelent to stderr) or to that and an additional file destination (via the --logdest argument).

We need to be able to configure tasks to log via the event broker, both to enable easy ingest of logs into the database for analysis and to reduce the load on the filesystem. A command line option (e.g. reuse of the --logdest argument) should enable this. In addition, an environment variable should also be able to enable this feature. (The variable would be set by the parallelization middleware.)

Change History

comment:1 Changed 6 years ago by DefaultCC Plugin

  • Cc price, ktl, rhl added

comment:2 Changed 6 years ago by ktl

  • Cc rowen added
  • Status changed from new to assigned

comment:3 Changed 6 years ago by ktl

  • Owner changed from ktl to srp
  • reviewstatus changed from notReady to needsReview

OK, I think this works about as well as it can. The tests were giving me a lot of angst. This is not very long, so I'm assigning to Steve, but hopefully Russell can take a quick look as well to make sure I didn't do too much violence to his code.

pipe_base $ git diff -w --stat master...tickets/2447
 python/lsst/pipe/base/argumentParser.py |   76 ++++++++++---
 python/lsst/pipe/base/cmdLineTask.py    |    1 +
 tests/testLogDest.py                    |  178 +++++++++++++++++++++++++++++++
 3 files changed, 238 insertions(+), 17 deletions(-)

comment:4 follow-up: ↓ 8 Changed 6 years ago by srp

  • Owner changed from srp to ktl
  • reviewstatus changed from needsReview to reviewed

The only comment I have is the event logging tests are completely skipped unless the ctrl_events package is set up, so that set of tests are not exercised on regular builds. Once the package is setup, I was able to verify that the test ran, and watched with another utility that six messages were set to the logging daemon, as advertised. I'm not sure if this was intentional or not.

comment:5 Changed 6 years ago by price

Why does this live in pipe_base, instead of pex_logging? Surely pipe_base should just be requesting some URL for the logger, which pex_logging should deliver?

comment:6 follow-up: ↓ 9 Changed 6 years ago by rowen

I agree with Paul. The URL parsing code is very specialized and I'd rather see it somewhere else -- pex_logging or ctrl_events.

pipe_base is supposed to be a very low-level package and I am not happy to have it depend on ctrl_events. On the other hand, I don't see how to avoid that -- if we have pex_logging handle URLs then pex_logging needs to depend on ctrl_events. Still...this is a lot of specialized code that I'd rather see elsewhere.

Picky point: this seems clumsy:

if namespace.logdest is None and "LSST_LOGDEST" in os.environ:
    namespace.logdest = os.environ["LSST_LOGDEST"]

compared to the following, which has less stutter and is more idiomatic:

if namespace.logdest is None:
    namespace.logdest = os.environ.get("LSST_LOGDEST", None)

comment:7 Changed 6 years ago by price

I didn't think there was a dependence on ctrl_events, as the eups table file hadn't been updated, but I'm wrong: there is an explicit import of lsst.ctrl.events, unprotected by a try block.

Perhaps we need a mechanism for registering log URL parsers?

comment:8 in reply to: ↑ 4 Changed 6 years ago by ktl

Replying to srp:

The only comment I have is the event logging tests are completely skipped unless the ctrl_events package is set up, so that set of tests are not exercised on regular builds.

It's intentional that ctrl_events is not required by pipe_base. Given that, the test that uses the package must be skipped if the package is not available. (Setting up the package for the purpose of the test would be user-hostile.)

watched with another utility that six messages were set to the logging daemon, as advertised.

The test is supposed to confirm this as well by receiving and checking the messages.

comment:9 in reply to: ↑ 6 Changed 6 years ago by ktl

Replying to price, rowen, and price:

I agree with Paul. The URL parsing code is very specialized and I'd rather see it somewhere else -- pex_logging or ctrl_events.

The URL parsing would have to live in ctrl_events, but the registry for the URL methods would have to live in pex_logging. This would necessitate changes to three packages instead of one. In addition, I'd like to replace pex_logging with an alternative based on third-party code that would already have the equivalent registry, so writing one now would involve even more throwaway code.

pipe_base intentionally does not depend on ctrl_events. The missing try block around the import was unintentional. I'll fix it when I come back if this code is not required in the short term to reduce filesystem load.

[get with default]

Sorry, I haven't yet gotten used to that idiom. I'll fix that too.

comment:10 Changed 6 years ago by ktl

  • Status changed from assigned to inTicketWork
  • reviewstatus changed from reviewed to notReady

Given that this doesn't seem to be time-critical, I'll take this back and do it the "right" way with the changes to ctrl_events and pex_logging, maybe or maybe not combined with the change to third-party code for the latter.

Note: See TracTickets for help on using tickets.