Ticket #1351 (closed enhancement: fixed)

Opened 5 years ago

Last modified 3 years ago

log observer bridging to python logging facility

Reported by: ctheune Owned by: therve
Priority: highest Milestone:
Component: core Keywords:
Cc: exarkun, ctheune, therve, dreid, itamarst, oubiwann Branch:
Author: Launchpad Bug:

Description


Attachments

logger.py Download (1.2 KB) - added by ctheune 5 years ago.

Change History

Changed 5 years ago by ctheune

This one isn't too fancy because it is for embedding into Zope 3. This you might
want to change: 

- use a different logger system for each message depending on the dict. 
- use a different logging level depending on whether the message is a failure or
not 

...

Changed 5 years ago by ctheune

Changed 5 years ago by exarkun

I'm not sure why this should be added to Twisted?  Python's logging system is
potentially blocking.  Using this could block an entire Twisted application any
time a message is logged.

Changed 5 years ago by ctheune

Hmm. Interesting. itamar wanted me to post that bridge because he liked the idea.
Obviously I totally forgot about blocking ...

Changed 4 years ago by therve

  • owner set to therve
  • component set to conch

Changed 4 years ago by therve

  • component changed from conch to core

Changed 4 years ago by therve

  • owner therve deleted
  • priority changed from normal to highest
  • keywords review added
  • cc therve added

Ready to review in logging-bridge-1351.

Changed 4 years ago by dreid

  • keywords review removed
  • owner set to dreid

Changed 4 years ago by dreid

  • owner changed from dreid to therve
  • _safeFormat doesn't have a docstring. Yes it's private, but it might be useful to implementors of other observers and the docstring should explain what it's for and how to use it.
  • LoggingObserver isn't a very descriptive class name, PythonLoggingObserver would probably be better.
  • There is an opportunity in emit to use the eventDict to support more logging levels than just INFO and ERROR. Perhaps a logLevel key in the eventDict could be used to set the log level, and use self.logger.log(level, msg) to send the message)

Good docstrings, and good test cases. I'm all for making twisted's logging more flexible and giving people the option of using the logging module is a great start.

Changed 4 years ago by therve

  • cc dreid added
  • owner therve deleted
  • keywords review added

Thanks for your review. Indeed adding log level seems fine. I changed the name to PythonLoggingObserver and add the docstring.

Changed 4 years ago by dreid

  • keywords review removed
  • owner set to therve

_safeFormat's second argument "crap" isn't very descriptive. perhaps fmtDict would be better?

It feels like textFromEventDict is a tool for authors of log observers, so maybe we should document the keys that it expects in eventDict.

Otherwise this looks good to merge.

Changed 4 years ago by therve

  • owner therve deleted
  • keywords review added

Thanks, I ended up writing too much English for pushing it further, so I put it back for review.

Changed 4 years ago by dreid

  • owner set to dreid

Changed 4 years ago by dreid

  • status changed from new to assigned

Changed 4 years ago by dreid

  • keywords review removed
  • status changed from assigned to new
  • owner changed from dreid to therve
Try to format the string C{fmtString} using C{fmtDict} arguments,
swallowing every errors to always return a string.

Should be all errors, or every error.

- C{isError}: boolean indication the nature of the event. Required if
    message is empty.

I think you mean indicating. Also isError isn't actually required if message is empty because format will be used if present.

Perhaps this should be moved to the docs for message with something along these lines.

- C{message}: by default, it holds the final text. It's required, but can
    be empty if either C{isError} or C{format} is provided.
- C{format}: string format used in place of C{message} to customize
    the event. It uses every other present keys of C{eventDict} to format
    the text.

Perhaps "It uses all keys present in C{eventDict} to format the text."? And the incompatibility between isError and format could be cleared up elsewhere?

Changed 4 years ago by therve

  • keywords review added
  • owner changed from therve to dreid

Thanks a lot for the corrections!

Changed 4 years ago by dreid

  • keywords review removed
  • owner changed from dreid to therve
