Add logger rotation for python services #1512

patrickelectric · 2023-02-22T18:59:47Z

No description provided.

patrickelectric · 2023-02-23T19:12:07Z

It does not zip logs from the camera manager since the extension is a date and not a .log file, we should fix on the camera manager, but for everything else it's working.

joaoantoniocardoso · 2023-02-23T20:27:42Z

core/services/log_zipper/main.py

+    for file in files:
+        try:
+            os.remove(file)
+            logger.debug(f"Deleted file: {file}")
+        except OSError as e:
+            logger.debug(f"Error deleting file: {file} - {e}")


As I said in the other PR, we should check if the file is opened (by some other service) before removing it, otherwise, we would lose all the next log lines until the service creates another log file.

Any suggestion of how to do it ?
I did try the following approach but was unable to make it work:

Yeah, I'm not sure why your code is not working, makes sense to me. My only concern is if using attrs parameter to create the info dict is populating correctly the open_files. Have you tried calling Process.open_files() instead? I believe it might skip some processes when an exception happens when it tries to access the process information, and by calling Process.open_files() you'd get the exception.

As an alternative, from the terminal it's usually done with lsof <filename>, maybe you could just call it from Python and check its return?

Can we use delete_everything here instead of os.remove?

@joaoantoniocardoso check the latest version

joaoantoniocardoso

Now I can't click on 'remove' 👀

patrickelectric · 2023-03-02T14:50:15Z

Now I can't click on 'remove' 👀

Remove is disabled if the size is under 100MB.

joaoantoniocardoso · 2023-03-02T14:54:04Z

Now I can't click on 'remove' eyes

Remove is disabled if the size is under 100MB.

Makes sense. I'll let it run for a while here to see the rotation. Anything specific you want me to pay attention to?

patrickelectric · 2023-03-02T15:00:54Z

Now I can't click on 'remove' eyes

Remove is disabled if the size is under 100MB.

Makes sense. I'll let it run for a while here to see the rotation. Anything specific you want me to pay attention to?

Keep in mind that the PR is zipping things in a higher frequency to help with the test.

joaoantoniocardoso · 2023-03-02T16:06:27Z

I'm still waiting for the zip to happen, but Kraken seems to be generating doubled logs:

edit: Yeah, there are two init_logs calls in kraken.py

core/services/log_zipper/main.py

core/services/kraken/main.py

joaoantoniocardoso · 2023-03-02T16:34:17Z

core/services/log_zipper/setup.py

