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
Re-instore web logging into http.log #1327
Conversation
... on my way to update the doc ... |
👎I think this way of doing was a technical debt in eight. This prevents those logs to be exported to splunk or logstash like the other logs. |
the test broke anyway, not enough mocking there ...
|
Travis should be happy now ... |
What I do with logstash and splunk is that I directly send the log data in json form from the python logging system to the log server. without going to files. something similar to: The twistd.log logging is supposed to be configured in buildbot.tac. I think what you can do with buildbot.tac is not documented enough. You can really do neat stuff. For docker/heroku/paas, this is the same thing, you can configure this in the buildbot.tac: Forcing to go through files is the good old way. That's why splunk is so good at it. It works, but this is uneeded I/O for me. |
👎 for documenting the buildbot.tac and having to support more of those kind of crazy setups ! Our goal is to make buildbot accessible, not to constrain ourselves to some borderline usages. At the moment, the twisted.log file is literally polluted by the http logs. This happened in eight, there was a discussion about it, a bug was filled (http://trac.buildbot.net/ticket/684), and the consensus was to provide a http.log file. I'm not sure we should have that discussion again. And I'm pretty sure one of your buildbot.tac trick will allow you to continue do the same while we will help more basic users dig through their logs ... |
I agree that there should be an easy way, and a more configurable way. I'm just asking that this http.log is more configurable. best log practices changed since 5 years ago. So yes this is something that for me should be discussed. my proposal:
if logfileName is set to None, then the class Site() is used instead of RotateLogSite, and I'm happy. |
Ok, fine. Let's do it that way. |
Let's review that one again ! As a side note, this should be fixed now that we have configurations (not new in this PR) to allow reconfig to work. |
return server.Site._openLogFile(self, path) | ||
|
||
httplog = None | ||
if new_config.www['logfileName']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The coding style for such tests is usually
if new_config.www['logfileName'] is not None:
your condition also catches == "" which is not what we want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we wanted to catch '' as False ! What would it means to set a logfileName to '' otherwise ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean I intentionnaly not checked for None, but leaved it open.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we need to document it.
My experience is that '' as False/None is confusing for people so I prefer treat it as error (which will appear indirectly when passing '' to RotateLogSite, when it tries to create the log over the directory filename)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for documenting '' as False. (BTW, I had completely forgot to add your entry in the doc ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
The tests are failing, and I think previous had some pep8 issues |
Re-instore web logging into http.log Fixes ticket:3006
This is also TRAC-3006.
I shamelessly stole the logging class from eight.