Add logging_util and rotating log archive for uiserver logs #376

datamel · 2022-08-11T12:36:35Z

Logs for the uiserver can now be found in ~/.cylc/uiserver/log

These changes close #352

When a new instance is started, the previous log is archived. The archive will retain the last 5 logs.
For convenience, and to keep functionality similar to cylc flow, a log symlink, linking to the latest log has been added.

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).
Any dependency changes applied to setup.cfg.

Appropriate tests are included (unit and/or functional).

Appropriate change log entry included.

I have opened a documentation PR at cylc/cylc-doc/pull/XXXX.
No documentation update required.

codecov-commenter · 2022-08-11T12:46:47Z

Codecov Report

Merging #376 (e95e263) into master (fe49e35) will increase coverage by 0.56%.
The diff coverage is 95.12%.

@@            Coverage Diff             @@
##           master     #376      +/-   ##
==========================================
+ Coverage   79.15%   79.72%   +0.56%     
==========================================
  Files          11       12       +1     
  Lines        1118     1159      +41     
  Branches      215      219       +4     
==========================================
+ Hits          885      924      +39     
- Misses        195      197       +2     
  Partials       38       38

Impacted Files	Coverage Δ
cylc/uiserver/__init__.py	`87.50% <50.00%> (-12.50%)`	⬇️
cylc/uiserver/logging_util.py	`100.00% <100.00%> (ø)`
cylc/uiserver/scripts/gui.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cylc/uiserver/scripts/hub.py

oliver-sanders

Tested, works well.

cylc/uiserver/jupyter_config.py

oliver-sanders · 2022-08-17T08:08:23Z

Nice, tested with single & multi user modes, rollover worked for both even when interleaved.

wxtim · 2022-08-17T14:45:24Z

cylc/uiserver/logging_util.py

+        """Increment the log number by one for each log"""
+        # increments each log number, done in descending order to avoid
+        # filename conflicts


style suggestion

Suggested change

"""Increment the log number by one for each log"""

# increments each log number, done in descending order to avoid

# filename conflicts

"""Increment the log number by one for each log.

Increments each log number, done in descending order to avoid

filename conflicts.

"""

wxtim · 2022-08-17T14:50:05Z

cylc/uiserver/logging_util.py

+            self.file_path, f"*{self.LOG_NAME_EXTENSION}")), reverse=True)
+        for file in log_files:
+            log_num = int(Path(file).name.partition('-')[0]) + 1
+            new_file_name = Path(


Why can't you just

if log_num > 5: file.unlink()

And never create the older archive members?

wxtim · 2022-08-17T14:51:23Z

cylc/uiserver/scripts/hub.py

@@ -24,7 +24,7 @@

 from cylc.uiserver import (
    __version__,
-    __file__ as uis_pkg
+    __file__ as uis_pkg,


Any particular reason?

No, I initially added init_log to this import. I'll remove.

FYI: trailing commas [perfectly valid] are a signal to black to leave room for further additions so can be considered functional. Although we don't use black I use trailing commas as a matter of course.

So do I; it was more that I was curious that this had been changed at all for this PR.

wxtim · 2022-08-17T14:55:19Z

cylc/uiserver/tests/test_logging_util.py

+    for file in [
+        '03-uiserver.log',
+        '01-uiserver.log',
+        '04-uiserver.log',
+        '02-uiserver.log',
+        '05-uiserver.log',
+        '07-uiserver.log',
+        '06-uiserver.log'
+    ]:


You could make these tests a bit smaller if you wished to:

Suggested change

for file in [

'03-uiserver.log',

'01-uiserver.log',

'04-uiserver.log',

'02-uiserver.log',

'05-uiserver.log',

'07-uiserver.log',

'06-uiserver.log'

]:

for file in [f'{i:d02}-uiserver.log' for i in range(1,7)]

I was making them intentionally out of numerical order.

wxtim · 2022-08-17T14:57:36Z

cylc/uiserver/tests/test_logging_util.py

+        ]:
+        log_file = LOG.file_path.joinpath(file)
+        log_file.touch()
+        log_file.write_text(f"File: {file}")


I find this easier to understand?

Suggested change

log_file.write_text(f"File: {file}")

log_file.write_text(f"This file started as: {file}")

wxtim · 2022-08-17T14:59:15Z

cylc/uiserver/tests/test_logging_util.py

+        f'{LOG.file_path}/03-uiserver.log',
+        f'{LOG.file_path}/02-uiserver.log']
+    assert sorted(log_files) == sorted(expected_files)
+    actual_output = Path(LOG.file_path/'03-uiserver.log').read_text() # LOG.file_path/{03-uiserver.log}


