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.

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

Include datetime in httpok and memmon script logs #75

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

erral
Copy link

@erral erral commented Feb 12, 2016

Useful to know when the httpOk or Memmon scripts did something, because currently they only log what the did, not when they did it.

@erral
Copy link
Author

erral commented Feb 12, 2016

There is a setuptools error on the python 3.2 tests, that seems to be unrelated to superlance...

@erral
Copy link
Author

erral commented Feb 12, 2016

This addresses #71

@hvelarde
Copy link

@erral thanks for addressing this! wouldn't be better to test the presence of the time stamp using assertRegexpMatches?

@erral
Copy link
Author

erral commented Jan 27, 2017

Maybe...

anyway the tests are run also for python 2.6 (https://github.com/Supervisor/superlance/blob/master/.travis.yml#L7) and according to the python docs assertRegexpMatches was introduced on python 2.7

@hvelarde
Copy link

hvelarde commented Jun 6, 2017

can someone review this and give some feedback? @mnaberez? @tseaver?

@erral what do you think on using the server locale? for instance, in one server I see this format on nginx log: [06/Jun/2017:06:25:12 -0300].

if I use the code you implemented it gets printed differently: '2017-06-06T12:51:55.352592'.

Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

Overall, the intent looks good.

  • I would be inclined to centralize the repeated "generate a message line with datestamp" bits into methods on the HTTPOk and Memmon classes, so that I could test it explicitly (the updated tests just skip over it).
  • Using a standard ISO format seems much more natural to me for logging than a locale-generated one.

@hvelarde
Copy link

hvelarde commented Jun 6, 2017

yes it could be more natural from the programing point of view, but from the user interface perspective is confusing as I will have to do time zone conversions every time I look at the logs; add to that daylight timezones and you are creating two problems instead of solving one.

@mnaberez
Copy link
Member

mnaberez commented Jun 6, 2017

I suggest logging timestamps the exact same way supervisord does, whatever that may be (it would be in supervisor/loggers.py). This would allow timestamps in the different logs to be compared easily.

@hvelarde
Copy link

hvelarde commented Jun 6, 2017

here's a typical log from supervisor and seems to be using local time:

2017-06-06 14:21:11,574 WARN killing 'instance3' (5835) with SIGKILL
2017-06-06 14:21:11,574 INFO waiting for instance3 to stop
2017-06-06 14:21:11,833 INFO stopped: instance3 (terminated by SIGKILL)
2017-06-06 14:21:11,840 INFO spawned: 'instance3' with pid 2768

it would be interesting to add the level also.

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

Successfully merging this pull request may close these issues.

4 participants