Send log messages to log4j systems instead of system out / error #3231

DarshitChanpura · 2023-08-22T15:55:27Z

Description

A lot of files (mostly tests) have sysout's which do nothing but degrade the performance. This PR removes all sysouts from tests, replace sysouts with logger in project files.
Also removes all e.printStacktrace(); calls.

tools/ package - no sysouts were removed from here
HTTPSpnegoAuthenticator.java - a portion was not modified since it enabled debugging on console.

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Darshit Chanpura <[email protected]>

codecov · 2023-08-22T16:06:39Z

Codecov Report

Merging #3231 (17d83a1) into main (75f529a) will increase coverage by 0.03%.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##               main    #3231      +/-   ##
============================================
+ Coverage     62.43%   62.47%   +0.03%     
+ Complexity     3352     3350       -2     
============================================
  Files           254      254              
  Lines         19748    19737      -11     
  Branches       3334     3334              
============================================
+ Hits          12330    12331       +1     
+ Misses         5789     5777      -12     
  Partials       1629     1629

Files Changed	Coverage Δ
...ic/auth/http/kerberos/HTTPSpnegoAuthenticator.java	`0.00% <0.00%> (ø)`
...ch/security/securityconf/DynamicConfigFactory.java	`54.76% <ø> (+0.64%)`	⬆️
...arch/security/ssl/OpenSearchSecuritySSLPlugin.java	`85.11% <ø> (+1.55%)`	⬆️
...opensearch/security/ssl/util/SSLRequestHelper.java	`67.85% <0.00%> (+0.60%)`	⬆️
...opensearch/security/tools/AuditConfigMigrater.java	`0.00% <ø> (ø)`
...n/java/org/opensearch/security/tools/Migrater.java	`0.00% <ø> (ø)`
...a/org/opensearch/security/tools/SecurityAdmin.java	`44.50% <ø> (+0.09%)`	⬆️
...g/opensearch/security/auditlog/sink/DebugSink.java	`100.00% <100.00%> (ø)`

DarshitChanpura · 2023-08-22T16:43:18Z

Taking a look at the test failure

src/main/java/org/opensearch/security/tools/AuditConfigMigrater.java

src/test/java/org/opensearch/security/ConfigTests.java

peternied

Nice - thanks for keeping the console output cleaner. Can you see about adding an rule to prevent system.out/system.err usage to keep the codebase programmatically cleaned?

Note; I think adding exceptions is fine for a couple places we do want console output

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura · 2023-08-22T19:25:40Z

Nice - thanks for keeping the console output cleaner. Can you see about adding an rule to prevent system.out/system.err usage to keep the codebase programmatically cleaned?

Note; I think adding exceptions is fine for a couple places we do want console output

I was contemplating between adding such check and not adding it, but your comment helped decide :D. I have added a github check.

peternied · 2023-08-22T19:30:30Z

.github/workflows/code-hygiene.yml

@@ -83,3 +83,17 @@ jobs:
          fi
        done
        exit $exit_code
+
+  check-println:


Instead of a new check, what about leveraging checkstyle's RegexpSingleline, below was how the 'extension' filter was added, maybe we could add entries for system.out.print & system.err.print that would be easier to maintain

<module name="RegexpSingleline"> <property name="format" value="extension"/> <property name="ignoreCase" value="true"/> <property name="message" value="Extension should only be used sparingly to keep implementations as generic as possible" /> <property name="severity" value="error"/> </module>

I tried that but didn't have a working solution, I'll invest some more time to see if I can make it work

I was not able to have it working with:

<module name="RegexpSingleline"> <property name="format" value="System\.out\.println" /> <property name="message" value="Do not use System.out.println" />  <property name="fileExtensions" value="java" /> <property name="exclude" value="./src/main/java/org/opensearch/security/tools, ./src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java" /> </module>

Try building off of this, works locally for me

<module name="RegexpSingleline"> <property name="format" value="println"/> <property name="ignoreCase" value="true"/> <property name="message" value="SYSTEM.OUT.PRINTLN NONONO!" /> <property name="severity" value="error"/> </module>

