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

Log files updates #165

Merged
merged 7 commits into from
Dec 4, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 79 additions & 13 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
# Getting Started
If you are unsure about a possible contribution to the project contact the project owners about your idea.
If you are unsure about a possible contribution to the project contact the project owners about
your idea.

Please see the [code of conduct](https://github.com/panoptes/POCS/blob/develop/CODE_OF_CONDUCT.md) for our
Please see the [code of conduct](https://github.com/panoptes/POCS/blob/develop/CODE_OF_CONDUCT.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be a link to the primary branch (which is changeable), so please remove "blob/develop/" from the URL.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately those kind of links don't work in github. The blob/develop is the only way to link to an active item in to the repo AFAIK.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it.

for our
playground rules and follow them during all your contributions.


# Pull Request Process
* Pre-requisites
- Ensure you have [github account](https://github.com/join)
- If the change you wish to make is not already an Issue in the project please create one specifying the need.
- If the change you wish to make is not already an Issue in the project please create one
specifying the need.
* Process
- Create a fork of the repository and use a topic branch within your fork to make changes.
- All of our repositories have a default branch of `develop` when you first clone them, but your work should be in a separate branch.
- All of our repositories have a default branch of `develop` when you first clone them, but
your work should be in a separate branch.
- Create a branch with a descriptive name, e.g.:
- `git checkout -b new-camera-simulator`
- `git checkout -b issue-28`
Expand All @@ -22,22 +26,84 @@ playground rules and follow them during all your contributions.


# Setting up Local Environment
- Follow instructions on the [Coding in PANOPTES](https://github.com/panoptes/POCS/wiki/Coding-in-PANOPTES).
- Follow instructions on the
Copy link
Contributor

Choose a reason for hiding this comment

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

and in README.md... which probably means we need to spend some time (not immediately) to organize the docs into sensible locations.

[Coding in PANOPTES](https://github.com/panoptes/POCS/wiki/Coding-in-PANOPTES).


# Testing
- All changes should have corresponding tests and existing tests should pass after your changes.
- For more on testing see the [Coding in PANOPTES](https://github.com/panoptes/POCS/wiki/Coding-in-PANOPTES) page.
- For more on testing see the
[Coding in PANOPTES](https://github.com/panoptes/POCS/wiki/Coding-in-PANOPTES) page.


# Code Formatting

- All Python should use [PEP 8 Standards](https://www.python.org/dev/peps/pep-0008/)
- Use a tool such as [yapf](https://github.com/google/yapf) to format your
files; we'd rather spend time developing PANOPTES and not arguing about
style.
- Line length is set at 100 characters instead of 80
- It is recommended to have your editor auto-format code whenever you save a file rather than
attempt to go back and change an entire file all at once.
- Do not leave in commented-out code or unnecessary whitespace.
- Variable/function/class and file names should be meaningful and descriptive
- File names should be lower case and underscored, not contain spaces. For
example, `my_file.py` instead of `My File.py`
- Define any project specific terminology or abbreviations you use in the file you use them
- Variable/function/class and file names should be meaningful and descriptive.
- File names should be lower case and underscored, not contain spaces. For example, `my_file.py`
instead of `My File.py`.
- Define any project specific terminology or abbreviations you use in the file you use them.
- Variable/function/class and file names should be meaningful and descriptive.
- File names should be underscored, not contain spaces ex. my_file.py.
Copy link
Contributor

Choose a reason for hiding this comment

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

Two duplicate (older) lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow, I think I have merged this incorrectly every single time. ;)

- Define any project specific terminology or abbreviations you use in the file where you use them.

# Log Messages

Use appropriate logging:
- Log level:
- DEBUG (i.e. `self.logger.debug()`) should attempt to capture all run-time information.
- INFO (i.e. `self.logger.info()`) should be used sparingly and meant to convey information to a
person actively watching a running unit.
- WARNING (i.e. `self.logger.warning()`) should alert when something does not go as expected but
operation of unit can continue.
- ERROR (i.e. `self.logger.error()`) should be used at critical levels when operation cannot
continue.
- The logger supports variable information without the use of the `format` method.
- There is a `say` method that is meant to be used in friendly manner to convey information to a
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a say method available on sub-classes of PanBase that is meant...

user. This should be used only for personable output and is typically displayed in the "chat box"
Copy link
Contributor

Choose a reason for hiding this comment

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

By "personable output", shall I assume that you mean something like "messages intended for the operator"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sort of yes. I made a change to description above. The say method is only available to the POCS class (not in PanBase and therefore not in all classes) and should be very friendly output. As in something that you could feasibly send to twitter if you wanted your unit to do such a thing.

The only place so far that there is really operator output was for the drift alignment stuff and that was simply to support Nem the one time he was doing that. During normal operations the say output doesn't include anything technical.

The method is also a place that could easily be internationalized, which I have an Issue for, which would mostly be for fun. But that way someone can make their unit "speak" in whatever language they want.

of the PAWS website. These messages are also sent to the INFO level logger

#### Logging examples:

_Note: These are meant to illustrate the logging calls and are not necessarily indicative of real
operation_

```
self.logger.info("PANOPTES unit initialized: {}", self.config['name'])

self.say("I'm all ready to go, first checking the weather")

self.logger.debug("Setting up weather station")

self.logger.warning('Problem getting wind safety: {}'.format(e))

self.logger.debug("Rain: {} Clouds: {} Dark: {} Temp: {:.02f}",
is_raining,
is_cloudy,
is_dark,
temp_celsius
)

self.logger.error('Unable to connect to AAG Cloud Sensor, cannot continue')
```

#### Viewing log files

- You typically want to follow an active log file by using `tail -f` on the command line.
- The [`grc`](https://github.com/garabik/grc) (generic colouriser) can be used with `tail` to get
pretty log files.

```
(panoptes-env) $ grc tail -f $PANDIR/logs/pocs_shell.log
```

The following screenshot shows commands entered into a `jupyter-console` in the top panel and the
log file in the bottom panel.

<p align="center">
<img src="http://www.projectpanoptes.org/images/log-example.png" width="600">
</p>
2 changes: 2 additions & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
## [Unreleased]
### Changed
- Logs: separate files for each level and new naming scheme

## [0.5.1] - 2017-12-02
### Added
Expand Down
18 changes: 18 additions & 0 deletions conf_files/log.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,15 @@ logger:
all:
handlers: [all]
propagate: true
info:
handlers: [info]
propagate: true
warn:
handlers: [warn]
propagate: true
error:
handlers: [error]
propagate: true

handlers:
all:
Expand All @@ -24,12 +30,24 @@ logger:
formatter: detail
when: W6
backupCount: 4
info:
class: logging.handlers.TimedRotatingFileHandler
level: INFO
formatter: detail
when: W6
backupCount: 4
warn:
class: logging.handlers.TimedRotatingFileHandler
level: WARNING
formatter: detail
when: W6
backupCount: 4
error:
class: logging.handlers.TimedRotatingFileHandler
level: ERROR
formatter: detail
when: W6
backupCount: 4

root:
level: DEBUG
Expand Down
7 changes: 5 additions & 2 deletions pocs/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ def __init__(self, state_machine_file='simple_state_table', messaging=False, **k
PanBase.__init__(self, **kwargs)

self.logger.info('Initializing PANOPTES unit')
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove old, non-parameterized message.

self.name = self.config.get('name', 'Generic PANOPTES Unit')
self.logger.info('Initializing PANOPTES unit - {} - {}',
self.name,
self.config['location']['name']
)

self._processes = {}

Expand All @@ -67,8 +72,6 @@ def __init__(self, state_machine_file='simple_state_table', messaging=False, **k

self.status()

self.name = self.config.get('name', 'Generic PANOPTES Unit')
self.logger.info('Welcome {}!'.format(self.name))
self.say("Hi there!")

@property
Expand Down
3 changes: 2 additions & 1 deletion pocs/images.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,8 @@ def pointing_error(self):
namedtuple: Pointing error information
"""
if self._pointing_error is None:
assert self.pointing is not None, self.logger.warn("No WCS, can't get pointing_error")
assert self.pointing is not None, self.logger.warning(
"No WCS, can't get pointing_error")
Copy link
Contributor

Choose a reason for hiding this comment

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

WCS?

assert self.header_pointing is not None

if self.wcs is None:
Expand Down
6 changes: 3 additions & 3 deletions pocs/mount/bisque.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ def set_target_coordinates(self, coords):
target_set = False

if self.is_parked:
self.logger.warning("Mount is parked")
self.logger.info("Mount is parked")
else:
# Save the skycoord coordinates
self.logger.debug("Setting target coordinates: {}".format(coords))
Expand Down Expand Up @@ -173,9 +173,9 @@ def slew_to_target(self):
success = False

if self.is_parked:
self.logger.warning("Mount is parked")
self.logger.info("Mount is parked")
elif self._target_coordinates is None:
self.logger.warning("Target Coordinates not set")
self.logger.info("Target Coordinates not set")
else:
# Get coordinate format from mount specific class
mount_coords = self._skycoord_to_mount_coord(self._target_coordinates)
Expand Down
4 changes: 2 additions & 2 deletions pocs/mount/mount.py
Original file line number Diff line number Diff line change
Expand Up @@ -358,9 +358,9 @@ def slew_to_target(self):
success = False

if self.is_parked:
self.logger.warning("Mount is parked")
self.logger.info("Mount is parked")
elif not self.has_target:
self.logger.warning("Target Coordinates not set")
self.logger.info("Target Coordinates not set")
else:
success = self.query('slew_to_target')

Expand Down
71 changes: 58 additions & 13 deletions pocs/utils/logger.py
Original file line number Diff line number Diff line change
@@ -1,16 +1,43 @@
import os
import sys
import time
import datetime
import logging
import logging.config
import os
import time

from tempfile import gettempdir

from .config import load_config


class PanLogger(object):
""" Logger for PANOPTES with format style strings """

def __init__(self, logger):
super(PanLogger, self).__init__()
self.logger = logger

def _process_str(self, fmt, *args, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this method is necessary. According to the python documentation, the loggers do exactly this formatting.

Copy link
Member Author

Choose a reason for hiding this comment

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

This supports the actual format style, e.g. {:0.2f}, {:40s}, rather than just supporting {} as placeholders. The loggers support this via the %f syntax, which is simply for backwards-compatibility. This allows us to just get rid of actual format calls in the logger but still keep the same syntax.

I could definitely be wrong about this but the format specifiers didn't seem to work for me otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Perhaps add a comment explaining that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Update now that I'm trying to use the logger: it is confusing to have PanLogger use a different formatting convention from python's loggers, even though the API is otherwise identical.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I follow. You should be able to use either convention.

log_str = fmt
if len(args) > 0 or len(kwargs) > 0:
log_str = fmt.format(*args, **kwargs)

return log_str

def debug(self, fmt, *args, **kwargs):
self.logger.debug(self._process_str(fmt, *args, **kwargs))

def info(self, fmt, *args, **kwargs):
self.logger.info(self._process_str(fmt, *args, **kwargs))

def warning(self, fmt, *args, **kwargs):
self.logger.warning(self._process_str(fmt, *args, **kwargs))

def error(self, fmt, *args, **kwargs):
self.logger.error(self._process_str(fmt, *args, **kwargs))


def get_root_logger(profile='panoptes', log_config=None):
""" Creates a root logger for PANOPTES used by the PanBase object

Returns:
logger(logging.logger): A configured instance of the logger
"""
Expand All @@ -23,23 +50,41 @@ def get_root_logger(profile='panoptes', log_config=None):
for name, formatter in log_config['formatters'].items():
log_config['formatters'][name].setdefault('()', _UTCFormatter)

log_file_lookup = {
'all': "{}/logs/panoptes.log".format(os.getenv('PANDIR', '/var/panoptes')),
'warn': "{}/logs/warnings.log".format(os.getenv('PANDIR', '/var/panoptes')),
}
log_fname_datetime = datetime.datetime.utcnow().strftime('%Y%m%dT%H%M%SZ')
else:
log_fname_datetime = datetime.datetime.now().strftime('%Y%m%dT%H%M%SZ')

# Setup log file names
invoked_script = os.path.basename(sys.argv[0])
log_dir = '{}/logs'.format(os.getenv('PANDIR', gettempdir()))
log_fname = '{}-{}-{}'.format(invoked_script, os.getpid(), log_fname_datetime)
log_symlink = '{}/{}.log'.format(log_dir, invoked_script)

# Setup the TimeedRotatingFileHandler to backup in middle of day intead of middle of night
# Set log filename and rotation
for handler in log_config.get('handlers', []):
log_config['handlers'][handler].setdefault('filename', log_file_lookup[handler])
if handler in ['all', 'warn']:
log_config['handlers'][handler].setdefault('atTime', datetime.time(hour=11, minute=30))
# Set the filename
full_log_fname = '{}/{}-{}.log'.format(log_dir, log_fname, handler)
log_config['handlers'][handler].setdefault('filename', full_log_fname)

# Setup the TimeedRotatingFileHandler for middle of day
log_config['handlers'][handler].setdefault('atTime', datetime.time(hour=11, minute=30))

if handler == 'all':
# Symlink the log file to $PANDIR/logs/panoptes.log
try:
os.unlink(log_symlink)
except FileNotFoundError:
pass
finally:
os.symlink(full_log_fname, log_symlink)

# Configure the logger
logging.config.dictConfig(log_config)

# Get the logger and set as attribute to class
logger = logging.getLogger(profile)

# Don't want log messages from state machine library
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the comment as I had no idea what this was about. I wonder if this is the kind of thing that could be in conf_files/log.yaml. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we ever want to show these. There is a lot of output that is generated (something like: prepare, before enter, enter, after enter, before exit, exit, etc) and we already have our own logging for entering/exiting each state.

Copy link
Contributor

@jamessynge jamessynge Dec 4, 2017

Choose a reason for hiding this comment

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

Got it. In that case, please say so succinctly.

Don't want log messages from state machine library, it is very noisy and we have our own logging of state transitions.

logging.getLogger('transitions.core').setLevel(logging.WARNING)

try:
Expand All @@ -48,7 +93,7 @@ def get_root_logger(profile='panoptes', log_config=None):
except Exception: # pragma: no cover
pass

return logger
return PanLogger(logger)


class _UTCFormatter(logging.Formatter):
Expand Down