+setuptools.setup(
+    name="log_zipper",
+    version="0.1.0",
+    description="logrotate but better",


that's a bold statement isn't it 😆

joaoantoniocardoso · 2023-03-02T16:36:30Z

core/services/kraken/main.py

 try:
-    logger.add(get_new_log_path(SERVICE_NAME))
+    init_logger(SERVICE_NAME)
 except Exception as e:
    print(f"unable to set logging path: {e}")


If the logger can fail, we should check everywhere if we want the services to be prevented or not running without logs.

If the logger can fail, we should check everywhere if we want the services to be prevented or not running without logs.

That's not related to this PR.

patrickelectric · 2023-03-02T17:52:54Z

I'm still waiting for the zip to happen, but Kraken seems to be generating doubled logs:

edit: Yeah, there are two init_logs calls in kraken.py

Fixed.

joaoantoniocardoso

Zipped file names are not that consistent, but I'm okay with that

patrickelectric · 2023-03-02T19:28:34Z

Zipped file names are not that consistent, but I'm okay with that

They are rotated and not rotated logs

joaoantoniocardoso · 2023-03-02T19:38:54Z

Zipped file names are not that consistent, but I'm okay with that

They are rotated and not rotated logs

it makes sense, thanks

…minutes Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

patrickelectric · 2023-03-10T11:26:32Z

@ES-Alexander where should this be documented ?

ES-Alexander · 2023-03-10T12:11:32Z

@ES-Alexander where should this be documented ?

What's the most relevant UI element?
As initial thoughts:

the service logs are mentioned in the BlueOS Settings section
the log files are individually accessible from the File Browser
the log files are created by the Available Services

I feel like the behaviour change is most noticeable in the file browser, so maybe we should

document it in the file browser section
- add a main point about services creating logs, and
  - a sub-point saying that logs are rotated (at x rate) to avoid filling up space
add a "see the File Browser (link) to access log files for individual services" note/dot-point to the BlueOS Settings section

I'm unsure whether it's relevant to change the Available Services docs, and it's also possible we want to rename the Log Browser section to Autopilot Log Browser, or at least clarify the text in it to make it explicit that that's for autopilot logs, not BlueOS service logs.

joaoantoniocardoso · 2023-03-10T14:39:16Z

@ES-Alexander where should this be documented ?

What's the most relevant UI element? As initial thoughts:
* the service logs are mentioned in the [BlueOS Settings](https://github.com/bluerobotics/ardusub-zola/blob/BlueOS-devel-temp/advanced-usage/index.md#blueos-settings) section

* the log files are individually accessible from the [File Browser](https://github.com/bluerobotics/ardusub-zola/blob/BlueOS-devel-temp/advanced-usage/index.md#file-browser)

* the log files are created by the [Available Services](https://github.com/bluerobotics/ardusub-zola/blob/BlueOS-devel-temp/advanced-usage/index.md#available-services)
I feel like the behaviour change is most noticeable in the file browser, so maybe we should
1. document it in the file browser section
   
   * add a main point about services creating logs, and
     
     * a sub-point saying that logs are rotated (at x rate) to avoid filling up space

2. add a "see the File Browser (link) to access log files for individual services" note/dot-point to the BlueOS Settings section
I'm unsure whether it's relevant to change the Available Services docs, and it's also possible we want to rename the Log Browser section to Autopilot Log Browser, or at least clarify the text in it to make it explicit that that's for autopilot logs, not BlueOS service logs.

I'd vote to change to Autopilot Log Browser as I've found myself entering the wrong page a few times when searching for our services logs. 😆

ES-Alexander · 2023-03-12T04:08:28Z

I'd vote to change to Autopilot Log Browser as I've found myself entering the wrong page a few times when searching for our services logs. 😆

@joaoantoniocardoso fair enough - I also think it would be helpful to clarify, although on reflection it might be preferable to have a shorter name like "Autopilot Logs" instead. Regardless, the documentation follows the implementation, so this is a BlueOS issue. Maybe we can discuss preferences in the next BlueOS meeting :-)

We could also consider having separated "Autopilot" and "BlueOS" sections in the menu-bar :-P

Williangalvani · 2023-03-12T16:23:18Z

"Flight logs" maybe?

…

On Sun, Mar 12, 2023, 01:08 ES-Alexander ***@***.***> wrote: I'd vote to change to Autopilot Log Browser as I've found myself entering the wrong page a few times when searching for our services logs. 😆 @joaoantoniocardoso <https://github.com/joaoantoniocardoso> fair enough - I also think it would be helpful to clarify, although on reflection it might be preferable to have a shorter name like "Autopilot Logs" instead. Regardless, the documentation follows the implementation, so this is a BlueOS issue. Maybe we can discuss preferences in the next BlueOS meeting :-) We could also consider having separated "Autopilot" and "BlueOS" sections in the menu-bar :-P — Reply to this email directly, view it on GitHub <#1512 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA6T53H2ZYJ23BM4JIH5D5TW3VD4NANCNFSM6AAAAAAVEWUPZM> . You are receiving this because your review was requested.Message ID: ***@***.***>

patrickelectric force-pushed the rotate_loggert branch 8 times, most recently from bd747b8 to 75e9d29 Compare February 23, 2023 18:47

patrickelectric marked this pull request as ready for review February 23, 2023 19:11

patrickelectric requested review from Williangalvani and joaoantoniocardoso February 23, 2023 19:11

joaoantoniocardoso reviewed Feb 23, 2023

View reviewed changes

patrickelectric force-pushed the rotate_loggert branch 2 times, most recently from 68f334a to 927ab66 Compare March 2, 2023 13:44

joaoantoniocardoso requested changes Mar 2, 2023

View reviewed changes

patrickelectric requested a review from joaoantoniocardoso March 2, 2023 15:35

joaoantoniocardoso requested changes Mar 2, 2023

View reviewed changes

core/services/log_zipper/main.py Outdated Show resolved Hide resolved

core/services/kraken/main.py Outdated Show resolved Hide resolved

joaoantoniocardoso reviewed Mar 2, 2023

View reviewed changes

patrickelectric force-pushed the rotate_loggert branch from 927ab66 to 78f520b Compare March 2, 2023 17:52

joaoantoniocardoso approved these changes Mar 2, 2023

View reviewed changes

patrickelectric force-pushed the rotate_loggert branch from 78f520b to 9a89dbf Compare March 6, 2023 11:30

ES-Alexander added the docs-needed Change needs to be documented label Mar 8, 2023

patrickelectric added 5 commits March 8, 2023 13:07

core: lib: commonwealth: logs: Add LogRotator to rotate log every 10 …

ec4cee4

…minutes Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

core: services: Use new init_logger function

9724249

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

core: services: Add log_zipper

a333d67

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

core: start-bluos-core: Add log_zipper service

5671292

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

core: {libs,services}: Move delete_everything to commonwealth

86f6531

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>

patrickelectric force-pushed the rotate_loggert branch from 9a89dbf to 86f6531 Compare March 8, 2023 16:07

ES-Alexander mentioned this pull request Mar 10, 2023

advanced-usage: Add information about zipped logs bluerobotics/ardusub-zola#64

Merged

patrickelectric merged commit f6e74bb into bluerobotics:master Mar 10, 2023

ES-Alexander added docs-complete Change documentation has been completed and removed docs-needed Change needs to be documented labels Mar 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logger rotation for python services #1512

Add logger rotation for python services #1512

patrickelectric commented Feb 22, 2023

patrickelectric commented Feb 23, 2023

joaoantoniocardoso Feb 23, 2023

patrickelectric Feb 27, 2023

joaoantoniocardoso Feb 27, 2023 •

edited

joaoantoniocardoso Mar 2, 2023

patrickelectric Mar 2, 2023

joaoantoniocardoso left a comment

patrickelectric commented Mar 2, 2023 •

edited

joaoantoniocardoso commented Mar 2, 2023

patrickelectric commented Mar 2, 2023

joaoantoniocardoso commented Mar 2, 2023 •

edited

joaoantoniocardoso Mar 2, 2023

joaoantoniocardoso Mar 2, 2023 •

edited

patrickelectric Mar 2, 2023

patrickelectric commented Mar 2, 2023

joaoantoniocardoso left a comment •

edited

patrickelectric commented Mar 2, 2023

joaoantoniocardoso commented Mar 2, 2023

patrickelectric commented Mar 10, 2023

ES-Alexander commented Mar 10, 2023

joaoantoniocardoso commented Mar 10, 2023

ES-Alexander commented Mar 12, 2023

Williangalvani commented Mar 12, 2023 via email

Add logger rotation for python services #1512

Add logger rotation for python services #1512

Conversation

patrickelectric commented Feb 22, 2023

patrickelectric commented Feb 23, 2023

joaoantoniocardoso Feb 23, 2023

Choose a reason for hiding this comment

patrickelectric Feb 27, 2023

Choose a reason for hiding this comment

joaoantoniocardoso Feb 27, 2023 • edited

Choose a reason for hiding this comment

joaoantoniocardoso Mar 2, 2023

Choose a reason for hiding this comment

patrickelectric Mar 2, 2023

Choose a reason for hiding this comment

joaoantoniocardoso left a comment

Choose a reason for hiding this comment

patrickelectric commented Mar 2, 2023 • edited

joaoantoniocardoso commented Mar 2, 2023

patrickelectric commented Mar 2, 2023

joaoantoniocardoso commented Mar 2, 2023 • edited

joaoantoniocardoso Mar 2, 2023

Choose a reason for hiding this comment

joaoantoniocardoso Mar 2, 2023 • edited

Choose a reason for hiding this comment

patrickelectric Mar 2, 2023

Choose a reason for hiding this comment

patrickelectric commented Mar 2, 2023

joaoantoniocardoso left a comment • edited

Choose a reason for hiding this comment

patrickelectric commented Mar 2, 2023

joaoantoniocardoso commented Mar 2, 2023

patrickelectric commented Mar 10, 2023

ES-Alexander commented Mar 10, 2023

joaoantoniocardoso commented Mar 10, 2023

ES-Alexander commented Mar 12, 2023

Williangalvani commented Mar 12, 2023 via email

joaoantoniocardoso Feb 27, 2023 •

edited

patrickelectric commented Mar 2, 2023 •

edited

joaoantoniocardoso commented Mar 2, 2023 •

edited

joaoantoniocardoso Mar 2, 2023 •

edited

joaoantoniocardoso left a comment •

edited