+    There's a way we could make this if not safer at least more
+    informative: perhaps some sort of str/repr wrapper objects
+    could be wrapped around the things inside of C{fmtDict}. That way
+    if the event dict contains an object with a bad __repr__, we
+    can only cry about that individual object instead of the
+    entire event dict.

I'm not sure this is relevant to the docstring. It's discussing a possible implementation change instead of anything about it's usage. Other than that, this looks ready to merge. Keep up the good work.

Changed 4 years ago by therve

  • status changed from new to closed
  • resolution set to fixed

(In [19696]) Merge logging-bridge-1351

Author: therve, ctheune Reviewer: dreid Fixes #1351

Add a log observer bridging to python logging module. It allows better integration for people who already use logging but want to use twisted log methods. In the process some methods have been extracted from FileLogObserver to be easier to use by other observers.

Changed 4 years ago by therve

(In [19697]) Revert [19696], test regression with web2. Refs #1351.

Changed 4 years ago by therve

  • status changed from closed to reopened
  • resolution fixed deleted

Changed 4 years ago by therve

  • status changed from reopened to new

Changed 4 years ago by therve

  • owner therve deleted
  • keywords review added

There was a problem with textFromEventDict when it returns None. I think it should be an error as it fails silently. What about adding a deprecation warning here ?

Changed 4 years ago by itamarst

Documentation should be added somewhere to the log howto.

And, I hate to say this... do we want to consider twistd integration?

Changed 4 years ago by itamarst

  • cc itamarst added

Changed 4 years ago by exarkun

We do want to consider twistd integration, but that shouldn't be done in a way which is specific to this code, so I think it's beyond the scope of this ticket.

Do we have a ticket for general logging configuration yet?

Changed 4 years ago by therve

  • owner set to therve
  • keywords review removed

I write the documentation if you review it.

For twistd integration, it seems in scope of #638.

Changed 4 years ago by therve

  • keywords review added
  • owner therve deleted

I've done a basic introduction in the howto. That's my first lore document so please be indulgent :).

Also, I had no answer regarding textFromEventDict returning None: I really think that's a broken behaviour.

Changed 4 years ago by exarkun

  • keywords review removed
  • owner set to therve

The howto docs should include a warning about blocking behavior. Also, startLoggingWithObserver isn't really the preferred interface to the logging system. PythonLoggingObserver has those nice start and stop methods. Perhaps we should suggest that people use them? :) Oh, which brings up this point: PythonLoggingObserver.start and PythonLoggingObserver.stop have no test coverage. :)

I also expect PythonLoggingObserver should be new-style. I'm uncertain what the testing implementations of this statement are, though (should there be tests which fail if it is classic? should they rely on new-style behavior or should they use isinstance? etc)

Changed 4 years ago by therve

  • owner therve deleted
  • keywords review added

I've update the docs and the tests.

I actually tested the new-style behavior using super(). I don't know if it's so great, but that seems like a primary need to me. Of course I could do a stupid issubclass call instead.

Changed 3 years ago by glyph

  • keywords review removed
  • owner set to therve
  • PythonLoggingObserver's docstrings have some problems.
    • It should be more specific. "Python logging module" should be "the Python standard library L{logging} module", so that it's clear we are referring specifically to that one and not to an arbitrary logging module you might like to use. (I would suggest a name change too, except that would imply a ridiculous name, like PythonStandardLibraryLoggingModuleObserver or something like that, so never mind...)
    • "so be sure to not use this, or assume." - that seems to be ending in the middle of a sentence. It should also stress that code within Twisted, such as twisted.web, assumes that the logging module does not block, except perhaps on local disk access.
    • the epydoc formatting in 'emit' is messed up. The the line before the code sample should end with "::" and the prefixing the lines with "|" is no longer necessary; that recommendation should be deleted from the coding standard, sorry. It was based on an epydoc bug that does not exist in pydoctor; and probably not even in more recent epydoc versions. You should probably  grab pydoctor from the website yourself, so you can preview the output.
  • The docstring of textFromEventDict implies that eventDict may not have any other keys. It should be clarified that, in fact, it can have arbitrary keys other than the ones specified. This is what the 'format' string will be formatting, after all.

