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

Improvements to logging (#3262) #3267

Merged
merged 17 commits into from Apr 5, 2017

Conversation

Projects
None yet
4 participants
@ketan
Copy link
Member

commented Mar 15, 2017

No description provided.

@ketan ketan added the in progress label Mar 15, 2017

@ketan ketan force-pushed the ketan:improve/loggging branch 9 times, most recently from 297f733 to 452a8d8 Mar 15, 2017

@varshavaradarajan

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2017

  • stop-agent.sh - Change the LOG_FILE path to $AGENT_WORK_DIR/logs/agent-bootstrapper.log
  • the path for autoregister.properties, guid.txt etc is hardcoded to be config directory. Make the config directory configurable on the agent side as well. This will involve change agent.sh to include the system property cruise.config.dir as a startup argument

@ketan ketan added this to the Release 17.4 milestone Mar 31, 2017

@jyotisingh

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2017

Did not test it by bringing up an agent, but the code looks good to me..about the deletion of agent manifest file have you checked if the packaged agent MANIFEST file continues to include all the required info. This is what I see in the current version:

Manifest-Version: 1.0
Go-Version: 17.4.0-4712
Implementation-Title: agent
Target-Compatibility: 1.8
Implementation-Version: 17.4.0-4712
Source-Compatibility: 1.8
Go-Agent-Bootstrap-Class: com.thoughtworks.go.agent.AgentProcessParent
 Impl
Go-Revision: c943bb57d7d7d18c1d3cf9a9bd3385d56edbdd37
ImplementationVersion: 17.4.0-4712
Main-Class: com.simontuffs.onejar.Boot
One-Jar-Main-Class: com.thoughtworks.go.agent.AgentMain
@ketan

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2017

have you checked if the packaged agent MANIFEST file continues to include all the required info

All jars are packaged via the Jar gradle task, and MANIFEST.MF content defined in the task itself. None of them slurp in configuration from a MANIFEST.MF file.

@jyotisingh

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2017

Then I guess its all good.

@jyotisingh

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2017

the only thing is build hasn't run, so may be you would want to see that through, esp. the functional tests

@@ -39,8 +40,9 @@

public class DevelopmentServer {
public static void main(String[] args) throws Exception {
LogConfigurator logConfigurator = new LogConfigurator(DEFAULT_LOG4J_CONFIGURATION_FILE);
logConfigurator.initialize();

This comment has been minimized.

Copy link
@varshavaradarajan

varshavaradarajan Apr 3, 2017

Contributor

No need for this. LoggingInitializer will take care of it. The statement Could not find file 'config/log4j.properties'. Attempting to load from classpath. appears twice otherwise

This comment has been minimized.

Copy link
@ketan

ketan Apr 4, 2017

Author Member

This happens because the main program uses a different classloader than the webapp. So the logger is actually initialized twice. Side-effect of how logging is setup, unfortunately nothing (simple) that can be done about it.

fi

mkdir -p "${LOG_DIR}"

STDOUT_LOG_FILE=$LOG_DIR/${SERVICE_NAME}-bootstrapper.out.log

This comment has been minimized.

Copy link
@varshavaradarajan

varshavaradarajan Apr 3, 2017

Contributor

There is a LOG_FILE variable that is exported unnecessarily and stop-agent.sh also sets it unnecessarily. I did a git grep -w "LOG_FILE" in the project and found no usages in the code, except for JavaApplicationStub64. I think we can remove it.

@ketan ketan force-pushed the ketan:improve/loggging branch from 35d6765 to 30f2c13 Apr 4, 2017

@ketan
Copy link
Member Author

left a comment

@varshavaradarajan @jyotisingh — I think I'm done submitting additional changes to this PR. You probably want to see the last 5 commits —

  • 30f2c13 - Remove declaration of unwanted LOG_FILE environment variable. (6 minutes ago)
  • 45ff550 - Remove deprecated AGENT_TRUST_FILE constant. (6 minutes ago)
  • 4f6e8ef - Remove hard-coded config dir and use the one defined in SystemEnvironment instead (6 minutes ago)
  • 5aa840c - Ignore the log dir (6 minutes ago)
  • 35c8090 - Change log dir to logs (6 minutes ago)
@@ -39,8 +40,9 @@

public class DevelopmentServer {
public static void main(String[] args) throws Exception {
LogConfigurator logConfigurator = new LogConfigurator(DEFAULT_LOG4J_CONFIGURATION_FILE);
logConfigurator.initialize();

This comment has been minimized.

Copy link
@ketan

ketan Apr 4, 2017

Author Member

This happens because the main program uses a different classloader than the webapp. So the logger is actually initialized twice. Side-effect of how logging is setup, unfortunately nothing (simple) that can be done about it.

@@ -55,7 +55,6 @@ clean.doFirst {
delete 'agent-plugins.zip'
delete 'tfs-impl.jar'
delete 'go-agent.log'

This comment has been minimized.

Copy link
@varshavaradarajan

varshavaradarajan Apr 5, 2017

Contributor

Doesn't this have to be logs directory now?

@varshavaradarajan

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2017

@ketan ketan force-pushed the ketan:improve/loggging branch 2 times, most recently from af06677 to d84b7f3 Apr 5, 2017

ketan added some commits Mar 15, 2017

Introduce a `LogConfigurator` to configure logging
* this will look for a log file in the current working directory
* in case the file is not found, will look for the same file in the
  classpath
* in case of missing classpath resource, will fallback to configure
  logging to STDOUT at INFO level
Remove `Log4jDirectConfigurer` which was initializing too late
Spring was initializing the logger, which might have been too late in
case some component failed early.

ketan added some commits Mar 15, 2017

Remove deprecated `AGENT_TRUST_FILE` constant.
Cleanup the `AGENT_TRUST_FILE` in bootstrapper so it will eventually
happen on a bootstrapper upgrade. Avoid messing with that file in agent
process
Remove declaration of unwanted `LOG_FILE` environment variable.
Leave declaration in `JavaApplicationStub64` which seems to use it
locally.

@ketan ketan merged commit af91a90 into gocd:master Apr 5, 2017

5 of 6 checks passed

build-windows-PR/build-server
Details
build-linux-PR/build-non-server
Details
build-linux-PR/build-server
Details
build-windows-PR/build-non-server
Details
licence/cla Contributor License Agreement is signed.
Details
trigger/do-nothing
Details

@zabil zabil removed the in progress label Apr 5, 2017

@ketan ketan deleted the ketan:improve/loggging branch Apr 13, 2017

@jyotisingh jyotisingh referenced this pull request Apr 26, 2017

Closed

Improve logging #3262

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.