-
Notifications
You must be signed in to change notification settings - Fork 17
Log API Improvements #974
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
base: master
Are you sure you want to change the base?
Log API Improvements #974
Conversation
…age` fields - Ensured `data` field is unescaped JSON, while `message` remains a simple String - Updated `EcsLayout.json` to include `message`, `data`, and `context` fields (or add a new template) - `context` is populated via MDC (`ThreadContext.put`) and supports simple key-value pairs
…to be compatible with the existing implementation of the log (the old implementation remains unchanged).
…abs/JavaClasses into feature/log-improvement
|
||
default boolean isEnabled(int logLevel) { return false; } | ||
|
||
//boolean isEnabled(int logLevel, String topic); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this method commented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
|
||
public static void write(String message, String topic, int logLevel, Object data) { | ||
getLogger(topic).write(message, logLevel, data, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to call write(message, logLevel, data, false); here to avoid duplicating code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrappercommon/pom.xml
Outdated
</dependency> | ||
<dependency> | ||
<groupId>com.google.code.gson</groupId> | ||
<artifactId>gson</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're already using a JSON handling library (org.json), so I'd consider whether we can use that instead of Gson to avoid having two separate libraries for JSON processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're already using a JSON handling library (org.json), so I'd consider whether we can use that instead of Gson to avoid having two separate libraries for JSON processing.
I did an implementation using org.json, the problems I found with this library are these:
- It doesn't keep the order of the inserted element in the JSON (It uses internally a HashMap instead of LinkedHashMap).
- The code is more complex than using Gson.
The order of the elements is my main concern, do you think it is not relevant for an end user? For example, to print the parameters of a function, or print SDTs, etc., they will be out of order after serialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order of elements in a JSON object is not relevant according to the JSON specification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order of elements in a JSON object is not relevant according to the JSON specification
Ok, this is the implementation using org.json: Uses org.json instead of Gson
Map<String, Object> mapMessage = new LinkedHashMap<>(); | ||
|
||
if (data == null || (data instanceof String && "null".equals(data.toString()))) { | ||
mapMessage.put(dataKey, (Object) null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this cast is not necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is fixed when I migrate to org.json, in this commit: Uses org.json instead of Gson
|
||
for (Appender appender : config.getAppenders().values()) { | ||
if (appender instanceof AbstractAppender) { | ||
Object layout = ((AbstractAppender) appender).getLayout(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this cast is not necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix: Remove casting
@@ -0,0 +1,57 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't understand this template when exactly is it loaded to be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is loaded here: PR #959
Fix: Move it to the resources folder. When compiling it will be in the root of the jar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments.
@@ -2,13 +2,12 @@ | |||
|
|||
public class LogLevel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do something like this? To ensure type safety
public enum LogLevel {
OFF(0),
TRACE(1),
DEBUG(5),
INFO(10),
WARNING(15),
ERROR(20),
FATAL(30);
private final int lvl;
LogLevel(int lvl) { this.lvl = lvl; }
public int intValue() { return lvl; }
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a function like this inside that enum, to be able to assign the value of the received log level.
public static LogLevel fromInt(int lvl) { for (LogLevel level : LogLevel.values()) { if (level.intValue() == lvl) { return level; } } return LogLevel.OFF; }
Fix: Ensure type safety
return getLogger().isInfoEnabled(); | ||
} | ||
|
||
public static boolean isWarnEnabled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your level constant is called WARNING, but the method is isWarnEnabled(). It’s better if the naming matches exactly (e.g. either rename the constant to WARN or the method to isWarningEnabled())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
@Plugin(name = "CustomMessageFactory", category = TemplateResolverFactory.CATEGORY) | ||
public final class CustomMessageFactory implements EventResolverFactory { | ||
private static final CustomMessageFactory INSTANCE = new CustomMessageFactory(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now you have a package-private default constructor (implicit), plus a single INSTANCE. It’s clearer to readers if you explicitly prevent instantiation
private static final CustomMessageFactory INSTANCE = new CustomMessageFactory();
private CustomMessageFactory() { /* no instances */ }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import org.apache.logging.log4j.layout.template.json.resolver.TemplateResolverFactory; | ||
|
||
|
||
@Plugin(name = "CustomMessageFactory", category = TemplateResolverFactory.CATEGORY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure about this one but right now the plugin is called "CustomMessageFactory", but your resolver class is CustomMessageResolver. It can be confusing in XML/JSON layouts if you have to write:
<CustomMessageFactory …/>
Maybe renaming the plugin to "CustomMessage" (or "CustomMessageResolver") so it reads naturally in config and maps 1:1 to your resolver’s functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the example in the official documentation, I don't know what the CustomMessageFactory string is for.
Anyway I have made the change as you say.
Fix: Plugin rename
|
||
private static final String STACKTRACE_KEY = "stackTrace"; | ||
private static final String MESSAGE_KEY = "message"; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You currently always build your JSON or text payload—even if that level is disabled—only to have Log4j drop it. To avoid the cost of JSON parsing/serialization on disabled levels, add at the top of your write(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static final String STACKTRACE_KEY = "stackTrace"; | ||
private static final String MESSAGE_KEY = "message"; | ||
|
||
private void writeTextFormat(String message, int logLevel, Object data, boolean stackTrace) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now every write() walks the entire configuration looking for a JsonTemplateLayout. That’s very expensive if you’re logging hundreds of messages per second. Instead:
- Compute it once (e.g. in your constructor, or lazily in a volatile Boolean isJsonLayout field).
- Reuse that flag on each write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mapMessage.put(STACKTRACE_KEY, getStackTraceAsList()); | ||
} | ||
|
||
String json = new Gson().newBuilder().serializeNulls().create().toJson(mapMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You call new Gson().newBuilder().serializeNulls().create() on every text‐format write.
Instead:
private static final Gson GSON =
new GsonBuilder().serializeNulls().create();
And then reuse GSON.toJson(...). This avoids constant object churn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was fixed when I migrated to org.json instead of Gson: Uses org.json instead of Gson
|
||
private static List<String> getStackTraceAsList() { | ||
List<String> stackTraceLines = new ArrayList<>(); | ||
for (StackTraceElement ste : Thread.currentThread().getStackTrace()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure about this one but I think that collecting the entire Thread.currentThread().getStackTrace() will include your own logging frames plus JVM internals. If what you want is the user’s call stack, consider filtering out frames until you reach the first non-logging class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cherry pick to beta failed, 1 conflicted file in commit c6c58e5
|
Cherry pick to beta success |
Issue:204872
Add custom JSON logging support with unescaped
data
and plainmessage
fieldsdata
field is unescaped JSON, whilemessage
remains a simple StringEcsLayout.json
to includemessage
,data
, andcontext
fields (or add a new template)context
is populated via MDC (ThreadContext.put
) and supports simple key-value pairs