You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "ppkarwasz (via GitHub)" <gi...@apache.org> on 2024/03/28 12:44:00 UTC
[PR] Add logging to `PropertiesUtil` and fix `Duration` parser (logging-log4j2)
ppkarwasz opened a new pull request, #2424:
URL: https://github.com/apache/logging-log4j2/pull/2424
Profiting from the `StatusLogger` enhancements in #2273, we add logging statement in `PropertiesUtil`.
We also add tests for the `getDurationProperty` method and fix its behavior.
Part of #2290.
Closes #1969
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add logging to `PropertiesUtil` and fix `Duration` parser (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #2424:
URL: https://github.com/apache/logging-log4j2/pull/2424#discussion_r1543618080
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java:
##########
@@ -649,45 +674,57 @@ public static Map<String, Properties> partitionOnCommonPrefixes(
* @return true if system properties tell us we are running on Windows.
*/
public boolean isOsWindows() {
- return getStringProperty("os.name", "").startsWith("Windows");
+ return SystemPropertiesPropertySource.getSystemProperty("os.name", "").startsWith("Windows");
+ }
+
+ static Duration parseDuration(final String value) {
+ final Matcher matcher = DURATION_PATTERN.matcher(value);
+ if (matcher.matches()) {
+ return Duration.of(TimeUnit.parseAmount(matcher), TimeUnit.parseUnit(matcher));
+ }
+ throw new DateTimeParseException("Invalid duration value: should be in the format nnn[unit].", value, 0);
}
private enum TimeUnit {
- NANOS("ns,nano,nanos,nanosecond,nanoseconds", ChronoUnit.NANOS),
- MICROS("us,micro,micros,microsecond,microseconds", ChronoUnit.MICROS),
- MILLIS("ms,milli,millis,millsecond,milliseconds", ChronoUnit.MILLIS),
- SECONDS("s,second,seconds", ChronoUnit.SECONDS),
- MINUTES("m,minute,minutes", ChronoUnit.MINUTES),
- HOURS("h,hour,hours", ChronoUnit.HOURS),
- DAYS("d,day,days", ChronoUnit.DAYS);
-
- /*
- * https://errorprone.info/bugpattern/ImmutableEnumChecker
- * This field is effectively immutable.
- */
- @SuppressWarnings("ImmutableEnumChecker")
- private final String[] descriptions;
+ NANOS(new String[] {"ns", "nano", "nanos", "nanosecond", "nanoseconds"}, ChronoUnit.NANOS),
+ MICROS(new String[] {"us", "micro", "micros", "microsecond", "microseconds"}, ChronoUnit.MICROS),
+ MILLIS(new String[] {"ms", "milli", "millis", "millisecond", "milliseconds"}, ChronoUnit.MILLIS),
+ SECONDS(new String[] {"s", "second", "seconds"}, ChronoUnit.SECONDS),
+ MINUTES(new String[] {"m", "minute", "minutes"}, ChronoUnit.MINUTES),
+ HOURS(new String[] {"h", "hour", "hours"}, ChronoUnit.HOURS),
+ DAYS(new String[] {"d", "day", "days"}, ChronoUnit.DAYS);
- private final ChronoUnit timeUnit;
+ private final String[] descriptions;
+ private final TemporalUnit timeUnit;
- TimeUnit(final String descriptions, final ChronoUnit timeUnit) {
- this.descriptions = descriptions.split(",");
+ TimeUnit(final String[] descriptions, final TemporalUnit timeUnit) {
+ this.descriptions = descriptions;
this.timeUnit = timeUnit;
}
- static Duration getDuration(final String time) {
- final String value = time.trim();
- TemporalUnit temporalUnit = ChronoUnit.MILLIS;
- long timeVal = 0;
- for (TimeUnit timeUnit : values()) {
- for (String suffix : timeUnit.descriptions) {
- if (value.endsWith(suffix)) {
- temporalUnit = timeUnit.timeUnit;
- timeVal = Long.parseLong(value.substring(0, value.length() - suffix.length()));
+ private static long parseAmount(final MatchResult matcher) {
Review Comment:
This method has nothing to do with `TimeUnit`. I would rather place it right after `parseDuration` and rename it to `parseDurationAmount`.
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java:
##########
@@ -649,45 +674,57 @@ public static Map<String, Properties> partitionOnCommonPrefixes(
* @return true if system properties tell us we are running on Windows.
*/
public boolean isOsWindows() {
- return getStringProperty("os.name", "").startsWith("Windows");
+ return SystemPropertiesPropertySource.getSystemProperty("os.name", "").startsWith("Windows");
+ }
+
+ static Duration parseDuration(final String value) {
+ final Matcher matcher = DURATION_PATTERN.matcher(value);
+ if (matcher.matches()) {
+ return Duration.of(TimeUnit.parseAmount(matcher), TimeUnit.parseUnit(matcher));
+ }
+ throw new DateTimeParseException("Invalid duration value: should be in the format nnn[unit].", value, 0);
}
private enum TimeUnit {
- NANOS("ns,nano,nanos,nanosecond,nanoseconds", ChronoUnit.NANOS),
- MICROS("us,micro,micros,microsecond,microseconds", ChronoUnit.MICROS),
- MILLIS("ms,milli,millis,millsecond,milliseconds", ChronoUnit.MILLIS),
- SECONDS("s,second,seconds", ChronoUnit.SECONDS),
- MINUTES("m,minute,minutes", ChronoUnit.MINUTES),
- HOURS("h,hour,hours", ChronoUnit.HOURS),
- DAYS("d,day,days", ChronoUnit.DAYS);
-
- /*
- * https://errorprone.info/bugpattern/ImmutableEnumChecker
- * This field is effectively immutable.
- */
- @SuppressWarnings("ImmutableEnumChecker")
- private final String[] descriptions;
+ NANOS(new String[] {"ns", "nano", "nanos", "nanosecond", "nanoseconds"}, ChronoUnit.NANOS),
+ MICROS(new String[] {"us", "micro", "micros", "microsecond", "microseconds"}, ChronoUnit.MICROS),
+ MILLIS(new String[] {"ms", "milli", "millis", "millisecond", "milliseconds"}, ChronoUnit.MILLIS),
+ SECONDS(new String[] {"s", "second", "seconds"}, ChronoUnit.SECONDS),
+ MINUTES(new String[] {"m", "minute", "minutes"}, ChronoUnit.MINUTES),
+ HOURS(new String[] {"h", "hour", "hours"}, ChronoUnit.HOURS),
+ DAYS(new String[] {"d", "day", "days"}, ChronoUnit.DAYS);
- private final ChronoUnit timeUnit;
+ private final String[] descriptions;
+ private final TemporalUnit timeUnit;
- TimeUnit(final String descriptions, final ChronoUnit timeUnit) {
- this.descriptions = descriptions.split(",");
+ TimeUnit(final String[] descriptions, final TemporalUnit timeUnit) {
+ this.descriptions = descriptions;
this.timeUnit = timeUnit;
}
- static Duration getDuration(final String time) {
- final String value = time.trim();
- TemporalUnit temporalUnit = ChronoUnit.MILLIS;
- long timeVal = 0;
- for (TimeUnit timeUnit : values()) {
- for (String suffix : timeUnit.descriptions) {
- if (value.endsWith(suffix)) {
- temporalUnit = timeUnit.timeUnit;
- timeVal = Long.parseLong(value.substring(0, value.length() - suffix.length()));
+ private static long parseAmount(final MatchResult matcher) {
+ final String amount = matcher.group(1);
+ try {
+ return Long.parseLong(amount);
+ } catch (final NumberFormatException e) {
+ throw new DateTimeParseException(
+ "Invalid time amount '" + amount + "'", matcher.group(), matcher.start(1), e);
+ }
+ }
+
+ private static TemporalUnit parseUnit(final MatchResult matcher) {
Review Comment:
Why does `TimeUnit#parseUnit()` receive a `MatchResult` instead of a `String`?
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java:
##########
@@ -23,6 +23,7 @@
import java.io.InputStream;
import java.nio.charset.Charset;
import java.time.Duration;
+import java.time.format.DateTimeParseException;
Review Comment:
`DateTimeParseException` doesn't display any of its input arguments when printed. Could you instead use `IllegalArgumentException` with self-explanatory messages, please?
For instance, consider the following _example_:
```
final String message = String.format("provided duration `%s` doesn't match the expected `%s` pattern", input, pattern);
throw new IllegalArgumentException(message, cause);
```
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java:
##########
@@ -64,6 +68,8 @@ public final class PropertiesUtil {
private static final Lazy<PropertiesUtil> COMPONENT_PROPERTIES =
Lazy.lazy(() -> new PropertiesUtil(LOG4J_PROPERTIES_FILE_NAME, false));
+ private static final Pattern DURATION_PATTERN = Pattern.compile("([+-]?\\d+)\\s*(\\w+)?", Pattern.CASE_INSENSITIVE);
Review Comment:
*Nit:* I would rather dynamically build this pattern using the exact list of supported values from `TimeUnit`.
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java:
##########
@@ -649,45 +674,57 @@ public static Map<String, Properties> partitionOnCommonPrefixes(
* @return true if system properties tell us we are running on Windows.
*/
public boolean isOsWindows() {
- return getStringProperty("os.name", "").startsWith("Windows");
+ return SystemPropertiesPropertySource.getSystemProperty("os.name", "").startsWith("Windows");
+ }
+
+ static Duration parseDuration(final String value) {
+ final Matcher matcher = DURATION_PATTERN.matcher(value);
+ if (matcher.matches()) {
+ return Duration.of(TimeUnit.parseAmount(matcher), TimeUnit.parseUnit(matcher));
+ }
+ throw new DateTimeParseException("Invalid duration value: should be in the format nnn[unit].", value, 0);
Review Comment:
I don't think someone who is not able to correctly type a duration value can understand what is expected from them by looking at the hinted `nnn[unit]` format. Can we make the example/pattern a little bit more human-friendly?
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/SystemPropertiesPropertySource.java:
##########
@@ -32,19 +34,32 @@
@ServiceProvider(value = PropertySource.class, resolution = Resolution.OPTIONAL)
public class SystemPropertiesPropertySource implements PropertySource {
+ private static final Logger LOGGER = StatusLogger.getLogger();
private static final int DEFAULT_PRIORITY = 0;
private static final String PREFIX = "log4j2.";
+ private static final PropertySource INSTANCE = new SystemPropertiesPropertySource();
+
/**
- * Used by bootstrap code to get system properties without loading PropertiesUtil.
+ * Method used by Java 9+ to instantiate providers
+ * @since 2.24.0
+ * @see java.util.ServiceLoader
*/
+ public static PropertySource provider() {
+ return INSTANCE;
+ }
Review Comment:
1. What do these methods do?
2. What do they have to do with this PR?
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add logging to `PropertiesUtil` and fix `Duration` parser (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #2424:
URL: https://github.com/apache/logging-log4j2/pull/2424#discussion_r1544550920
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java:
##########
@@ -64,6 +68,8 @@ public final class PropertiesUtil {
private static final Lazy<PropertiesUtil> COMPONENT_PROPERTIES =
Lazy.lazy(() -> new PropertiesUtil(LOG4J_PROPERTIES_FILE_NAME, false));
+ private static final Pattern DURATION_PATTERN = Pattern.compile("([+-]?\\d+)\\s*(\\w+)?", Pattern.CASE_INSENSITIVE);
Review Comment:
I don't think such an optimization is necessary: the `getDurationProperty` method is only maintained for BC and is not used by any of our artifacts.
In Log4j Core I would prefer a standard `Duration.of` converter instead of this custom format.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add logging to `PropertiesUtil` and fix `Duration` parser (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #2424:
URL: https://github.com/apache/logging-log4j2/pull/2424#discussion_r1544546233
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java:
##########
@@ -649,45 +674,57 @@ public static Map<String, Properties> partitionOnCommonPrefixes(
* @return true if system properties tell us we are running on Windows.
*/
public boolean isOsWindows() {
- return getStringProperty("os.name", "").startsWith("Windows");
+ return SystemPropertiesPropertySource.getSystemProperty("os.name", "").startsWith("Windows");
+ }
+
+ static Duration parseDuration(final String value) {
+ final Matcher matcher = DURATION_PATTERN.matcher(value);
+ if (matcher.matches()) {
+ return Duration.of(TimeUnit.parseAmount(matcher), TimeUnit.parseUnit(matcher));
+ }
+ throw new DateTimeParseException("Invalid duration value: should be in the format nnn[unit].", value, 0);
}
private enum TimeUnit {
- NANOS("ns,nano,nanos,nanosecond,nanoseconds", ChronoUnit.NANOS),
- MICROS("us,micro,micros,microsecond,microseconds", ChronoUnit.MICROS),
- MILLIS("ms,milli,millis,millsecond,milliseconds", ChronoUnit.MILLIS),
- SECONDS("s,second,seconds", ChronoUnit.SECONDS),
- MINUTES("m,minute,minutes", ChronoUnit.MINUTES),
- HOURS("h,hour,hours", ChronoUnit.HOURS),
- DAYS("d,day,days", ChronoUnit.DAYS);
-
- /*
- * https://errorprone.info/bugpattern/ImmutableEnumChecker
- * This field is effectively immutable.
- */
- @SuppressWarnings("ImmutableEnumChecker")
- private final String[] descriptions;
+ NANOS(new String[] {"ns", "nano", "nanos", "nanosecond", "nanoseconds"}, ChronoUnit.NANOS),
+ MICROS(new String[] {"us", "micro", "micros", "microsecond", "microseconds"}, ChronoUnit.MICROS),
+ MILLIS(new String[] {"ms", "milli", "millis", "millisecond", "milliseconds"}, ChronoUnit.MILLIS),
+ SECONDS(new String[] {"s", "second", "seconds"}, ChronoUnit.SECONDS),
+ MINUTES(new String[] {"m", "minute", "minutes"}, ChronoUnit.MINUTES),
+ HOURS(new String[] {"h", "hour", "hours"}, ChronoUnit.HOURS),
+ DAYS(new String[] {"d", "day", "days"}, ChronoUnit.DAYS);
- private final ChronoUnit timeUnit;
+ private final String[] descriptions;
+ private final TemporalUnit timeUnit;
- TimeUnit(final String descriptions, final ChronoUnit timeUnit) {
- this.descriptions = descriptions.split(",");
+ TimeUnit(final String[] descriptions, final TemporalUnit timeUnit) {
+ this.descriptions = descriptions;
this.timeUnit = timeUnit;
}
- static Duration getDuration(final String time) {
- final String value = time.trim();
- TemporalUnit temporalUnit = ChronoUnit.MILLIS;
- long timeVal = 0;
- for (TimeUnit timeUnit : values()) {
- for (String suffix : timeUnit.descriptions) {
- if (value.endsWith(suffix)) {
- temporalUnit = timeUnit.timeUnit;
- timeVal = Long.parseLong(value.substring(0, value.length() - suffix.length()));
+ private static long parseAmount(final MatchResult matcher) {
+ final String amount = matcher.group(1);
+ try {
+ return Long.parseLong(amount);
+ } catch (final NumberFormatException e) {
+ throw new DateTimeParseException(
+ "Invalid time amount '" + amount + "'", matcher.group(), matcher.start(1), e);
+ }
+ }
+
+ private static TemporalUnit parseUnit(final MatchResult matcher) {
Review Comment:
`matcher` is used for error handling, but I can remove it.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add logging to `PropertiesUtil` and fix `Duration` parser (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz merged PR #2424:
URL: https://github.com/apache/logging-log4j2/pull/2424
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add logging to `PropertiesUtil` and fix `Duration` parser (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #2424:
URL: https://github.com/apache/logging-log4j2/pull/2424#discussion_r1543694204
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/SystemPropertiesPropertySource.java:
##########
@@ -32,19 +34,32 @@
@ServiceProvider(value = PropertySource.class, resolution = Resolution.OPTIONAL)
public class SystemPropertiesPropertySource implements PropertySource {
+ private static final Logger LOGGER = StatusLogger.getLogger();
private static final int DEFAULT_PRIORITY = 0;
private static final String PREFIX = "log4j2.";
+ private static final PropertySource INSTANCE = new SystemPropertiesPropertySource();
+
/**
- * Used by bootstrap code to get system properties without loading PropertiesUtil.
+ * Method used by Java 9+ to instantiate providers
+ * @since 2.24.0
+ * @see java.util.ServiceLoader
*/
+ public static PropertySource provider() {
+ return INSTANCE;
+ }
Review Comment:
> A provider method is a public static method named "provider" with no formal parameters and a return type that is assignable to the service's interface or class.
States [the Java 9 `ServiceLoader` javadoc](https://docs.oracle.com/javase%2F9%2Fdocs%2Fapi%2F%2F/java/util/ServiceLoader.html).
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org