Add ECS-reformatting OVERRIDE #1793

eyalkoren · 2021-04-28T09:53:50Z

What does this PR do?

Adds the ability to OVERRIDE original logs with ECS-formatted events. As opposed to SHADE and REPLACE options, this is not limited only to log files, but to additional logging options. Most importantly, it allows to reformat System.out logs to ECS, thus enabling native Docker/k8s support.

Mechanism specifications:

Supports dynamic configuration of the log_ecs_reformatting config
Guarantees that each log event is handled exactly once
ECS shade/replace appenders should be created lazily only if and when configured, in order to avoid the creation of the ecs.json file where not required
Advice calls from the logging framework should be very efficient, unless when the configuration changes and resources need to be created per appender.
A config option allows to disable override/shade based on the underlying framework formatter type (where formatter is framework-specific, e.g Layout in log4j or Encoder in Logback)

Checklist

I have updated CHANGELOG.asciidoc
I have added tests that prove the specifications above are maintained
I have tested manually with log4j1, log4j2 and Logback
I have made corresponding changes to the documentation
I have implemented the new config option
I have updated the Log Onboarding spec with required changes/additions (e.g. new config option).

apmmachine · 2021-04-28T10:14:01Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #1793 updated
Start Time: 2021-05-04T12:24:06.388+0000
Duration: 59 min 26 sec
Commit: 67d15b6

Test stats 🧪

Test	Results
Failed	0
Passed	2004
Skipped	19
Total	2023

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	2004
Skipped	19
Total	2023

...m-log4j2-plugin/src/main/java/co/elastic/apm/agent/log4j2/Log4j2AppenderGetLayoutAdvice.java

...er-plugin-common/src/main/java/co/elastic/apm/agent/log/shader/AbstractLogShadingHelper.java

...ugin/apm-log4j2-plugin/src/main/java/co/elastic/apm/agent/log4j2/Log4j2LogShadingHelper.java

…option

felixbarny

LGTM. Just some minor stylistic comments.

felixbarny · 2021-05-03T15:38:40Z

...ugin-common/src/main/java/co/elastic/apm/agent/log/shader/AbstractEcsReformattingHelper.java

