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

refactor(apollo logging): Simplify the default log path to /opt/logs #4833

Merged
merged 97 commits into from
Apr 8, 2023

Conversation

klboke
Copy link
Contributor

@klboke klboke commented Apr 4, 2023

What's the purpose of this PR

refactor(apollo logging): Simplify the default log path to /opt/logs

motivation

The default log path for all Apollo services is prefixed with 100003172, which seems to have no additional purpose but hinders quick locating of log files. It's time to remove it.

klboke and others added 30 commits May 16, 2019 11:07
@klboke klboke requested a review from nobodyiam April 4, 2023 03:11
@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #4833 (0892fa0) into master (3a6ff67) will increase coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #4833      +/-   ##
============================================
+ Coverage     48.38%   48.40%   +0.01%     
- Complexity     1722     1724       +2     
============================================
  Files           346      346              
  Lines         10827    10827              
  Branches       1078     1078              
============================================
+ Hits           5239     5241       +2     
+ Misses         5266     5264       -2     
  Partials        322      322              

see 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

shoothzj
shoothzj previously approved these changes Apr 4, 2023
@nobodyiam
Copy link
Member

There is an issue that the gc.log would conflict if they are running in one machine.

if [[ "$javaexe" ]]; then
version=$("$javaexe" -version 2>&1 | awk -F '"' '/version/ {print $2}')
version=$(echo "$version" | awk -F. '{printf("%03d%03d",$1,$2);}')
# now version is of format 009003 (9.3.x)
if [ $version -ge 011000 ]; then
JAVA_OPTS="$JAVA_OPTS -Xlog:gc*:$LOG_DIR/gc.log:time,level,tags -Xlog:safepoint -Xlog:gc+heap=trace"
elif [ $version -ge 010000 ]; then
JAVA_OPTS="$JAVA_OPTS -Xlog:gc*:$LOG_DIR/gc.log:time,level,tags -Xlog:safepoint -Xlog:gc+heap=trace"
elif [ $version -ge 009000 ]; then
JAVA_OPTS="$JAVA_OPTS -Xlog:gc*:$LOG_DIR/gc.log:time,level,tags -Xlog:safepoint -Xlog:gc+heap=trace"
else
JAVA_OPTS="$JAVA_OPTS -XX:+UseParNewGC"
JAVA_OPTS="$JAVA_OPTS -Xloggc:$LOG_DIR/gc.log -XX:+PrintGCDetails"
JAVA_OPTS="$JAVA_OPTS -XX:+UseConcMarkSweepGC -XX:+UseCMSCompactAtFullCollection -XX:+UseCMSInitiatingOccupancyOnly -XX:CMSInitiatingOccupancyFraction=60 -XX:+CMSClassUnloadingEnabled -XX:+CMSParallelRemarkEnabled -XX:CMSFullGCsBeforeCompaction=9 -XX:+CMSClassUnloadingEnabled -XX:+PrintGCDateStamps -XX:+PrintGCApplicationConcurrentTime -XX:+PrintHeapAtGC -XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles=5 -XX:GCLogFileSize=5M"
fi
fi

@shoothzj
Copy link
Member

shoothzj commented Apr 5, 2023

As I think, we can use a more reasonable name, like /opt/apollo/logs, not magic number

@klboke
Copy link
Contributor Author

klboke commented Apr 6, 2023

@nobodyiam

There is an issue that the gc.log would conflict if they are running in one machine.

Adding prefix of ServiceName to gc.log path

@klboke klboke requested a review from nobodyiam April 7, 2023 03:39
Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

@nobodyiam nobodyiam merged commit b30cbe5 into apolloconfig:master Apr 8, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Apr 8, 2023
@nobodyiam nobodyiam added this to the 2.2.0 milestone Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants