Review request for Log4j JsonTemplateLayout

[vy]

Review request for Log4j JsonTemplateLayout

[felixbarny]

Great to see this being adopted in log4j! Nice work 👏

threadLocal performs the best, since every instance is stored in TLAB

TLAB stands for thread local allocation buffer which is something different. You might want to change TLAB -> ThreadLocal

I would also explain the possibility of class loader leaks when using the threadLocal strategy with application servers.

In the ECS JSON layout, MDC values are serialized on the top-level now, as supposed to nested under labels and may contain non-string values.

/cc @xeraa FYI

[vy]

Thanks for the thorough review @felixbarny, really appreciated!

TLAB stands for thread local allocation buffer which is something different. You might want to change TLAB -> ThreadLocal

☑️

I would also explain the possibility of class loader leaks when using the threadLocal strategy with application servers.

☑️

In the ECS JSON layout, MDC values are serialized on the top-level now, as supposed to nested under labels

☑️

and may contain non-string values.

I've checked out the sources of log4j2-ecs-layout-0.4.0 and there EcsLayout contains the following lines:

    private final TriConsumer<String, Object, StringBuilder> WRITE_MDC = new TriConsumer<String, Object, StringBuilder>() {
        @Override
        public void accept(final String key, final Object value, final StringBuilder stringBuilder) {
            stringBuilder.append('\"');
            JsonUtils.quoteAsString(key, stringBuilder);
            stringBuilder.append("\":\"");
            JsonUtils.quoteAsString(EcsJsonSerializer.toNullSafeString(String.valueOf(value)), stringBuilder);
            stringBuilder.append("\",");
        }
    };

    private void serializeAdditionalFieldsAndMDC(LogEvent event, StringBuilder builder) {
        final int length = additionalFields.length;
        if (!event.getContextData().isEmpty() || length > 0) {
            // ...
            event.getContextData().forEach(WRITE_MDC, builder);
        }
    }

Would you mind elaborating on how come an MDC value of type, say, int gets emitted as a JSON number rather than a text?

[mzeijen]

I reviewed the documentation and cross checked it with our current setup of the LogstashLayout. The documentation is well written and understandable. I don't see a reason why we can't migrate from LogstashLayout to JsonTemplateLayout.

There is one feature of which I don't understand completely what its function is and that is the "Additonal event template fields". Why is it useful and when should I use it? Maybe that could be elaborated a bit more in the documentation.

Another question: is $${json:exception:stackTrace:text} from LogstashLayout the same as an exception resolver with stringified enabled?

I do have one suggestion and that is to add output examples of the effects of the resolvers and such. That would make it easier to understand what they do. This might make the documentation more bloated however, so I can understand if you are reluctant.

[felixbarny]

Would you mind elaborating on how come an MDC value of type, say, int gets emitted as a JSON number rather than a text?

What I meant is that currently the MDCs are stringified in EcsLayout.json, which is not needed when serializing at the top level:

https://github.com/apache/logging-log4j2/blob/bdc12dcd265ce155591f05ce48e46310a4bf0e66/log4j-layout-json-template/src/main/resources/EcsLayout.json#L30

[vy]

Thanks so much for taking time to review the manual @mzeijen, really appreciated!

There is one feature of which I don't understand completely what its function is and that is the "Additonal event template fields". Why is it useful and when should I use it? Maybe that could be elaborated a bit more in the documentation.

☑️ Updated the manual.

Another question: is $${json:exception:stackTrace:text} from LogstashLayout the same as an exception resolver with stringified enabled?

Correct. This will do the trick:

{
  "$resolver": "exception",
  "field": "stackTrace",
  "stringified": true
}

I do have one suggestion and that is to add output examples of the effects of the resolvers and such. That would make it easier to understand what they do. This might make the documentation more bloated however, so I can understand if you are reluctant.

Yes... I am indeed sitting on the fence about this one. I think I will first wait and see the reaction of the users. If there pops up many of such questions, I will add examples.

[vy]

Would you mind elaborating on how come an MDC value of type, say, int gets emitted as a JSON number rather than a text?

What I meant is that currently the MDCs are stringified in EcsLayout.json, which is not needed when serializing at the top level:

https://github.com/apache/logging-log4j2/blob/bdc12dcd265ce155591f05ce48e46310a4bf0e66/log4j-layout-json-template/src/main/resources/EcsLayout.json#L30

Ah... Now I see your point. I'd rather stick to stringified version to match the log4j2-ecs-layout behavior. This feels like the option with the least surprise factor for users. If needed, they can override it via additional fields anyway.

[vy]

Log4j 2.14.0 is released! 🥳 Thanks to everyone for the feedback on the design drafts of JsonTemplateLayout shipped with this release!