Optional - make a separate ticket or implement these:

  • The comment that dreid called out earlier as not relevant to the docstring, "There's a way we could make this if not safer...", could fairly easily be implemented by using twisted.python.reflect.safe_str. Given that this comment was there before, I'll leave it up to your discretion whether to fix it or file a new ticket.
  • It seems likely that users who want this functionality would also want it exposed via a twistd command-line option. I personally don't care about this functionality though, so it's hard to say for sure.

Changed 3 years ago by therve

  • owner therve deleted
  • keywords review added

I've done the docstrings cleanup. For the 2 other points:

  • That doesn't seem too obvious to use safe_str. For example, today you could use format with something like %0.4f for example, which means we can't convert all the eventDict elements to strings.
  • For twistd command line, this class is no less (or more :)) available to users than other log configuration, so that seems out of the scope of this. It seems something may be done in #638 (which is not still available for review but should be soon).

Thanks for pushing me to install pydoctor, I had been to lazy to do it previously :)

Changed 3 years ago by oubiwann

  • cc oubiwann added

Changed 3 years ago by drewp

A message from the link police:

The howto with a new section about usage of PythonLoggingObserver is here: http://twistedmatrix.com/trac/browser/branches/logging-bridge-1351/doc/core/howto/logging.xhtml

The new PythonLoggingObserver class is here: http://twistedmatrix.com/trac/browser/branches/logging-bridge-1351/twisted/python/log.py#L398

The code LGTM. I'm looking forward to using this with the tac configuration proposed on http://twistedmatrix.com/trac/ticket/638

Changed 3 years ago by exarkun

  • keywords review removed
  • owner set to therve

For the docstring of test_strangeEventDict, I'd suggest something along these lines instead:

Verify that an event dictionary which is not an error and has an empty message isn't recorded.

Keeping the note about the historic behavior may also be useful, but it shouldn't be the primary test description.

Why does PythonLoggingIntegrationTestCase use bare asserts instead of trial assert methods?

Changed 3 years ago by exarkun

Oops, one other thing. Running the full Twisted test suite with this branch merged to trunk eventually fails like this:

twisted.trial.test.test_output
  TestImportErrors
    test_badimport ...                                                     [OK]
Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/trunk/bin/trial", line 24, in ?
    run()
  File "/home/exarkun/Projects/Twisted/trunk/twisted/scripts/trial.py", line 345, in run
    test_result = trialRunner.run(suite)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 795, in run
    suite.run(result)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 285, in run
    TestSuite.run(self, result)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 149, in run
    test(result)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 138, in __call__
    return self.run(result)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 176, in run
    super(LoggedSuite, self).run(result)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 149, in run
    test(result)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 138, in __call__
    return self.run(result)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 149, in run
    test(result)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 138, in __call__
    return self.run(result)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 149, in run
    test(result)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 138, in __call__
    return self.run(result)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 149, in run
    test(result)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 138, in __call__
    return self.run(result)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 149, in run
    test(result)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/unittest.py", line 537, in __call__
    return self.run(*args, **kwargs)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/unittest.py", line 735, in run
    log.msg("--> %s <--" % (self.id()))
  File "/home/exarkun/Projects/Twisted/trunk/twisted/python/log.py", line 246, in msg
    o = self.observers.pop(i)
IndexError: pop index out of range

Changed 3 years ago by therve

  • keywords review added
  • owner changed from therve to exarkun

There was a stupid typo, and I did other corrections, thanks.

Changed 3 years ago by exarkun

  • owner changed from exarkun to therve
  • keywords review removed

The logging.xhtml text is still a bit rough. We can probably clean it up later (suggesting text in a review comment seems lame, and if I just check in changes then we'll have to find another reviewer), and it's certainly clear enough to convey the information it is trying to convey.

All the code looks good, and the test suite passes now. Looks good to merge, I'd say.

Changed 3 years ago by therve

  • status changed from new to closed
  • resolution set to fixed

Closed in [20100].

Note: See TracTickets for help on using tickets.