+     * Used when {@link LoggingConfiguration#logEcsReformatting log_ecs_reformatting} is set to
+     * {@link LogEcsReformatting#SHADE SHADE} or {@link LogEcsReformatting#REPLACE REPLACE}.
+     */
+    private static final WeakConcurrentMap<Object, Object> originalAppender2ecsAppender = WeakMapSupplier.createMap();


Why are all maps static?

A habit 🙂
This guarantees there are no duplication, no matter where and how many helpers are created. Conceptually, everything here could be static, but with the maps this may actually have an effect if not.
Any reason to make them non-static? Are you worried from higher contention or something?

If they weren't static, we could use generics WeakConcurrentMap<A, A>/WeakConcurrentMap<A, R> which could make things a bit more readable. As we already have a static singleton instance, I was just a bit confused why the maps need to be shared across the different implementations.
That's not a blocker though.

It's a good point, but since I use dummy formatter and appender to optimize lookups that can't or shouldn't result with real implementations, switching to generic maps means adding the complication of creating these dummies in the helper subclasses. I like the fact that it is hidden from the sub-helpers, but I can try this out

felixbarny · 2021-05-03T16:00:30Z

...m-log4j2-plugin/src/main/java/co/elastic/apm/agent/log4j2/Log4j2AppenderGetLayoutAdvice.java

+ * Therefore, by instrumenting this method and replacing the returned {@link Layout}, we can implement the
+ * {@link co.elastic.apm.agent.logging.LogEcsReformatting#OVERRIDE OVERRIDE} use case.
+ */
+public class Log4j2AppenderGetLayoutAdvice {


I think we should agree on where to put advices. Either as static inner classes or as stand-alone classes.

I'd prefer the static inner class approach a lot. It makes instrumentations more cohesive as it's easier to grasp where an advice is applied to.

I agree it's much more readable this way, the thing is that it's much easier to get it wrong and make the instrumentation dependent on library types. I fixed quite a few of those lately, and I think a main reason for that is allowing something in the import section, that is not allowed somewhere in the class.

Are you thinking about automatically validating the imports? Or are you saying that it's just easier for the reviewer to verify that there are no invalid imports?

I just had an idea how we could verify that the instrumentation doesn't use invalid classes. In AbstractInstrumentationTest, we could create an isolated child class loader (parent=bootstrap) that loads each instrumentation class and calls the get*Matcher methods. That would result in linkage errors if library types are referenced.

Are you thinking about automatically validating the imports? Or are you saying that it's just easier for the reviewer to verify that there are no invalid imports?

Auto-validating the imports means looking at source, so probably not. My experience shows that it's easier for the author to add a reference to something that is already at the imports in the IDE, and easier for the reviewer to miss.

I just had an idea how we could verify that the instrumentation doesn't use invalid classes. In AbstractInstrumentationTest, we could create an isolated child class loader (parent=bootstrap) that loads each instrumentation class and calls the get*Matcher methods. That would result in linkage errors if library types are referenced.

Good idea! You mean a separate URL CL that knows the agent jar and is the child of the boot CL (since it's a unit test, we cannot rely on the boot CL to load agent classes)?
The only thing is that we will still rely only on tested JVMs, and may be exposed to differences in linkage in non tested ones, but should capture such issues very quickly in most cases.

felixbarny · 2021-05-03T16:01:02Z

...m-log4j2-plugin/src/main/java/co/elastic/apm/agent/log4j2/Log4j2AppenderGetLayoutAdvice.java

+            // Effectively disables instrumentation to all database appenders
+            return null;
+        }
+        Layout<? extends Serializable> ecsLayout = Log4J2EcsReformattingHelper.instance().getEcsOverridingFormatterFor(thisAppender);


Add a static field for the helper.

(also applies to other similar places)

felixbarny · 2021-05-03T16:05:44Z

...back-plugin-impl/src/main/java/co/elastic/apm/agent/logback/LogbackAppenderAppendAdvice.java

-        return thisAppender instanceof FileAppender && eventObject instanceof ILoggingEvent &&
-            LogbackLogShadingHelper.instance().shouldSkipAppend((FileAppender<ILoggingEvent>) thisAppender);
+
+        return eventObject instanceof ILoggingEvent && LogbackEcsReformattingHelper.instance().onAppendEnter(thisAppender);


It's a pitty that we can't use @Local in indy advices. Propagating the config value from the enter to the exit advice that way would be neat. But ofc. the ThreadLocal also works fine.

It wouldn't be enough in this case though, because in log4j2 we need to use it in a separate advice as well

felixbarny · 2021-05-03T16:07:15Z

...ack-plugin-impl/src/main/java/co/elastic/apm/agent/logback/LogbackEcsReformattingHelper.java

+
+import javax.annotation.Nullable;
+
+class LogbackEcsReformattingHelper extends AbstractEcsReformattingHelper<OutputStreamAppender<ILoggingEvent>, Encoder<ILoggingEvent>> {


I really like how straightforward the implementations now are.

Thanks!
The abstract helper contains some complexity, but not as bad as I feared and I am also satisfied with how library helpers ended up being all about library-specific-ECS-logging and the advices are minimal and almost identical.

…option

AlexanderWert · 2021-05-31T08:47:10Z

Closes #1617

Add ECS-reformatting OVERRIDE option to log4j2

5af8c36

github-actions bot added the agent-java label Apr 28, 2021

eyalkoren added 2 commits April 28, 2021 13:39

Trying to fix Mockito issue

bbb6b7d

Test enhancement

10fb88c

felixbarny reviewed Apr 28, 2021

View reviewed changes

eyalkoren added 6 commits April 28, 2021 21:19

Adding support for Logback

feddc77

Merge remote-tracking branch 'upstream/master' into ecs-log-override-…

c728df9

…option

Support dynamic config and log4j1

767ece5

Simplify everything

ac48949

Add tests for dynamic configuration

5aa6fb4

Merge remote-tracking branch 'upstream/master' into ecs-log-override-…

7f49e03

…option

eyalkoren changed the title ~~Add ECS-reformatting OVERRIDE option to log4j2~~ Add ECS-reformatting OVERRIDE May 2, 2021

eyalkoren marked this pull request as ready for review May 2, 2021 08:53

Adding formatter allow-list config

9e10c4f

eyalkoren requested a review from SylvainJuge May 2, 2021 12:29

felixbarny approved these changes May 3, 2021

View reviewed changes

eyalkoren added 3 commits May 4, 2021 15:16

Applying review suggestions

54e01e4

Merge remote-tracking branch 'upstream/master' into ecs-log-override-…

cac0eb6

…option

Adding to CHANGELOG

67d15b6

eyalkoren merged commit 49d0db5 into elastic:master May 4, 2021

eyalkoren deleted the ecs-log-override-option branch May 4, 2021 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ECS-reformatting OVERRIDE #1793

Add ECS-reformatting OVERRIDE #1793

eyalkoren commented Apr 28, 2021 •

edited by zube bot

Loading

apmmachine commented Apr 28, 2021 •

edited

Loading

Build stats

Test stats 🧪

Trends 🧪

Test stats 🧪

felixbarny left a comment

felixbarny May 3, 2021

eyalkoren May 4, 2021

felixbarny May 4, 2021

eyalkoren May 4, 2021

felixbarny May 3, 2021

eyalkoren May 4, 2021 •

edited

Loading

felixbarny May 4, 2021

eyalkoren May 4, 2021

felixbarny May 3, 2021

felixbarny May 4, 2021

eyalkoren May 4, 2021

felixbarny May 3, 2021

eyalkoren May 4, 2021

felixbarny May 3, 2021

eyalkoren May 4, 2021

AlexanderWert commented May 31, 2021


		import javax.annotation.Nullable;

		class LogbackEcsReformattingHelper extends AbstractEcsReformattingHelper<OutputStreamAppender<ILoggingEvent>, Encoder<ILoggingEvent>> {

Add ECS-reformatting OVERRIDE #1793

Add ECS-reformatting OVERRIDE #1793

Conversation

eyalkoren commented Apr 28, 2021 • edited by zube bot Loading

What does this PR do?

Checklist

apmmachine commented Apr 28, 2021 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

💚 Flaky test report

Test stats 🧪

felixbarny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyalkoren May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderWert commented May 31, 2021

eyalkoren commented Apr 28, 2021 •

edited by zube bot

Loading

apmmachine commented Apr 28, 2021 •

edited

Loading

eyalkoren May 4, 2021 •

edited

Loading