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

Allow setting the log level via a CLI argument #8295

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

ricekot
Copy link
Member

@ricekot ricekot commented Jan 10, 2024

No description provided.

@kingthorin
Copy link
Member

Related to: #4513

@kingthorin
Copy link
Member

kingthorin commented Jan 10, 2024

I don't see anything wrong with this. But I'm not clear on the implementation, is this using env var values? (ex: Where/how would the user set/override it?).

@ricekot
Copy link
Member Author

ricekot commented Jan 11, 2024

These can be set by passing flags to the JVM, e.g.

java -Dzap.log.level=debug -jar ./zap-2.15.0-SNAPSHOT.jar

Maybe we should also update zap.sh to accept a flag and append this to the arguments it passes to the JVM.

@psiinon
Copy link
Member

psiinon commented Jan 11, 2024

I think we should have a relatively easy documented way for users to set it, and if that means updating zap.sh (and zap.bat??) then we should do so.

@thc202 thc202 added this to the 2.15.0 milestone Jan 11, 2024
@ricekot ricekot force-pushed the cmd-line-loglevel branch 2 times, most recently from 0e2ddf2 to 871f9a4 Compare January 13, 2024 14:49
@ricekot ricekot changed the title Allow setting the log level via system properties Allow setting the log level via a CLI argument Jan 13, 2024
@ricekot
Copy link
Member Author

ricekot commented Jan 13, 2024

Updated as discussed in IRC - added a -loglevel flag that can be used to set the log level.

@psiinon
Copy link
Member

psiinon commented Jan 15, 2024

-loglevel debug gives way more than usual, but I guess thats expected?
-loglevel bad is ignored - I expected that to fail with some message about an invalid logging level..

Other than that its looking good 😁

@ricekot
Copy link
Member Author

ricekot commented Jan 16, 2024

-loglevel debug gives way more than usual

We could change it to only set the level for the ZAP classes, and leave the root log level as-is.

-loglevel bad is ignored - I expected that to fail with some message about an invalid logging level..

That was intentional. I thought a bad log level didn't warrant crashing ZAP, since we have a log level in the log4j2.properties file anyway, but I can tweak this.

@psiinon
Copy link
Member

psiinon commented Jan 16, 2024

+1 for only setting the ZAP logging levels (others may disagree?)
We should at least give a warning/error on a bad level otherwise it might be hard for a user to debug a typo?
The help will say which log levels are supported?

Signed-off-by: ricekot <github@ricekot.com>
@thc202
Copy link
Member

thc202 commented Jan 17, 2024

Thank you!

@kingthorin kingthorin merged commit b2aed8b into zaproxy:main Jan 17, 2024
9 checks passed
@ricekot ricekot deleted the cmd-line-loglevel branch January 17, 2024 12:16
Copy link

This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

4 participants