What's the comment for?

I'll remove.

wxtim · 2022-08-17T15:10:43Z

cylc/uiserver/logging_util.py

+        log_files = sorted(glob(os.path.join(
+            self.file_path, f"*{self.LOG_NAME_EXTENSION}")), reverse=True)
+        for file in log_files:
+            log_num = int(Path(file).name.partition('-')[0]) + 1


You might want to consider using a try/except loop around this, or making the glob a bit more careful ([0-9]*{self.LOG_NAME_EXTENSION}).

Otherwise I can break this thus:

touch ~/.cylc/uiserver/log/iduhafiuhbjvfaiehr-uiserver.log

Good spot, unlikely but worth fixing.

wxtim

It looks like it's working, apart from my destructive testing where I added fkjaiuj-uiserver.log to the log directory.

I've made a few suggestions of the mild/moderate sort.

datamel · 2022-08-18T18:51:22Z

@wxtim I've done a small refactor to address your comments.

Add logging_util and rotating log archive for uiserver logs

8d5364d

datamel added this to the 1.2.0 milestone Aug 11, 2022

datamel self-assigned this Aug 11, 2022

Add new files

dbaaf40

datamel requested a review from oliver-sanders August 11, 2022 12:52

Move log init to function

6b04631

oliver-sanders reviewed Aug 12, 2022

View reviewed changes

cylc/uiserver/scripts/hub.py Outdated Show resolved Hide resolved

oliver-sanders reviewed Aug 12, 2022

View reviewed changes

Remove logging init from hub

0f20510

datamel commented Aug 12, 2022

View reviewed changes

cylc/uiserver/jupyter_config.py Show resolved Hide resolved

Change log

2253028

oliver-sanders approved these changes Aug 17, 2022

View reviewed changes

oliver-sanders requested a review from wxtim August 17, 2022 08:08

wxtim reviewed Aug 17, 2022

View reviewed changes

datamel added 2 commits August 18, 2022 16:57

Refactor

427c57f

Tidy

e95e263

datamel requested a review from wxtim August 18, 2022 18:51

wxtim approved these changes Aug 19, 2022

View reviewed changes

wxtim merged commit 549afbd into cylc:master Aug 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logging_util and rotating log archive for uiserver logs #376

Add logging_util and rotating log archive for uiserver logs #376

datamel commented Aug 11, 2022 •

edited by oliver-sanders

Loading

codecov-commenter commented Aug 11, 2022 •

edited

Loading

oliver-sanders left a comment

oliver-sanders commented Aug 17, 2022

wxtim Aug 17, 2022

wxtim Aug 17, 2022

wxtim Aug 17, 2022

datamel Aug 18, 2022

oliver-sanders Aug 18, 2022 •

edited

Loading

wxtim Aug 19, 2022

wxtim Aug 17, 2022 •

edited

Loading

datamel Aug 18, 2022

wxtim Aug 17, 2022

wxtim Aug 17, 2022

datamel Aug 18, 2022

wxtim Aug 17, 2022 •

edited

Loading

oliver-sanders Aug 18, 2022

wxtim left a comment

datamel commented Aug 18, 2022

	log_file.write_text(f"File: {file}")
	log_file.write_text(f"This file started as: {file}")

Add logging_util and rotating log archive for uiserver logs #376

Add logging_util and rotating log archive for uiserver logs #376

Conversation

datamel commented Aug 11, 2022 • edited by oliver-sanders Loading

codecov-commenter commented Aug 11, 2022 • edited Loading

Codecov Report

oliver-sanders left a comment

Choose a reason for hiding this comment

oliver-sanders commented Aug 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sanders Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wxtim Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wxtim Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wxtim left a comment

Choose a reason for hiding this comment

datamel commented Aug 18, 2022

datamel commented Aug 11, 2022 •

edited by oliver-sanders

Loading

codecov-commenter commented Aug 11, 2022 •

edited

Loading

oliver-sanders Aug 18, 2022 •

edited

Loading

wxtim Aug 17, 2022 •

edited

Loading

wxtim Aug 17, 2022 •

edited

Loading