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

Limit mixxx.log size #13684

Merged
merged 2 commits into from
Dec 15, 2024
Merged

Limit mixxx.log size #13684

merged 2 commits into from
Dec 15, 2024

Conversation

daschuer
Copy link
Member

This sets a limit to 100 MB to lint the issue reported here:
#13660

The limit can be adjusted by the command line parameter --log-max-file-size

Copy link
Member

@acolombier acolombier left a comment

Choose a reason for hiding this comment

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

What is the expected behaviour with log rotation? Currently, my log files are rotated with up to 10 log archive:

-rw-rw-r--  90K  ../.mixxx/mixxx.log
-rw-rw-r--  90K  ../.mixxx/mixxx.log.1
-rw-rw-r-- 128K  ../.mixxx/mixxx.log.10
-rw-rw-r--  80K  ../.mixxx/mixxx.log.2
-rw-rw-r--  80K  ../.mixxx/mixxx.log.3
-rw-rw-r-- 116K  ../.mixxx/mixxx.log.4
-rw-rw-r--  80K  ../.mixxx/mixxx.log.5
-rw-rw-r-- 121K  ../.mixxx/mixxx.log.6
-rw-rw-r-- 254K  ../.mixxx/mixxx.log.7
-rw-rw-r-- 927K  ../.mixxx/mixxx.log.8
-rw-rw-r--  64K  ../.mixxx/mixxx.log.9

Is this something that Mixxx handle cross platform? Is that some logrotate Linux specific behaviour? In case linux already ship with some logrotate setup, I would suggest we keep unlimited the default in there.

Also a suggestion, instead of preventing further append to the log file, shall we discard the N first lines, N being the minimum required line to discard to allow appending the new line. Appreciate this will have a performance impact, but this will also make the log usable.

Another suggestion, we could solve this issue by introducing a duplication limiter like for ffmpeg, where the log file will not write duplicated lines but will instead write the line once, and maintain a repeater counter on the next line.

[HidIoThread Traktor Kontrol S2 9E2E_3] Unable to read buffered HID InputReports from "Traktor Kontrol S2 9E2E_3" : "hid_read_timeout: unexpected poll error (device disconnected)"
[Previous line repeated 1547 times]

The limitation is, you may have a repeated pattern, which wouldn't be de-dupped.

@daschuer
Copy link
Member Author

The log rotation happens after each start up of Mixxx. It is a Mixxx home made implementation.
We can improve on this topic a lot. This PR was just meant as a band aid for a real party stopper.
Intended for the 2.4. stable branch.

Does that suite?

@acolombier
Copy link
Member

I think it make sense to quickly issue that mitigation to 2.4, but I would be worried that this becomes a long term fix as well. Shall we merge that in 2.4 but not sync it with next version till we have a proper solution?

@daschuer
Copy link
Member Author

Shall we merge that in 2.4 but not sync it with next version till we have a proper solution?

This should help in the 2.5 and main branch in the same way. What is the main disadvantage?

src/util/logging.h Outdated Show resolved Hide resolved
src/util/cmdlineargs.h Outdated Show resolved Hide resolved
@daschuer
Copy link
Member Author

I have amended your suggestions.

@daschuer
Copy link
Member Author

daschuer commented Oct 6, 2024

merge?

@acolombier
Copy link
Member

What is the main disadvantage?

IMO, this could lead to the log file to include data as it will stop outputting if it reaches the limit. Usually, relevant information to help fixing a bug or a crash tends to be closer to the end of the file, which means this could limit the usefulness of the log file.

In the meantime, 100MB is fairly large, but I can imagine DJs using Mixxx to broadcast to radios for example may extend that limit

@daschuer
Copy link
Member Author

daschuer commented Oct 7, 2024

Do you propose a different size? Different approach?
On one hand we have the valid request for not using the entire disk for log files to protect against crashes. On the other hand we want to see everything in case of a crash.

@daschuer
Copy link
Member Author

daschuer commented Dec 2, 2024

@acolombier can we merge this as it is or is there something to do?

@daschuer daschuer added this to the 2.5.0 milestone Dec 2, 2024
@acolombier
Copy link
Member

I'm happy with merging this as mitigation. I thought @Swiftb0y had change requests tho?

src/util/logging.cpp Outdated Show resolved Hide resolved
@daschuer daschuer changed the base branch from 2.4 to 2.5 December 2, 2024 18:04
@daschuer
Copy link
Member Author

daschuer commented Dec 2, 2024

done

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Pretty sure this LGTM.

@JoergAtGithub
Copy link
Member

Lets merge this!

@JoergAtGithub JoergAtGithub merged commit 42d509e into mixxxdj:2.5 Dec 15, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants