ppkarwasz commented on code in PR #2419: URL: https://github.com/apache/logging-log4j2/pull/2419#discussion_r1544402118
########## log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterizedMapMessage.java: ########## @@ -0,0 +1,38 @@ +/* + * 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.message; + +import java.util.Map; + +/** + * Class Description goes here. + */ +public class ParameterizedMapMessage extends StringMapMessage { + + private static final long serialVersionUID = -7724723101786525409L; + private final Message baseMessage; + + ParameterizedMapMessage(Message baseMessage, Map<String, String> resourceMap) { + super(resourceMap); + this.baseMessage = baseMessage; + } + + @Override + public String getFormattedMessage() { + return baseMessage.getFormattedMessage(); + } Review Comment: Currently, Open Telemetry uses `MapMessage#getFormat()` and `MapMessage#get("message")` to extract the body of a map message (cf. [`LogEventMapper`](https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/7b5b7584a0fb9e9b7920aa114a5ec979901329dd/instrumentation/log4j/log4j-appender-2.17/library/src/main/java/io/opentelemetry/instrumentation/log4j/appender/v2_17/internal/LogEventMapper.java#L120-L154)). Therefore I would override the `getFormat()` to return `baseMessage.getFormattedMessage()`. Regarding how `getFormattedMessage` should work, I am not sure what a user would expect. My guess is that it should be something like: ``` key1="value" key2="value" Formatted base message ``` i.e. the "FULL" message. Note that many (even third-party) layouts know about `MultiformatMessage`, so they don't call this method. **Remark**: could we create an interface (e.g. `StructuredMessage`, `AttributeMessage` or something similar) that codifies how the current sub-classes of `MapMessage` work? E.g.: ```java public interface AttributeMessage<V> extends Message { @Override default String getFormat() { Message message = getMessage(); return message != null ? message.getFormattedMessage() : ""; } @Override String getFormattedMessage(); @Nullable Message getMessage(); Map<String, V> getData(); } ``` Also it would be nice to add some well-known constants for `MultiformatMessage`. Integrators **do** use strings like `JSON` in practice, but the specification does not prevent the creation of message implementations that return some garbage instead of JSON. ########## log4j-api/src/main/java/org/apache/logging/log4j/ResourceLogger.java: ########## @@ -0,0 +1,276 @@ +/* + * 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; + +import java.util.Collections; +import java.util.Map; +import java.util.function.Supplier; +import org.apache.logging.log4j.message.Message; +import org.apache.logging.log4j.message.ParameterizedMapMessageFactory; +import org.apache.logging.log4j.spi.AbstractLogger; +import org.apache.logging.log4j.spi.ExtendedLogger; +import org.apache.logging.log4j.status.StatusLogger; +import org.apache.logging.log4j.util.StackLocatorUtil; +import org.apache.logging.log4j.util.Strings; + +/** + * Logger for resources. Formats all events using the ParameterizedMapMessageFactory along with the provided + * Supplier. The Supplier provides resource attributes that should be included in all log events generated + * from the current resource. Note that since the Supplier is called for every LogEvent being generated + * the values returned may change as necessary. Care should be taken to make the Supplier as efficient as + * possible to avoid performance issues. + * + * Unlike regular Loggers ResourceLoggers CANNOT be declared to be static. A ResourceLogger + * must be declared as a class member that will be garbage collected along with the instance of the resource. + */ +public final class ResourceLogger extends AbstractLogger { + private static final long serialVersionUID = -5837924138744974513L; + private final ExtendedLogger logger; + + public static ResourceLoggerBuilder newBuilder() { + return new ResourceLoggerBuilder(); + } + + /* + * Pass our MessageFactory with its Supplier to AbstractLogger. This will be used to create + * the Messages prior to them being passed to the "real" Logger. + */ + private ResourceLogger(final ExtendedLogger logger, final Supplier<Map<String, String>> supplier) { + super(logger.getName(), new ParameterizedMapMessageFactory(supplier)); + this.logger = logger; + } + + @Override + public Level getLevel() { + return logger.getLevel(); + } + + @Override + public boolean isEnabled(Level level, Marker marker, Message message, Throwable t) { + return logger.isEnabled(level, marker, message, t); + } + + @Override + public boolean isEnabled(Level level, Marker marker, CharSequence message, Throwable t) { + return logger.isEnabled(level, marker, message, t); + } + + @Override + public boolean isEnabled(Level level, Marker marker, Object message, Throwable t) { + return logger.isEnabled(level, marker, message, t); + } + + @Override + public boolean isEnabled(Level level, Marker marker, String message, Throwable t) { + return logger.isEnabled(level, marker, message, t); + } + + @Override + public boolean isEnabled(Level level, Marker marker, String message) { + return logger.isEnabled(level, marker, message); + } + + @Override + public boolean isEnabled(Level level, Marker marker, String message, Object... params) { + return logger.isEnabled(level, marker, message, params); + } + + @Override + public boolean isEnabled(Level level, Marker marker, String message, Object p0) { + return logger.isEnabled(level, marker, message, p0); + } + + @Override + public boolean isEnabled(Level level, Marker marker, String message, Object p0, Object p1) { + return logger.isEnabled(level, marker, message, p0, p1); + } + + @Override + public boolean isEnabled(Level level, Marker marker, String message, Object p0, Object p1, Object p2) { + return logger.isEnabled(level, marker, message, p0, p1, p2); + } + + @Override + public boolean isEnabled(Level level, Marker marker, String message, Object p0, Object p1, Object p2, Object p3) { + return logger.isEnabled(level, marker, message, p0, p1, p2, p3); + } + + @Override + public boolean isEnabled( + Level level, Marker marker, String message, Object p0, Object p1, Object p2, Object p3, Object p4) { + return logger.isEnabled(level, marker, message, p0, p1, p2, p3, p4); + } + + @Override + public boolean isEnabled( + Level level, + Marker marker, + String message, + Object p0, + Object p1, + Object p2, + Object p3, + Object p4, + Object p5) { + return logger.isEnabled(level, marker, message, p0, p1, p2, p3, p4, p5); + } + + @Override + public boolean isEnabled( + Level level, + Marker marker, + String message, + Object p0, + Object p1, + Object p2, + Object p3, + Object p4, + Object p5, + Object p6) { + return logger.isEnabled(level, marker, message, p0, p1, p2, p3, p4, p5, p6); + } + + @Override + public boolean isEnabled( + Level level, + Marker marker, + String message, + Object p0, + Object p1, + Object p2, + Object p3, + Object p4, + Object p5, + Object p6, + Object p7) { + return logger.isEnabled(level, marker, message, p0, p1, p2, p3, p4, p5, p6, p7); + } + + @Override + public boolean isEnabled( + Level level, + Marker marker, + String message, + Object p0, + Object p1, + Object p2, + Object p3, + Object p4, + Object p5, + Object p6, + Object p7, + Object p8) { + return logger.isEnabled(level, marker, message, p0, p1, p2, p3, p4, p5, p6, p7, p8); + } + + @Override + public boolean isEnabled( + Level level, + Marker marker, + String message, + Object p0, + Object p1, + Object p2, + Object p3, + Object p4, + Object p5, + Object p6, + Object p7, + Object p8, + Object p9) { + return logger.isEnabled(level, marker, message, p0, p1, p2, p3, p4, p5, p6, p7, p8, p9); + } + + @Override + public void logMessage(String fqcn, Level level, Marker marker, Message message, Throwable t) { + logger.logMessage(fqcn, level, marker, message, t); + } + + /** + * Constructs a ResourceLogger. + */ + public static final class ResourceLoggerBuilder { Review Comment: Personally I would prefer to just have a `ResourceLoggerBuilder` (or `LoggerBuilder`) **interface** and access it through a new `Logger#newDetachedLoggerBuilder` method. Creating "detached" loggers might be used for other purposes than adding key/value pairs to a message. For example someone might want to use a different `MessageFactory`. ########## log4j-api/src/main/java/org/apache/logging/log4j/ScopedContext.java: ########## @@ -0,0 +1,558 @@ +/* + * 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; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.Callable; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; +import java.util.function.Supplier; +import org.apache.logging.log4j.internal.ScopedContextAnchor; +import org.apache.logging.log4j.status.StatusLogger; + +/** + * Context that can be used for data to be logged in a block of code. + * + * While this is influenced by ScopedValues from Java 21 it does not share the same API. While it can perform a + * similar function as a set of ScopedValues it is really meant to allow a block of code to include a set of keys and + * values in all the log events within that block. The underlying implementation must provide support for + * logging the ScopedContext for that to happen. + * + * The ScopedContext will not be bound to the current thread until either a run or call method is invoked. The + * contexts are nested so creating and running or calling via a second ScopedContext will result in the first + * ScopedContext being hidden until the call is returned. Thus the values from the first ScopedContext need to + * be added to the second to be included. + * + * The ScopedContext can be passed to child threads by including the ExecutorService to be used to manage the + * run or call methods. The caller should interact with the ExecutorService as if they were submitting their + * run or call methods directly to it. The ScopedContext performs no error handling other than to ensure the + * ThreadContext and ScopedContext are cleaned up from the executed Thread. + * + * @since 2.24.0 + */ +public class ScopedContext { Review Comment: [This part seems independent from the `ResourceLogger`] While technically this class is very well written and could benefit some users, I am more and more convinced that we shouldn't offer this kind of context-propagation API. `ThreadLocal` was a big hit and allowed users to store and retrieve key/value pairs for logging purposes (and not only :wink:), but nowadays there are APIs specialized in this kind of things. Except the [`context-propagation`](https://github.com/micrometer-metrics/context-propagation) project I cited on the mailing-list, there is an entire [Context Propagation](https://opentelemetry.io/docs/concepts/context-propagation/) service from OpenTelemetry. If we were to offer users `ScopedContext`, I am afraid that sooner or later they would need to refactor it to use another API instead. The OTel [`Context`](https://javadoc.io/doc/io.opentelemetry/opentelemetry-context/latest/io/opentelemetry/context/Context.html) class offers many more wrappers than this class. In my opinion, what we should do instead is to help application and libraries that instrumented their code with `ThreadContext` to migrate to a specialized context-propagation API. For example we could submit to [`open-telemetry/opentelemetry-instrumentation-java`] a `ThreadContextMap` implementation that uses the [Bagage API](https://opentelemetry.io/docs/specs/otel/baggage/api/) ########## log4j-api/src/main/java/org/apache/logging/log4j/ResourceLogger.java: ########## @@ -0,0 +1,276 @@ +/* + * 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; + +import java.util.Collections; +import java.util.Map; +import java.util.function.Supplier; +import org.apache.logging.log4j.message.Message; +import org.apache.logging.log4j.message.ParameterizedMapMessageFactory; +import org.apache.logging.log4j.spi.AbstractLogger; +import org.apache.logging.log4j.spi.ExtendedLogger; +import org.apache.logging.log4j.status.StatusLogger; +import org.apache.logging.log4j.util.StackLocatorUtil; +import org.apache.logging.log4j.util.Strings; + +/** + * Logger for resources. Formats all events using the ParameterizedMapMessageFactory along with the provided + * Supplier. The Supplier provides resource attributes that should be included in all log events generated + * from the current resource. Note that since the Supplier is called for every LogEvent being generated + * the values returned may change as necessary. Care should be taken to make the Supplier as efficient as + * possible to avoid performance issues. + * + * Unlike regular Loggers ResourceLoggers CANNOT be declared to be static. A ResourceLogger + * must be declared as a class member that will be garbage collected along with the instance of the resource. + */ +public final class ResourceLogger extends AbstractLogger { Review Comment: Shouldn't this extend `ExtendedLoggerWrapper` instead? -- 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]
