Skip to content
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.

Sign up for GitHub

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

Work In Progress integration test draft #12

Closed
wants to merge 1 commit into from
Closed

Work In Progress integration test draft #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions log4j2-ecs-layout/src/test/java/co/elastic/logging/log4j2/CustomPatternConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
package co.elastic.logging.log4j2;

public class CustomPatternConverter {
}
35 changes: 35 additions & 0 deletions log4j2-ecs-layout/src/test/java/co/elastic/logging/log4j2/IntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package co.elastic.logging.log4j2;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.core.LoggerContext;
import org.apache.logging.log4j.core.config.ConfigurationSource;
import org.apache.logging.log4j.core.config.Configurator;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;

public class IntegrationTest {

felixbarny marked this conversation as resolved.
Show resolved Hide resolved
@BeforeEach
public void init() throws IOException {
// ConfigurationSource source = new ConfigurationSource(new FileInputStream("log4j2.properties"));
Configurator.initialize(null, "log4j2.properties");
felixbarny marked this conversation as resolved.
Show resolved Hide resolved
}

@Test
public void testxxx(){
Logger logger = LogManager.getLogger(getClass());
felixbarny marked this conversation as resolved.
Show resolved Hide resolved
LoggerContext.getContext().getConfiguration().getProperties().put("node.id", "foo");
felixbarny marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@felixbarny felixbarny Sep 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually this line which is wrong. It works when setting the currentContext parameter to false.

Suggested change
LoggerContext.getContext().getConfiguration().getProperties().put("node.id", "foo");
LoggerContext.getContext(false).getConfiguration().getProperties().put("node.id", "foo");

If that's feasible, it would be much simpler and also more performant to not support pattern layouts within EcsLayout. Pattern layouts are meant to be used for the whole message. Using them for an alternative to lookups would be kind of a hack.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should still support pattern converters as they would be a great way to extend the ECSLayout. I understand that the ECS is expecting some fields formatted in one way, but at the same time apps using it have requirements and user's expectation that we want to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance wise I think there should not be a difference. we can always measure this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In contrast to other libraries, this one is opinionated towards ECS. If users want to customize the field name where the logger name is stored, this is probably not the library for them. I see ECS as a protocol which this library conforms to.

Also, just by providing the ability to resolve an additional property like this

    <KeyValuePair key="logger" value="%logger"/>

Does not remove the default log.logger ECS field.

On the other hand, it doesn't do much harm and it's easier to integrate than I have originally thought. So I went ahead and created a PR for it: #24

I would still suggest relying on lookups as opposed to pattern converters in Elasticsearch as lookups are more widely supported by various other libraries. Also note, that something like the %node_and_cluster_id or %exceptionAsJson pattern would not work here, as the value will always be JSON encoded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at the same time ECS is a permissive schema, we will definitely have fields which are not defined there.
If elasticsearch is to be migrated to ECS Layout then obviously we want to make sure all the current use cases work. I would prefer to avoid maintaining 3 layouts at the same time. (we still maintain plain text logs)
I am worried that %exceptionAsJson would not work. Lots of users or developers still rely on visually checking logs and having an unreadable exception + stacktrace would be bad experience for them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a PR to serialize the stackTraceAsArray: #28

final LoggerContext ctx = LoggerContext.getContext();
// ctx.reconfigure();

logger.info("he");
Logger ff = LogManager.getLogger("ff");
ff.info("ff");
}
}
15 changes: 15 additions & 0 deletions log4j2-ecs-layout/src/test/java/co/elastic/logging/log4j2/Log4j2EcsLayoutTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.apache.logging.log4j.core.BasicConfigurationFactory;
import org.apache.logging.log4j.core.Logger;
import org.apache.logging.log4j.core.LoggerContext;
import org.apache.logging.log4j.core.appender.ConsoleAppender;
import org.apache.logging.log4j.core.config.ConfigurationFactory;
import org.apache.logging.log4j.core.util.KeyValuePair;
import org.apache.logging.log4j.test.appender.ListAppender;
Expand Down Expand Up @@ -82,6 +83,9 @@ public void setUp() throws Exception {

listAppender = new ListAppender("ecs", null, ecsLayout, false, false);
listAppender.start();
ConsoleAppender consoleAppender = ConsoleAppender.createDefaultAppenderForLayout(ecsLayout);
consoleAppender.start();
root.addAppender(consoleAppender);
root.addAppender(listAppender);
root.setLevel(Level.DEBUG);
ctx.getConfiguration().getProperties().put("node.id", "foo");
Expand All @@ -92,6 +96,17 @@ public void tearDown() throws Exception {
ThreadContext.clearAll();
}

@Test
void patternConverters(){
ctx.getConfiguration().getProperties().put("node.id", "%d");
root.debug("test ");
}

@Test
void markerLogger(){

}

@Test
void globalLabels() throws Exception {
putMdc("trace.id", "foo");
Expand Down
17 changes: 17 additions & 0 deletions log4j2-ecs-layout/src/test/resources/log4j2.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
status = error

# log action execution errors for easier debugging
logger.action.name = org.elasticsearch.action
logger.action.level = debug


appender.console.type = Console
appender.console.name = console
appender.console.layout.type = EcsLayout
appender.console.layout.serviceName = ES_ECS
appender.console.layout.additionalFields0.type = KeyValuePair
appender.console.layout.additionalFields0.key = node.name
appender.console.layout.additionalFields0.value = ${node_name}

rootLogger.level = info
rootLogger.appenderRef.console.ref = console
33 changes: 17 additions & 16 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@
</developers>

<properties>
<maven.compiler.source>12</maven.compiler.source>
<version.junit-jupiter>5.5.1</version.junit-jupiter>
<maven.compiler.target>6</maven.compiler.target>
<maven.compiler.target>12</maven.compiler.target>
<maven.compiler.testTarget>10</maven.compiler.testTarget>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
Expand Down Expand Up @@ -131,21 +132,21 @@
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-javadoc-plugin</artifactId>
<version>3.1.1</version>
<configuration>
<additionalOptions>-html5</additionalOptions>
</configuration>
<executions>
<execution>
<goals>
<goal>jar</goal>
</goals>
</execution>
</executions>
</plugin>
<!-- <plugin>-->
<!-- <groupId>org.apache.maven.plugins</groupId>-->
<!-- <artifactId>maven-javadoc-plugin</artifactId>-->
<!-- <version>3.1.1</version>-->
<!-- <configuration>-->
<!-- <additionalOptions>-html5</additionalOptions>-->
<!-- </configuration>-->
<!-- <executions>-->
<!-- <execution>-->
<!-- <goals>-->
<!-- <goal>jar</goal>-->
<!-- </goals>-->
<!-- </execution>-->
<!-- </executions>-->
<!-- </plugin>-->
<!-- Check that we don't accidentally use features only available in Java 8+ -->
<plugin>
<groupId>org.codehaus.mojo</groupId>
Expand Down