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: Add show-startup-logs flag. #1218

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

apoorvapendse
Copy link

@apoorvapendse apoorvapendse commented Nov 20, 2024

This enables the user to enable or disable the startup logs while starting the tusd server. It helps the user suppress the startup logs as per their preference.

The flag has a default value of true.
Fixes #1216.

Before:
image


After:

set as false:

image

set as true explicity:
image

default:
image

@Acconut
Copy link
Member

Acconut commented Nov 26, 2024

Thank you for opening this PR! While I agree that it's useful to control the startup logs, I am not sure if a dedicated flag is the best approach. In #1216 (comment), I propose the idea of adding a generic flag for controlling the log level. If the log level does allow info messages, the startup logs would not be printed. Feel free to join the discussion in the other issue and let us know what you think.

This would also help us to remove the additional if statements that are introduced in this PR. They are quite cumbersome and I worry that in the future we forget about them and add direct log statements that should actually go in such an if statement. Using slog's method for logging at specific levels removes the need for these conditional clauses.

@apoorvapendse
Copy link
Author

Thanks for the feedback.
I'd like to collaborate with you to make the log level feature, if that's fine.
Let me know how you are planning to approach it.
I will read up on the documentation of slog in the meantime.

if Flags.S3Endpoint == "" {
if Flags.S3TransferAcceleration {
stdout.Printf("Using 's3://%s' as S3 bucket for storage with AWS S3 Transfer Acceleration enabled.\n", Flags.S3Bucket)
if Flags.ShowStartupLogs {

Choose a reason for hiding this comment

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

I think a better interface would be to have a function, printStartupLogLine, that does the ShowStartupLogs checks but is otherwise a wrapper for stdout.PrintF.

That'd avoid the excessive indentation here, as well as being likely a lot more robust to refactoring.

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense to me.
@Acconut what do you think about this?

Choose a reason for hiding this comment

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

You can probably just try implementing it; it's easier for maintainers to review a concept like this after seeing it. The current version seems unlikely to be merged.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I'll update this PR 👍
Thanks Tim!

@apoorvapendse
Copy link
Author

image

image

I've made the updations.

This adds the 'show-startup-logs' flag to the tusd server, allowing the
user to enable or disable startup logs. This helps the user suppress
startup logs as per their preference.

Fixes tus#1216.
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.

Option to disable noisy log output to stdout on startup
3 participants