Skip to content

Add option to provide URIs to monitor in addition to the config file #3501

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

Open
wants to merge 3 commits into
base: 2.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* 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.
*/
package org.apache.logging.log4j.core;

import static org.awaitility.Awaitility.waitAtMost;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Collections;
import java.util.concurrent.TimeUnit;
import org.apache.logging.log4j.core.config.Configuration;
import org.apache.logging.log4j.core.config.ConfigurationSource;
import org.apache.logging.log4j.core.config.Configurator;
import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilder;
import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilderFactory;
import org.apache.logging.log4j.core.config.properties.PropertiesConfiguration;
import org.apache.logging.log4j.core.util.Source;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.CleanupMode;
import org.junit.jupiter.api.io.TempDir;

public class MonitorUriTest {

private static final int MONITOR_INTERVAL = 3;

@Test
void testReconfigureOnChangeInMonitorUri(@TempDir(cleanup = CleanupMode.ON_SUCCESS) final Path tempDir)
throws IOException {
ConfigurationBuilder<PropertiesConfiguration> configBuilder =
ConfigurationBuilderFactory.newConfigurationBuilder(PropertiesConfiguration.class);
Path config = tempDir.resolve("config.xml");
Path monitorUri = tempDir.resolve("monitorUri.xml");
ConfigurationSource configSource = new ConfigurationSource(new Source(config), new byte[] {}, 0);
Configuration configuration = configBuilder
.setConfigurationSource(configSource)
.setMonitorInterval(String.valueOf(MONITOR_INTERVAL))
.add(configBuilder.newMonitorUri(monitorUri.toUri().toString()))
.build();

try (LoggerContext loggerContext = Configurator.initialize(configuration)) {
Files.write(monitorUri, Collections.singletonList("a change"));
waitAtMost(MONITOR_INTERVAL + 2, TimeUnit.SECONDS)
.until(() -> loggerContext.getConfiguration() != configuration);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.io.InputStream;
import java.lang.ref.WeakReference;
import java.net.URI;
import java.net.URISyntaxException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
Expand Down Expand Up @@ -132,6 +133,7 @@ public abstract class AbstractConfiguration extends AbstractFilterable implement
private ConcurrentMap<String, Appender> appenders = new ConcurrentHashMap<>();
private ConcurrentMap<String, LoggerConfig> loggerConfigs = new ConcurrentHashMap<>();
private List<CustomLevelConfig> customLevels = Collections.emptyList();
private List<URI> uris = Collections.emptyList();
private final ConcurrentMap<String, String> propertyMap = new ConcurrentHashMap<>();
private final Interpolator tempLookup = new Interpolator(propertyMap);
private final StrSubstitutor runtimeStrSubstitutor = new RuntimeStrSubstitutor(tempLookup);
Expand Down Expand Up @@ -323,10 +325,19 @@ public void start() {
LOGGER.info("Starting configuration {}...", this);
this.setStarting();
if (watchManager.getIntervalSeconds() >= 0) {
LOGGER.info(
"Start watching for changes to {} every {} seconds",
getConfigurationSource(),
watchManager.getIntervalSeconds());
if (uris != null && uris.size() > 0) {
LOGGER.info(
"Start watching for changes to {} and {} every {} seconds",
getConfigurationSource(),
uris,
watchManager.getIntervalSeconds());
watchMonitorUris();
} else {
LOGGER.info(
"Start watching for changes to {} every {} seconds",
getConfigurationSource(),
watchManager.getIntervalSeconds());
}
Comment on lines +328 to +340
Copy link
Member

Choose a reason for hiding this comment

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

Can't we simplify this as follows? (Can uris be null at all?)

Suggested change
if (uris != null && uris.size() > 0) {
LOGGER.info(
"Start watching for changes to {} and {} every {} seconds",
getConfigurationSource(),
uris,
watchManager.getIntervalSeconds());
watchMonitorUris();
} else {
LOGGER.info(
"Start watching for changes to {} every {} seconds",
getConfigurationSource(),
watchManager.getIntervalSeconds());
}
LOGGER.info(
"Start watching for changes to {} and {} every {} seconds",
getConfigurationSource(),
uris,
watchManager.getIntervalSeconds());
watchMonitorUris();

Copy link
Member

Choose a reason for hiding this comment

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

... and, this is the wrong place and time to do admit monitor URIs to the WatchManager. Would you move this admission operation (i.e., the logic in watchMonitorUris) to initializeWatchers, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @vy
The reason I did not put it in initializeWatchers() is because initializeWatchers() is called before the elements are read. The top level attributes (including monitorInterval) have been read at this stage and can therefore be used, but the elements (including monitorUris) have not been.

Would it be ok to move invoking watchMonitorUris() to intialization() which is earlier than here, but after the elements have been read? This would avoid having to read the monitorUri element earlier and separately from the other elements.

I can move it to initializeWatchers() either if you still prefer that, and add code to read the monitorUri element separately to the other elements, but I think the above would be cleaner in the code.

watchManager.start();
}
if (hasAsyncLoggers()) {
Expand All @@ -347,6 +358,17 @@ public void start() {
LOGGER.info("Configuration {} started.", this);
}

private void watchMonitorUris() {
if (this instanceof Reconfigurable) {
uris.stream().forEach(uri -> {
Source source = new Source(uri);
final ConfigurationFileWatcher watcher = new ConfigurationFileWatcher(
this, (Reconfigurable) this, listeners, source.getFile().lastModified());
watchManager.watch(source, watcher);
});
}
}

private boolean hasAsyncLoggers() {
if (root instanceof AsyncLoggerConfig) {
return true;
Expand Down Expand Up @@ -729,9 +751,16 @@ protected void doConfigure() {
} else if (child.isInstanceOf(AsyncWaitStrategyFactoryConfig.class)) {
final AsyncWaitStrategyFactoryConfig awsfc = child.getObject(AsyncWaitStrategyFactoryConfig.class);
asyncWaitStrategyFactory = awsfc.createWaitStrategyFactory();
} else if (child.isInstanceOf(MonitorUris.class)) {
uris = convertToJavaNetUris(child.getObject(MonitorUris.class).getUris());
} else {
final List<String> expected = Arrays.asList(
"\"Appenders\"", "\"Loggers\"", "\"Properties\"", "\"Scripts\"", "\"CustomLevels\"");
"\"Appenders\"",
"\"Loggers\"",
"\"Properties\"",
"\"Scripts\"",
"\"CustomLevels\"",
"\"MonitorUris\"");
LOGGER.error(
"Unknown object \"{}\" of type {} is ignored: try nesting it inside one of: {}.",
child.getName(),
Expand Down Expand Up @@ -767,6 +796,18 @@ protected void doConfigure() {
setParents();
}

private List<URI> convertToJavaNetUris(final List<Uri> uris) {
final List<URI> javaNetUris = new ArrayList<>();
for (Uri uri : uris) {
try {
javaNetUris.add(new URI(uri.getUri()));
} catch (URISyntaxException e) {
LOGGER.error("Error parsing monitor URI: " + uri, e);
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we want to proceed with (re)configuration obtained from an invalid configuration? I think we should propagate the exception and let it fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking here was to be consistent with other error scenarios, for example lines 726, 764, 790.
Also to let the exception propagate we would need to declare it on the doConfigure() method which could cause an impact on anyone with their own custom implementation which extends AbstractConfiguration

Copy link
Member

Choose a reason for hiding this comment

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

See this recent discussion on how to handle configuration failures. In short, configuration failures should result in the entire configuration to fail, instead of applying in partially. Existing code that doesn't adhere to this needs to be fixed, but that is another issue.

}
}
return javaNetUris;
}

public static Level getDefaultLevel() {
final String levelName = PropertiesUtil.getProperties()
.getStringProperty(DefaultConfiguration.DEFAULT_LEVEL, Level.ERROR.name());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* 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.
*/
package org.apache.logging.log4j.core.config;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import org.apache.logging.log4j.core.Core;
import org.apache.logging.log4j.core.config.plugins.Plugin;
import org.apache.logging.log4j.core.config.plugins.PluginElement;
import org.apache.logging.log4j.core.config.plugins.PluginFactory;

/**
* Container for MonitorUri objects.
*/
@Plugin(name = "MonitorUris", category = Core.CATEGORY_NAME, printObject = true)
public final class MonitorUris {

private final List<Uri> uris;

private MonitorUris(final Uri[] uris) {
this.uris = new ArrayList<>(Arrays.asList(uris));
}

/**
* Create a MonitorUris object to contain all the URIs to be monitored.
*
* @param uris An array of URIs.
* @return A MonitorUris object.
*/
@PluginFactory
public static MonitorUris createMonitorUris( //
@PluginElement("Uris") final Uri[] uris) {
return new MonitorUris(uris == null ? Uri.EMPTY_ARRAY : uris);
}

/**
* Returns a list of the {@code Uri} objects created during configuration.
* @return the URIs to be monitored
*/
public List<Uri> getUris() {
return uris;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
* 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.
*/
package org.apache.logging.log4j.core.config;

import java.util.Objects;
import org.apache.logging.log4j.core.Core;
import org.apache.logging.log4j.core.config.plugins.Plugin;
import org.apache.logging.log4j.core.config.plugins.PluginFactory;
import org.apache.logging.log4j.core.config.plugins.PluginValue;
import org.apache.logging.log4j.status.StatusLogger;

/**
* Descriptor of a URI object that is created via configuration.
*/
@Plugin(name = "Uri", category = Core.CATEGORY_NAME, printObject = true)
public final class Uri {

/**
* The empty array.
*/
static final Uri[] EMPTY_ARRAY = {};

private final String uri;

private Uri(final String uri) {
this.uri = Objects.requireNonNull(uri, "uri is null");
}

/**
* Creates a Uri object.
*
* @param uri the URI.
* @return A Uri object.
*/
@PluginFactory
public static Uri createUri( // @formatter:off
@PluginValue("uri") final String uri) {
// @formatter:on

StatusLogger.getLogger().debug("Creating Uri('{}')", uri);
return new Uri(uri);
}

/**
* Returns the URI.
*
* @return the URI
*/
public String getUri() {
return uri;
}

@Override
public int hashCode() {
return uri.hashCode();
}

@Override
public boolean equals(final Object object) {
if (this == object) {
return true;
}
if (!(object instanceof Uri)) {
return false;
}
final Uri other = (Uri) object;
return this.uri == other.uri;
}

@Override
public String toString() {
return "Uri[" + uri + "]";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,15 @@ public interface ConfigurationBuilder<T extends Configuration> extends Builder<T
*/
ConfigurationBuilder<T> add(CustomLevelComponentBuilder builder);

/**
* Adds a MonitorUri component.
* @param builder The MonitorUriComponentBuilder with all of its attributes set.
* @return this builder instance.
*/
default ConfigurationBuilder<T> add(MonitorUriComponentBuilder builder) {
return this;
}

/**
* Adds a Filter component.
* @param builder the FilterComponentBuilder with all of its attributes and sub components set.
Expand Down Expand Up @@ -272,6 +281,15 @@ public interface ConfigurationBuilder<T extends Configuration> extends Builder<T
*/
CustomLevelComponentBuilder newCustomLevel(String name, int level);

/**
* Returns a builder for creating MonitorUris
* @param uri The URI.
* @return A new MonitorUriComponentBuilder.
*/
default MonitorUriComponentBuilder newMonitorUri(String uri) {
return null;
}

/**
* Returns a builder for creating Filters.
* @param pluginName The Plugin type of the Filter.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* 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.
*/
package org.apache.logging.log4j.core.config.builder.api;

/**
* Assembler for constructing MonitorUri Components.
*/
public interface MonitorUriComponentBuilder extends ComponentBuilder<MonitorUriComponentBuilder> {}
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
* @since 2.4
*/
@Export
@Version("2.20.1")
@Version("2.25.0")
package org.apache.logging.log4j.core.config.builder.api;

import org.osgi.annotation.bundle.Export;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public class BuiltConfiguration extends AbstractConfiguration {
private Component propertiesComponent;
private Component customLevelsComponent;
private Component scriptsComponent;
private Component monitorUriComponent;
private String contentType = "text";

public BuiltConfiguration(
Expand Down Expand Up @@ -78,6 +79,10 @@ public BuiltConfiguration(
customLevelsComponent = component;
break;
}
case "MonitorUris": {
monitorUriComponent = component;
break;
}
}
}
this.rootComponent = rootComponent;
Expand All @@ -95,6 +100,9 @@ public void setup() {
if (customLevelsComponent.getComponents().size() > 0) {
children.add(convertToNode(rootNode, customLevelsComponent));
}
if (monitorUriComponent.getComponents().size() > 0) {
children.add(convertToNode(rootNode, monitorUriComponent));
}
children.add(convertToNode(rootNode, loggersComponent));
children.add(convertToNode(rootNode, appendersComponent));
if (filtersComponent.getComponents().size() > 0) {
Expand Down
Loading