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

logging: configure default logging config #349

Merged
merged 1 commit into from
Jun 14, 2022

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented May 17, 2022

Provide a default logging setup for the UIS:

  • Bump Traitlets to >=5.2.1 (needed for logging_config) trait.
  • Add default logging config to write to ~/.cylc/uiserver/uiserver.log at the INFO level.
  • Document in the readme for now.

Note this only captures the CylcUIServer part of the log, the base ServerApp log (i.e. the jupyter_server log) and any extensions (e.g. jupyter_lab) are excluded (can be easily added if desired but not much use from the Cylc perspective).

In combination with #348 this should capture workflow state transitions and tracebacks which are useful for debugging and monitoring for issues.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Does not need tests (please test manually)
  • Appropriate change log entry included.
  • No documentation update required (waiting on feature to be documented in jupyter_server)
  • No dependency changes.

@oliver-sanders
Copy link
Member Author

(rebased)

@oliver-sanders
Copy link
Member Author

oliver-sanders commented May 18, 2022

@wxtim pointed out that uncaptured tracebacks go to the ServerApp log rather than the CylcUIServer log so I've added logging for the ServerApp too (pointing at a different file).

Ignore that, Tim found that an exception raised by the "play" service (within the bit which actually performs the play) appears to get printed rather than logged. As it happens that method makes extensive use of try/except and logs any exceptions which occur so this should not be an issue. I'm not sure why an exception raised there gets printed rather than logged, my guess is that it's related to the ThreadPoolExecutor, perhaps exception handling would need to be configured there. Not a big issue though, we should be try/excepting everything in these methods.

'filename': str(USER_CONF_ROOT / 'uiserver.log'),
'mode': 'a',
'backupCount': 5,
'maxBytes': 10485760,
Copy link
Member Author

Choose a reason for hiding this comment

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

Copied from Cylc, perhaps we should reduce this a little?

Copy link
Member

Choose a reason for hiding this comment

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

Given the amount of logging the UIS doesn't produce I feel that this is likely to be overkill.

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 reckon 100'000 bytes should be ~2000 lines worth of log file which seems plenty long enough.

Suggested change
'maxBytes': 10485760,
'maxBytes': 100000,

@oliver-sanders
Copy link
Member Author

Could potentially bump this to the 1.0.2 milestone (only adds logging) but no pressure from our side (happy to use dev builds for the moment).

* Bump Traitlets to >=5.2.1 (needed for logging_config) trait.
* Add default logging config to write to ~/.cylc/uiserver/uiserver.log
  at the INFO level.
* Document in the readme for now.
@oliver-sanders
Copy link
Member Author

Rebased, added changelog.

Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

  • Code looks sane
  • Working as advertised

@wxtim wxtim merged commit 9f0e55d into cylc:master Jun 14, 2022
@oliver-sanders oliver-sanders deleted the logging-config-2 branch June 14, 2022 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants