gemmellr commented on code in PR #5136: URL: https://github.com/apache/activemq-artemis/pull/5136#discussion_r1709270218
########## docs/user-manual/logging.adoc: ########## @@ -3,7 +3,7 @@ :idseparator: - Apache ActiveMQ Artemis uses the https://www.slf4j.org/[SLF4J] logging facade for logging, with the broker assembly providing https://logging.apache.org/log4j/2.x/manual/[Log4J 2] as the logging implementation. -This is configurable via the `log4j2.properties` file found in the broker instance `etc` directory, which is configured by default to log to both the console and to a file. +When the broker is started by executing the `run` command, this is configurable via the `log4j2.properties` file found in the broker instance `etc` directory, which is configured by default to log to both the console and to a file. For the other CLI commands, this is configurable via the `log4j2-utility.properties` file found in the broker instance `etc` directory, which is configured by default to log only errors to the console. Review Comment: ```suggestion When the broker is started by executing the `run` command, this is configurable via the `log4j2.properties` file found in the broker instance `etc` directory, which is configured by default to log to both the console and to a file. For the other CLI commands, this is configurable via the `log4j2-utility.properties` file found in the broker instance `etc` directory, which is configured by default to log only errors to the console (in addition to the usual command output). ``` ########## docs/user-manual/versions.adoc: ########## @@ -309,7 +324,7 @@ Most existing customisations to the old configuration files and scripts will be As such you should compare the old configuration files with the refreshed ones and then port any missing customisations you may have made as necessary. The upgrade command itself will copy the older files it changes to an `old-config-bkp.` folder within the instance directory. -Similarly, if you had customised the old `logging.properties` file you may need to prepare analogous changes for the new `log4j2.properties` file. +Similarly, if you had customised the old `logging.properties` file you may need to prepare analogous changes for the new `log4j2.properties` and `log4j2-utility.properties` files. Review Comment: Not sure if this addition makes sense, as this section is about 2.27.0 which wouldnt have that file. ########## artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/Upgrade.java: ########## @@ -154,6 +154,17 @@ public Object run(ActionContext context) throws Exception { write("etc/" + Create.ETC_ARTEMIS_PROFILE_CMD, artemisProfileCmdTmp, filters, false, false); upgradeJDK(context, JDK_PREFIX_WINDOWS, "", KEEPING_JVM_ARGUMENTS, artemisProfileCmdTmp, artemisProfileCmd, artemisProfileCmdBkp, "set ARTEMIS_INSTANCE=\"", "set ARTEMIS_DATA_DIR=", "set ARTEMIS_ETC_DIR=", "set ARTEMIS_OOME_DUMP=", "set ARTEMIS_INSTANCE_URI=", "set ARTEMIS_INSTANCE_ETC_URI="); + + File artemisUtilityProfileCmd = new File(etcFolder, Create.ETC_ARTEMIS_UTILITY_PROFILE); + File artemisUtilityProfileCmdTmp = new File(tmp, Create.ETC_ARTEMIS_UTILITY_PROFILE); + File artemisUtilityProfileCmdBkp = new File(etcBkp, Create.ETC_ARTEMIS_UTILITY_PROFILE); + if (artemisUtilityProfileCmd.exists()) { Review Comment: Wrong constant being used, this is the Windows segment but these are all for the Linux file ########## artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/Upgrade.java: ########## @@ -348,6 +370,15 @@ private void upgradeLogging(ActionContext context, File etcFolder, File bkpFolde } } } + + File newDefaultLogging = new File(etcFolder, Create.ETC_LOG4J2_UTILITY_PROPERTIES); Review Comment: ```suggestion File newUtilityLogging = new File(etcFolder, Create.ETC_LOG4J2_UTILITY_PROPERTIES); ``` ########## docs/migration-guide/_configuration.adoc: ########## @@ -24,11 +24,13 @@ The main configuration file is `etc/broker.xml`. Similarly to ActiveMQ's `conf/activemq.xml`, this is where you configure most of the aspects of the broker, like connector ports, destination names, security policies, etc. We will go through this file in details in the following articles. -The `etc/artemis.profile` file is similar to the `bin/env` file in ActiveMQ. -Here you can configure environment variables for the broker, mostly regular JVM args related to SSL context, debugging, etc. +The `etc/artemis.profile` and `etc/artemis-utility.profile` files are similar to the `bin/env` file in ActiveMQ. +In the `etc/artemis.profile` you can configure environment variables for the broker started by executing the `run` command, mostly regular JVM args related to SSL context, debugging, etc. +In the `etc/artemis-utility.profile` file you can configure environment variables for all CLI commands other than run, mostly regular JVM args related to SSL context, debugging, etc. There's not much difference in logging configuration between two brokers, so anyone familiar with Java logging systems in general will find herself at home here. -The `etc/log4j-config.properties` file is where it's all configured. +The `etc/log4j.properties` file is where it's all configured for the broker. +The `etc/log4j-utility.properties` file is where it's all configured for all CLI commands other than run. Review Comment: file names are slightly wrong (original already was), should be log4j**2**.properties and log4j**2**-utility.properties ########## docs/user-manual/upgrading.adoc: ########## @@ -55,8 +55,8 @@ cd $NEW_ARTEMIS_DOWNLOAD/bin/ ./artemis upgrade PATH_TO_UPGRADING_INSTANCE ---- -The broker instance `bin/artemis` script and `etc/artemis.profile`(`artemis.cmd` and `artemis.cmd.profile` on Windows) will be updated to the new versions, setting its ARTEMIS_HOME to refer to the new broker version home path. -The tool will also create the new `<instance>/etc/log4j2.properties` configuration file if needed (e.g if you are migrating from a version prior to 2.27.0), and remove the old `<instance>/etc/logging.properties` file if present. +The broker instance scripts `bin/artemis`, `etc/artemis.profile` and `etc/artemis-utility.profile` (`artemis.cmd`, and `artemis.cmd.profile` on Windows) will be updated to the new versions, setting its ARTEMIS_HOME to refer to the new broker version home path. Review Comment: ```suggestion The broker instance script `bin/artemis` plus profiles `etc/artemis.profile` and `etc/artemis-utility.profile` (`artemis.cmd`, `artemis.cmd.profile`, and `artemis-utility.cmd.profile` on Windows) will be updated to the new versions, setting its ARTEMIS_HOME to refer to the new broker version home path. ``` ########## artemis-cli/src/main/resources/org/apache/activemq/artemis/cli/commands/etc/artemis-utility.profile: ########## @@ -0,0 +1,33 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +ARTEMIS_HOME='${artemis.home}' +ARTEMIS_INSTANCE='@artemis.instance@' +ARTEMIS_DATA_DIR='${artemis.instance.data}' +ARTEMIS_ETC_DIR='${artemis.instance.etc}' +ARTEMIS_OOME_DUMP='${artemis.instance.oome.dump}' + +# The logging config will need an URI +# this will be encoded in case you use spaces or special characters +# on your directory structure +ARTEMIS_INSTANCE_URI='${artemis.instance.uri}' +ARTEMIS_INSTANCE_ETC_URI='${artemis.instance.etc.uri}' + +# Java Opts +if [ -z "$JAVA_ARGS" ]; then + JAVA_ARGS="-Dlog4j2.configurationFile=${ARTEMIS_INSTANCE_ETC_URI}log4j2-utility.properties ${java-opts}" +fi Review Comment: The trouble with using JAVA_ARGS for this directly is, it then wont have any effect for anyone that is already setting JAVA_ARGS now, unless they notice this was added and adjust their JAVA_ARGS to include it. It also means anyone setting JAVA_ARGS later has to set this too, when they probably dont want to influence the logging. Between that, and people perhaps thinking they need to replace JAVA_ARGS to change this (if they dont know later definitions take priority and e.g use JAVA_ARGS_APPEND to set it 'again'), I think it would be worth considering having a specific variable for the logging config file selection to use (if set; the main profile wouldnt set it, this profile would) such that people could use it to override/set what the profile chooses to pass on to the main script. Much like the new profile picker variable. People could then still use JAVA_ARGS or JAVA_ARGS_APPEND to override it too if they want. (As example, part of the docs you updated in this PR, suggested people to set JAVA_ARGS for a command, but doesnt include setting this, so it wouldnt use this new logging config in that case) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected] For further information, visit: https://activemq.apache.org/contact