./gradlew checkstyleMain ... > Task :checkstyleMain [ant:checkstyle] [ERROR] /local/home/petern/git/security/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java:247: SYSTEM.OUT.PRINTLN NONONO! [RegexpSingleline] [ant:checkstyle] [ERROR] /local/home/petern/git/security/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java:248: SYSTEM.OUT.PRINTLN NONONO! [RegexpSingleline] [ant:checkstyle] [ERROR] /local/home/petern/git/security/src/main/java/org/opensearch/security/auditlog/sink/AuditLogSink.java:63: SYSTEM.OUT.PRINTLN NONONO! [RegexpSingleline] [ant:checkstyle] [ERROR] /local/home/petern/git/security/src/main/java/org/opensearch/security/auditlog/sink/DebugSink.java:30: SYSTEM.OUT.PRINTLN NONONO! [RegexpSingleline] [ant:checkstyle] [ERROR] /local/home/petern/git/security/src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java:161: SYSTEM.OUT.PRINTLN NONONO! [RegexpSingleline] [ant:checkstyle] [ERROR] /local/home/petern/git/security/src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java:162: SYSTEM.OUT.PRINTLN NONONO! [RegexpSingleline] [ant:checkstyle] [ERROR] /local/home/petern/git/security/src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java:228: SYSTEM.OUT.PRINTLN NONONO! [RegexpSingleline] [ant:checkstyle] [ERROR] /local/home/petern/git/security/src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java:229: SYSTEM.OUT.PRINTLN NONONO! [RegexpSingleline] [ant:checkstyle] [ERROR] /local/home/petern/git/security/src/main/java/org/opensearch/security/tools/AuditConfigMigrater.java:94: SYSTEM.OUT.PRINTLN NONONO! [RegexpSingleline] [ant:checkstyle] [ERROR] /local/home/petern/git/security/src/main/java/org/opensearch/security/tools/AuditConfigMigrater.java:109: SYSTEM.OUT.PRINTLN NONONO! [RegexpSingleline] [ant:checkstyle] [ERROR] /local/home/petern/git/security/src/main/java/org/opensearch/security/tools/AuditConfigMigrater.java:112: SYSTEM.OUT.PRINTLN NONONO! [RegexpSingleline]

