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

services - being able to externalize the logging configurations outside of the webapp #5826

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pmauduit
Copy link
Contributor

@pmauduit pmauduit commented Jul 9, 2021

With the suggested modifications, we can define a bean as a string in any XML configuration files, e.g.:

  <bean id="loggingConfigurationPath" class="java.lang.String">
    <constructor-arg value="/etc/geonetwork/log4j"/>
  </bean>

Then having a /etc/geonetwork/log4j/ directory with some log4j configurations like the default ones from the WEB-INF/classes
subdirectory, we can externalize from the webapp the logging configuration.

Not having the previous bean defined will fall back onto the current behaviour (e.g. using the files from the classpath).

the main advantage of this approach is that we can customize the logging configuration without having to recompile GeoNetwork. We can even add new configurations without atering the webapp content.

Tests:

  • runtime tested in a docker composition (adding the bean into WEB-INF/config-spring-geonetwork.xml)
  • mvn clean test from the service module OK. I'm wondering if it would deserve an extra integration tests actually.

With the suggested modifications, we can define a bean as a string in
any XML configuration files, e.g.:

```
  <bean id="loggingConfigurationPath" class="java.lang.String">
    <constructor-arg value="/etc/geonetwork/log4j"/>
  </bean>
```

Then having a `/etc/geonetwork/log4j/` directory with some log4j
configurations like the default ones from the WEB-INF/classes
subdirectory, we can externalize from the webapp the logging
configuration.

Not having the previous bean defined will fall back onto the current
behaviour (e.g. using the files from the classpath).

the main advantage of this approach is that we can customize the logging
configuration without having to recompile GeoNetwork. We can even add
new configurations without atering the webapp content.
@josegar74
Copy link
Member

Is this not similar to #4473?

@pmauduit
Copy link
Contributor Author

Is this not similar to #4473?

No, on the mentionned PR, the log_dir allows to define where the actual log files will be written, my PR is about to define a place for the logging configurations. The main motivation is that sometimes one want to be able to tweak the log4j configuration (e.g. using a consoleAppender in case of a docker setup, being able to redirect to a network socket for logstash consumption, ...) without having to modify them inside the webapp (as modifying the webapp generally means the need of a custom build).

@juanluisrp
Copy link
Contributor

Why don't use a property/environment variable instead of a bean? That would avoid to have to add a custom config-spring-geonetwork.xml file.

@pmauduit
Copy link
Contributor Author

Why don't use a property/environment variable instead of a bean? That would avoid to have to add a custom config-spring-geonetwork.xml file.

Good point: because in the georchestra build, we already have a xml bean configuration file for adding custom stuff (e.g. loading a properties file situated outside of the webapp, in what we call the "georchestra datadir"). But here it will be more relevant to be able to override it via a property/env variable. I'll update my PR.

@pmauduit
Copy link
Contributor Author

@juanluisrp Ok, I updated the PR to take into account your remark.

Grepping the code for System.getenv vs System.getProperty it sounds to me that a property should be more appropriate (there was already a geonetwork.file.encoding for instance). But I was wondering if there was any place where I could document it.

Taking into account comments from @juanluisrp
Can be used launching the jvm with `-Dgeonetwork.log.configdir=....`
@pmauduit pmauduit force-pushed the log4j-configuration-outside-the-webapp branch from d6a9ee1 to b147ea5 Compare August 19, 2021 09:40
@pmauduit
Copy link
Contributor Author

force-pushed because I forgot to optimize imports before pushing

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

4 participants