Skip to content

feat(logging): add support for key-value pairs in LambdaJsonEncoder #2377

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

Draft
cmtjk wants to merge 1 commit into
base: main
Choose a base branch
from
+77 −8
Draft

feat(logging): add support for key-value pairs in LambdaJsonEncoder #2377

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
6 changes: 6 additions & 0 deletions ...ng-logback/src/main/java/software/amazon/lambda/powertools/logging/logback/JsonUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
}

static void serializeMDCEntries(Map<String, String> mdcPropertyMap, JsonSerializer serializer) {
TreeMap<String, String> sortedMap = new TreeMap<>(mdcPropertyMap);

Check failure on line 58 in powertools-logging/powertools-logging-logback/src/main/java/software/amazon/lambda/powertools/logging/logback/JsonUtils.java

View workflow job for this annotation

GitHub Actions / pmd_analyse

Avoid using implementation types like 'TreeMap'; use the interface instead 

Excessive coupling to implementation types (e.g., `HashSet`) limits your ability to use alternate
implementations in the future as requirements change. Whenever available, declare variables
and parameters using a more general type (e.g, `Set`).

This rule reports uses of concrete collection types. User-defined types that should be treated
the same as interfaces can be configured with the property `allowedTypes`.

LooseCoupling (Priority: 1, Ruleset: Best Practices)
https://docs.pmd-code.org/snapshot/pmd_rules_java_bestpractices.html#loosecoupling
for (Map.Entry<String, String> entry : sortedMap.entrySet()) {
if (!PowertoolsLoggedFields.stringValues().contains(entry.getKey())) {
serializeMDCEntry(entry, serializer);
Expand All @@ -73,6 +73,12 @@
}
}

static void serializeKVPEntry(String key, Object value, JsonSerializer serializer) {
serializer.writeRaw(',');
serializer.writeFieldName(key);
serializer.writeObject(value);
}

static void serializeArguments(ILoggingEvent event, JsonSerializer serializer) throws IOException {
Object[] arguments = event.getArgumentArray();
if (arguments != null) {
Expand Down Expand Up @@ -110,9 +116,9 @@
return false;
}
if (str.charAt(0) == '-') {
if (str.length() == 1) {
return false;
}

Check failure on line 121 in powertools-logging/powertools-logging-logback/src/main/java/software/amazon/lambda/powertools/logging/logback/JsonUtils.java

View workflow job for this annotation

GitHub Actions / pmd_analyse

This if statement can be replaced by `return !{condition} && {elseBranch};` 

Avoid unnecessary if-then-else statements when returning a boolean. The result of
the conditional test can be returned instead.

SimplifyBooleanReturns (Priority: 1, Ruleset: Design)
https://docs.pmd-code.org/snapshot/pmd_rules_java_design.html#simplifybooleanreturns
return withDecimalsParsing(str, 1);
}
return withDecimalsParsing(str, 0);
Expand Down
20 changes: 12 additions & 8 deletions ...ck/src/main/java/software/amazon/lambda/powertools/logging/logback/LambdaJsonEncoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,7 @@
package software.amazon.lambda.powertools.logging.logback;

import static java.nio.charset.StandardCharsets.UTF_8;
import static software.amazon.lambda.powertools.logging.logback.JsonUtils.serializeArguments;
import static software.amazon.lambda.powertools.logging.logback.JsonUtils.serializeMDCEntries;
import static software.amazon.lambda.powertools.logging.logback.JsonUtils.serializeMDCEntry;
import static software.amazon.lambda.powertools.logging.logback.JsonUtils.serializeTimestamp;
import static software.amazon.lambda.powertools.logging.logback.JsonUtils.*;

import ch.qos.logback.classic.pattern.ThrowableHandlingConverter;
import ch.qos.logback.classic.pattern.ThrowableProxyConverter;
Expand All @@ -27,10 +24,8 @@
import ch.qos.logback.classic.spi.ThrowableProxy;
import ch.qos.logback.core.encoder.EncoderBase;
import java.io.IOException;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
import java.util.TreeMap;
import java.util.*;

import software.amazon.lambda.powertools.logging.internal.JsonSerializer;
import software.amazon.lambda.powertools.logging.internal.PowertoolsLoggedFields;

Expand Down Expand Up @@ -83,11 +78,13 @@

serializeException(event, serializer);

TreeMap<String, String> sortedMap = new TreeMap<>(event.getMDCPropertyMap());

Check failure on line 81 in powertools-logging/powertools-logging-logback/src/main/java/software/amazon/lambda/powertools/logging/logback/LambdaJsonEncoder.java

View workflow job for this annotation

GitHub Actions / pmd_analyse

Avoid using implementation types like 'TreeMap'; use the interface instead 

Excessive coupling to implementation types (e.g., `HashSet`) limits your ability to use alternate
implementations in the future as requirements change. Whenever available, declare variables
and parameters using a more general type (e.g, `Set`).

This rule reports uses of concrete collection types. User-defined types that should be treated
the same as interfaces can be configured with the property `allowedTypes`.

LooseCoupling (Priority: 1, Ruleset: Best Practices)
https://docs.pmd-code.org/snapshot/pmd_rules_java_bestpractices.html#loosecoupling
serializePowertools(sortedMap, serializer);

serializeMDCEntries(sortedMap, serializer);

serializeKeyValuePairs(event, serializer);

serializeArguments(event, serializer);

serializeThreadInfo(event, serializer);
Expand All @@ -104,6 +101,13 @@
return builder.toString().getBytes(UTF_8);
}

private void serializeKeyValuePairs(ILoggingEvent event, JsonSerializer serializer) {
Optional.ofNullable(event.getKeyValuePairs())
.orElse(Collections.emptyList()).stream()
.filter(Objects::nonNull)
.forEach(kvp -> serializeKVPEntry(String.valueOf(kvp.key), kvp.value, serializer));
}

private void serializeThreadInfo(ILoggingEvent event, JsonSerializer serializer) {
if (includeThreadInfo) {
if (event.getThreadName() != null) {
Expand All @@ -117,11 +121,11 @@
}
}

private void serializePowertools(TreeMap<String, String> sortedMap, JsonSerializer serializer) {

Check failure on line 124 in powertools-logging/powertools-logging-logback/src/main/java/software/amazon/lambda/powertools/logging/logback/LambdaJsonEncoder.java

View workflow job for this annotation

GitHub Actions / pmd_analyse

Avoid using implementation types like 'TreeMap'; use the interface instead 

Excessive coupling to implementation types (e.g., `HashSet`) limits your ability to use alternate
implementations in the future as requirements change. Whenever available, declare variables
and parameters using a more general type (e.g, `Set`).

This rule reports uses of concrete collection types. User-defined types that should be treated
the same as interfaces can be configured with the property `allowedTypes`.

LooseCoupling (Priority: 1, Ruleset: Best Practices)
https://docs.pmd-code.org/snapshot/pmd_rules_java_bestpractices.html#loosecoupling
if (includePowertoolsInfo) {
for (Map.Entry<String, String> entry : sortedMap.entrySet()) {
if (PowertoolsLoggedFields.stringValues().contains(entry.getKey())
&& !(entry.getKey().equals(PowertoolsLoggedFields.SAMPLING_RATE.getName()) && entry.getValue().equals("0.0"))) {

Check failure on line 128 in powertools-logging/powertools-logging-logback/src/main/java/software/amazon/lambda/powertools/logging/logback/LambdaJsonEncoder.java

View workflow job for this annotation

GitHub Actions / pmd_analyse

Position literals first in String comparisons 

Position literals first in all String comparisons, if the second argument is null then NullPointerExceptions
can be avoided, they will just return false. Note that switching literal positions for compareTo and
compareToIgnoreCase may change the result, see examples.

Note that compile-time constant strings are treated like literals. This is because they are inlined into
the class file, are necessarily non-null, and therefore cannot cause an NPE at runtime.

LiteralsFirstInComparisons (Priority: 1, Ruleset: Best Practices)
https://docs.pmd-code.org/snapshot/pmd_rules_java_bestpractices.html#literalsfirstincomparisons
serializeMDCEntry(entry, serializer);
}
}
Expand Down
59 changes: 59 additions & 0 deletions ...c/test/java/software/amazon/lambda/powertools/logging/internal/LambdaJsonEncoderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import java.nio.file.StandardOpenOption;
import java.text.SimpleDateFormat;
import java.util.Arrays;
import java.util.List;
import java.util.Collections;
import java.util.Date;
import java.util.TimeZone;
Expand All @@ -50,6 +51,7 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Order;
import org.junit.jupiter.api.Test;
import org.slf4j.event.KeyValuePair;
import software.amazon.lambda.powertools.common.stubs.TestLambdaContext;
import org.slf4j.LoggerFactory;
import org.slf4j.MDC;
Expand Down Expand Up @@ -442,4 +444,61 @@ void shouldLogException() {
.contains("\"stack\":\"java.lang.IllegalStateException: Unexpected value\\n");
}

@Test
void shouldLogKeyValuePairs() {
// GIVEN
LambdaJsonEncoder encoder = new LambdaJsonEncoder();
encoder.start();

Object[] arguments = {
"argument_01",
StructuredArguments.entry("structured_argument_01_retain", "retained"),
StructuredArguments.entry("structured_argument_02_overwrite", "to_be_overwritten")
};
LoggingEvent keyValuePairsLoggingEvent = new LoggingEvent("fqcn", logger, Level.INFO, "Key Value Pairs Test with argument: {}",
null, arguments);

MDC.put("mdc_01_retain", "retained");
MDC.put("mdc_02_overwrite", "to_be_overwritten");

keyValuePairsLoggingEvent.setKeyValuePairs(List.of(
new KeyValuePair("key_01_string", "value_01"),
new KeyValuePair("key_02_numeric", 2),
new KeyValuePair("key_03_decimal", 2.333),
new KeyValuePair("key_04_null", null),
new KeyValuePair("", "value_05_empty_key"),
new KeyValuePair(null, "value_06_null_key"),
new KeyValuePair("key_07_boolean_true", true),
new KeyValuePair("key_08_boolean_false", false),
new KeyValuePair("mdc_02_overwrite", "overwritten_by_kvp"),
new KeyValuePair("structured_argument_02_overwrite", "overwritten_by_kvp")
));

// WHEN
byte[] encoded = encoder.encode(keyValuePairsLoggingEvent);
String result = new String(encoded, StandardCharsets.UTF_8);

// THEN
assertThat(result)
// Arguments
.contains("Key Value Pairs Test with argument: argument_01")
.contains("\"structured_argument_01_retain\":\"retained\"")
// .doesNotContain("\"structured_argument_02_overwrite\":\"to_be_overwritten\"") TODO: Deduplication not implemented vor Arguments
// MDC
.contains("\"mdc_01_retain\":\"retained\"")
// .doesNotContain("\"mdc_02_overwrite\":\"to_be_overwritten\"") TODO: Deduplication not implemented vor Arguments
// Key Value Pairs
.contains("\"key_01_string\":\"value_01\"")
.contains("\"key_02_numeric\":2")
.contains("\"key_03_decimal\":2.333")
.contains("\"key_04_null\":null")
.contains("\"\":\"value_05_empty_key\"")
.contains("\"null\":\"value_06_null_key\"")
.contains("\"key_07_boolean_true\":true")
.contains("\"key_08_boolean_false\":false")
.contains("\"mdc_02_overwrite\":\"overwritten_by_kvp\"")
.contains("\"structured_argument_02_overwrite\":\"overwritten_by_kvp\"")
;
}

}