Yes, but the problem is how to exclude. There is no such Property called exclude. Do you know of anything of that might work?
Closest thing I found was this: https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/sun_checks.xml#L46-L50
However this applies to whole file, so I might have to end up creating a separate xml for sysout check anyway. (didn't try it yet)

I was able to achieve this by adding a separate file for println checks.

That's too hard lol - exclusions can be done in place, see https://github.com/opensearch-project/security/blob/main/DEVELOPER_GUIDE.md#checkstyle-violations

I know but this would allow us to exclude files in one place instead of marking them individually. I felt like this was cleaner, so I went with this option. What do you think?

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura · 2023-08-22T20:53:08Z

Taking a look at the test failure

Fixed

Signed-off-by: Darshit Chanpura <[email protected]>

opensearch-trigger-bot · 2023-08-24T15:38:26Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-3231-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 683c4f564e89583d9e98bc2fc4eaaf62b630e2f1
# Push it to GitHub
git push --set-upstream origin backport/backport-3231-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3231-to-2.x.

…pensearch-project#3231) A lot of files (mostly tests) have sysout's which do nothing but degrade the performance. This PR removes all sysouts from tests, replace sysouts with logger in project files. Also removes all e.printStacktrace(); calls. - `tools/` package - no sysouts were removed from here - HTTPSpnegoAuthenticator.java - a portion was not modified since it enabled debugging on console. Signed-off-by: Darshit Chanpura <[email protected]> (cherry picked from commit 683c4f5)

…ere necessary (#3231) (#3240) Backports #3231. Needed manual backport because of http5 imports and server `shutdown` (2.x) vs `awaitTermination` (main) method calls in test. Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura requested review from cliu123, cwperks, davidlago, peternied, RyanL1997, stephen-crawford, reta and willyborankin as code owners August 22, 2023 15:55

DarshitChanpura changed the title ~~Cleans up sysouts and replaces with logger statements where necessary~~ Clean up sysouts and replace with logger statements where necessary Aug 22, 2023

Cleans up sysouts and replaces with logger statements where necessary

9ff69ae

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura force-pushed the cleaning-sysouts branch from 6a8d0cf to 9ff69ae Compare August 22, 2023 16:00

peternied reviewed Aug 22, 2023

View reviewed changes

src/main/java/org/opensearch/security/tools/AuditConfigMigrater.java Outdated Show resolved Hide resolved

peternied reviewed Aug 22, 2023

View reviewed changes

src/test/java/org/opensearch/security/ConfigTests.java Outdated Show resolved Hide resolved

peternied reviewed Aug 22, 2023

View reviewed changes

Reverts changes in AuditMigrater and Migrater

76b869f

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura force-pushed the cleaning-sysouts branch from c8d211b to a25a993 Compare August 22, 2023 19:27

peternied reviewed Aug 22, 2023

View reviewed changes

Adds workflow check for println statement

770fa09

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura force-pushed the cleaning-sysouts branch from a25a993 to 770fa09 Compare August 22, 2023 19:30

Fixes broken SSL test

416150d

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura added 2 commits August 22, 2023 18:27

Removes extra new lines and does code cleanup

1bc83fc

Signed-off-by: Darshit Chanpura <[email protected]>

Adds println as checkstyle check

17d83a1

Signed-off-by: Darshit Chanpura <[email protected]>

willyborankin approved these changes Aug 23, 2023

View reviewed changes

DarshitChanpura self-assigned this Aug 23, 2023

peternied approved these changes Aug 23, 2023

View reviewed changes

DarshitChanpura merged commit 683c4f5 into opensearch-project:main Aug 23, 2023

DarshitChanpura added the backport 2.x backport to 2.x branch label Aug 24, 2023

opensearch-trigger-bot bot added the backport-failed label Aug 24, 2023

DarshitChanpura mentioned this pull request Aug 25, 2023

[Backport 2.x] Clean up sysouts and replace with logger statements where necessary (#3231) #3240

Merged

1 task

peternied changed the title ~~Clean up sysouts and replace with logger statements where necessary~~ Send log messages to log4j systems instead of system out / error Aug 31, 2023

peternied removed the backport-failed label Aug 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send log messages to log4j systems instead of system out / error #3231

Send log messages to log4j systems instead of system out / error #3231

DarshitChanpura commented Aug 22, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading

DarshitChanpura commented Aug 22, 2023

peternied left a comment

DarshitChanpura commented Aug 22, 2023

peternied Aug 22, 2023

DarshitChanpura Aug 22, 2023

DarshitChanpura Aug 22, 2023 •

edited

Loading

peternied Aug 22, 2023

DarshitChanpura Aug 22, 2023 •

edited

Loading

DarshitChanpura Aug 23, 2023

peternied Aug 23, 2023 •

edited

Loading

DarshitChanpura Aug 23, 2023

DarshitChanpura commented Aug 22, 2023

opensearch-trigger-bot bot commented Aug 24, 2023

Send log messages to log4j systems instead of system out / error #3231

Send log messages to log4j systems instead of system out / error #3231

Conversation

DarshitChanpura commented Aug 22, 2023 • edited Loading

Description

Check List

codecov bot commented Aug 22, 2023 • edited Loading

Codecov Report

DarshitChanpura commented Aug 22, 2023

peternied left a comment

Choose a reason for hiding this comment

DarshitChanpura commented Aug 22, 2023

peternied Aug 22, 2023

Choose a reason for hiding this comment

DarshitChanpura Aug 22, 2023

Choose a reason for hiding this comment

DarshitChanpura Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

peternied Aug 22, 2023

Choose a reason for hiding this comment

DarshitChanpura Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

DarshitChanpura Aug 23, 2023

Choose a reason for hiding this comment

peternied Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

DarshitChanpura Aug 23, 2023

Choose a reason for hiding this comment

DarshitChanpura commented Aug 22, 2023

opensearch-trigger-bot bot commented Aug 24, 2023

DarshitChanpura commented Aug 22, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading

DarshitChanpura Aug 22, 2023 •

edited

Loading

DarshitChanpura Aug 22, 2023 •

edited

Loading

peternied Aug 23, 2023 •

edited

Loading