Skip to content

Commit fd334db

Browse files
jeromevdljeromevdl
committed
code review
1 parent e7edb57 commit fd334db

File tree

4 files changed

+125
-25
lines changed

4 files changed

+125
-25
lines changed

‎powertools-logging/src/main/java/software/amazon/lambda/powertools/logging/internal/DefautlLoggingManager.java‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
/*
2+
* Copyright 2023 Amazon.com, Inc. or its affiliates.
3+
* Licensed under the Apache License, Version 2.0 (the
4+
* "License"); you may not use this file except in compliance
5+
* with the License. You may obtain a copy of the License at
6+
* http://www.apache.org/licenses/LICENSE-2.0
7+
* Unless required by applicable law or agreed to in writing, software
8+
* distributed under the License is distributed on an "AS IS" BASIS,
9+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
10+
* See the License for the specific language governing permissions and
11+
* limitations under the License.
12+
*
13+
*/
14+
15+
package software.amazon.lambda.powertools.logging.internal;
16+
17+
import org.slf4j.Logger;
18+
import org.slf4j.event.Level;
19+
20+
/**
21+
* When no LoggingManager is found, setting a default one with no action on logging implementation
22+
* Powertools cannot change the log level based on the environment variable, will use the logger configuration
23+
*/
24+
public class DefautlLoggingManager implements LoggingManager {
25+
26+
@Override
27+
public void setLogLevel(Level logLevel) {
28+
// do nothing
29+
}
30+
31+
@Override
32+
public Level getLogLevel(Logger logger) {
33+
return Level.ERROR;
34+
}
35+
}

‎powertools-logging/src/main/java/software/amazon/lambda/powertools/logging/internal/LambdaLoggingAspect.java‎

Lines changed: 33 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@
4848
import java.io.InputStreamReader;
4949
import java.io.OutputStream;
5050
import java.io.OutputStreamWriter;
51+
import java.io.PrintStream;
52+
import java.security.AccessController;
53+
import java.security.PrivilegedAction;
5154
import java.util.ArrayList;
5255
import java.util.Arrays;
5356
import java.util.List;
@@ -123,24 +126,40 @@ private static Level getLevelFromString(String level) {
123126
* @return an instance of {@link LoggingManager}
124127
* @throws IllegalStateException if no {@link LoggingManager} could be found
125128
*/
129+
@SuppressWarnings("java:S106") // S106: System.err is used rather than logger to make sure message is printed
126130
private static LoggingManager getLoggingManagerFromServiceLoader() {
127-
LoggingManager loggingManager;
131+
ServiceLoader<LoggingManager> loggingManagers;
132+
SecurityManager securityManager = System.getSecurityManager();
133+
if (securityManager == null) {
134+
loggingManagers = ServiceLoader.load(LoggingManager.class);
135+
} else {
136+
final PrivilegedAction<ServiceLoader<LoggingManager>> action = () -> ServiceLoader.load(LoggingManager.class);
137+
loggingManagers = AccessController.doPrivileged(action);
138+
}
128139

129-
ServiceLoader<LoggingManager> loggingManagers = ServiceLoader.load(LoggingManager.class);
130140
List<LoggingManager> loggingManagerList = new ArrayList<>();
131141
for (LoggingManager lm : loggingManagers) {
132142
loggingManagerList.add(lm);
133143
}
144+
return getLoggingManager(loggingManagerList, System.err);
145+
}
146+
147+
static LoggingManager getLoggingManager(List<LoggingManager> loggingManagerList, PrintStream printStream) {
148+
LoggingManager loggingManager;
134149
if (loggingManagerList.isEmpty()) {
135-
throw new IllegalStateException("No LoggingManager was found on the classpath, "
136-
+
137-
"make sure to add either powertools-logging-log4j or powertools-logging-logback to your dependencies");
138-
} else if (loggingManagerList.size() > 1) {
139-
throw new IllegalStateException(
140-
"Multiple LoggingManagers were found on the classpath: " + loggingManagerList
141-
+
142-
", make sure to have only one of powertools-logging-log4j OR powertools-logging-logback to your dependencies");
150+
printStream.println("ERROR. No LoggingManager was found on the classpath");
151+
printStream.println("ERROR. Applying default LoggingManager: POWERTOOLS_LOG_LEVEL variable is ignored");
152+
printStream.println("ERROR. Make sure to add either powertools-logging-log4j or powertools-logging-logback to your dependencies");
153+
loggingManager = new DefautlLoggingManager();
143154
} else {
155+
if (loggingManagerList.size() > 1) {
156+
printStream.println("WARN. Multiple LoggingManagers were found on the classpath");
157+
for (LoggingManager manager : loggingManagerList) {
158+
printStream.println("WARN. Found LoggingManager: [" + manager + "]");
159+
}
160+
printStream.println("WARN. Make sure to have only one of powertools-logging-log4j OR powertools-logging-logback to your dependencies");
161+
printStream.println("WARN. Using the first LoggingManager found on the classpath: [" + loggingManagerList.get(0) + "]");
162+
}
144163
loggingManager = loggingManagerList.get(0);
145164
}
146165
return loggingManager;
@@ -183,7 +202,7 @@ public Object around(ProceedingJoinPoint pjp,
183202
// 3. retrieve this temporary stream to log it (if enabled)
184203
// 4. write it back to the OutputStream provided by Lambda
185204
OutputStream backupOutputStream = null;
186-
if ((logging.logResponse() || "true".equals(POWERTOOLS_LOG_RESPONSE)) && isOnRequestStreamHandler) {
205+
if ((logging.logResponse() || POWERTOOLS_LOG_RESPONSE) && isOnRequestStreamHandler) {
187206
backupOutputStream = (OutputStream) proceedArgs[1];
188207
proceedArgs[1] = new ByteArrayOutputStream();
189208
}
@@ -193,7 +212,7 @@ public Object around(ProceedingJoinPoint pjp,
193212
try {
194213
lambdaFunctionResponse = pjp.proceed(proceedArgs);
195214
} catch (Throwable t) {
196-
if (logging.logError() || "true".equals(POWERTOOLS_LOG_ERROR)) {
215+
if (logging.logError() || POWERTOOLS_LOG_ERROR) {
197216
// logging the exception with additional context
198217
logger(pjp).error(MarkerFactory.getMarker("FATAL"), "Exception in Lambda Handler", t);
199218
}
@@ -205,7 +224,7 @@ public Object around(ProceedingJoinPoint pjp,
205224
coldStartDone();
206225
}
207226

208-
if ((logging.logResponse() || "true".equals(POWERTOOLS_LOG_RESPONSE))) {
227+
if ((logging.logResponse() || POWERTOOLS_LOG_RESPONSE)) {
209228
if (isOnRequestHandler) {
210229
logRequestHandlerResponse(pjp, lambdaFunctionResponse);
211230
} else if (isOnRequestStreamHandler && backupOutputStream != null) {
@@ -222,7 +241,7 @@ private Object[] logEvent(ProceedingJoinPoint pjp, Logging logging,
222241
boolean isOnRequestHandler, boolean isOnRequestStreamHandler) {
223242
Object[] proceedArgs = pjp.getArgs();
224243

225-
if (logging.logEvent() || "true".equals(POWERTOOLS_LOG_EVENT)) {
244+
if (logging.logEvent() || POWERTOOLS_LOG_EVENT) {
226245
if (isOnRequestHandler) {
227246
logRequestHandlerEvent(pjp, pjp.getArgs()[0]);
228247
} else if (isOnRequestStreamHandler) {

‎powertools-logging/src/main/java/software/amazon/lambda/powertools/logging/internal/LoggingConstants.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,11 @@ class LoggingConstants {
2121

2222
static String POWERTOOLS_SAMPLING_RATE = System.getenv("POWERTOOLS_LOGGER_SAMPLE_RATE"); /* not final for test purpose */
2323

24-
static String POWERTOOLS_LOG_EVENT = System.getenv("POWERTOOLS_LOGGER_LOG_EVENT"); /* not final for test purpose */
24+
static boolean POWERTOOLS_LOG_EVENT = "true".equals(System.getenv("POWERTOOLS_LOGGER_LOG_EVENT")); /* not final for test purpose */
2525

26-
static String POWERTOOLS_LOG_RESPONSE = System.getenv("POWERTOOLS_LOGGER_LOG_RESPONSE"); /* not final for test purpose */
26+
static boolean POWERTOOLS_LOG_RESPONSE = "true".equals(System.getenv("POWERTOOLS_LOGGER_LOG_RESPONSE")); /* not final for test purpose */
2727

28-
static String POWERTOOLS_LOG_ERROR = System.getenv("POWERTOOLS_LOGGER_LOG_ERROR"); /* not final for test purpose */
28+
static boolean POWERTOOLS_LOG_ERROR = "true".equals(System.getenv("POWERTOOLS_LOGGER_LOG_ERROR")); /* not final for test purpose */
2929

3030
private LoggingConstants() {
3131
// constants

‎powertools-logging/src/test/java/software/amazon/lambda/powertools/logging/internal/LambdaLoggingAspectTest.java‎

Lines changed: 54 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@
4646
import java.io.ByteArrayOutputStream;
4747
import java.io.File;
4848
import java.io.IOException;
49+
import java.io.PrintStream;
50+
import java.io.UnsupportedEncodingException;
4951
import java.lang.reflect.InvocationTargetException;
5052
import java.lang.reflect.Method;
5153
import java.nio.channels.FileChannel;
@@ -54,7 +56,9 @@
5456
import java.nio.file.NoSuchFileException;
5557
import java.nio.file.Paths;
5658
import java.nio.file.StandardOpenOption;
59+
import java.util.ArrayList;
5760
import java.util.Collections;
61+
import java.util.List;
5862
import org.junit.jupiter.api.AfterEach;
5963
import org.junit.jupiter.api.Assertions;
6064
import org.junit.jupiter.api.BeforeEach;
@@ -108,7 +112,7 @@ void setUp() throws IllegalAccessException, NoSuchMethodException, InvocationTar
108112
resetLogLevel(Level.INFO);
109113
writeStaticField(LoggingConstants.class, "LAMBDA_LOG_LEVEL", null, true);
110114
writeStaticField(LoggingConstants.class, "POWERTOOLS_LOG_LEVEL", null, true);
111-
writeStaticField(LoggingConstants.class, "POWERTOOLS_LOG_EVENT", null, true);
115+
writeStaticField(LoggingConstants.class, "POWERTOOLS_LOG_EVENT", false, true);
112116
writeStaticField(LoggingConstants.class, "POWERTOOLS_SAMPLING_RATE", null, true);
113117
try {
114118
FileChannel.open(Paths.get("target/logfile.json"), StandardOpenOption.WRITE).truncate(0).close();
@@ -475,7 +479,7 @@ void shouldLogEventForHandlerWithLogEventAnnotation() {
475479
void shouldLogEventForHandlerWhenEnvVariableSetToTrue() throws IllegalAccessException {
476480
try {
477481
// GIVEN
478-
LoggingConstants.POWERTOOLS_LOG_EVENT = "true";
482+
LoggingConstants.POWERTOOLS_LOG_EVENT = true;
479483

480484
requestHandler = new PowertoolsLogEnabled();
481485

@@ -491,14 +495,14 @@ void shouldLogEventForHandlerWhenEnvVariableSetToTrue() throws IllegalAccessExce
491495
File logFile = new File("target/logfile.json");
492496
assertThat(contentOf(logFile)).contains("\"body\":\"body\"").contains("\"messageId\":\"1234abcd\"").contains("\"awsRegion\":\"eu-west-1\"");
493497
} finally {
494-
writeStaticField(LoggingConstants.class, "POWERTOOLS_LOG_EVENT", "false", true);
498+
LoggingConstants.POWERTOOLS_LOG_EVENT = false;
495499
}
496500
}
497501

498502
@Test
499503
void shouldNotLogEventForHandlerWhenEnvVariableSetToFalse() throws IOException {
500504
// GIVEN
501-
LoggingConstants.POWERTOOLS_LOG_EVENT = "false";
505+
LoggingConstants.POWERTOOLS_LOG_EVENT = false;
502506

503507
// WHEN
504508
requestHandler = new PowertoolsLogEventDisabled();
@@ -543,7 +547,7 @@ void shouldLogResponseForHandlerWithLogResponseAnnotation() {
543547
void shouldLogResponseForHandlerWhenEnvVariableSetToTrue() throws IllegalAccessException {
544548
try {
545549
// GIVEN
546-
LoggingConstants.POWERTOOLS_LOG_RESPONSE = "true";
550+
LoggingConstants.POWERTOOLS_LOG_RESPONSE = true;
547551

548552
requestHandler = new PowertoolsLogEnabled();
549553

@@ -554,7 +558,7 @@ void shouldLogResponseForHandlerWhenEnvVariableSetToTrue() throws IllegalAccessE
554558
File logFile = new File("target/logfile.json");
555559
assertThat(contentOf(logFile)).contains("Bonjour le monde");
556560
} finally {
557-
writeStaticField(LoggingConstants.class, "POWERTOOLS_LOG_RESPONSE", "false", true);
561+
LoggingConstants.POWERTOOLS_LOG_RESPONSE = false;
558562
}
559563
}
560564

@@ -597,7 +601,7 @@ void shouldLogErrorForHandlerWithLogErrorAnnotation() {
597601
void shouldLogErrorForHandlerWhenEnvVariableSetToTrue() throws IllegalAccessException {
598602
try {
599603
// GIVEN
600-
LoggingConstants.POWERTOOLS_LOG_ERROR = "true";
604+
LoggingConstants.POWERTOOLS_LOG_ERROR = true;
601605

602606
requestHandler = new PowertoolsLogEnabled(true);
603607

@@ -611,7 +615,7 @@ void shouldLogErrorForHandlerWhenEnvVariableSetToTrue() throws IllegalAccessExce
611615
File logFile = new File("target/logfile.json");
612616
assertThat(contentOf(logFile)).contains("Something went wrong");
613617
} finally {
614-
writeStaticField(LoggingConstants.class, "POWERTOOLS_LOG_ERROR", "false", true);
618+
LoggingConstants.POWERTOOLS_LOG_ERROR = false;
615619
}
616620
}
617621

@@ -694,6 +698,48 @@ void shouldLogCorrelationIdOnAppSyncEvent() throws IOException {
694698
.containsEntry("correlation_id", eventId);
695699
}
696700

701+
@Test
702+
void testMultipleLoggingManagers_shouldWarnAndSelectFirstOne() throws UnsupportedEncodingException {
703+
// GIVEN
704+
List<LoggingManager> list = new ArrayList<>();
705+
list.add(new TestLoggingManager());
706+
list.add(new DefautlLoggingManager());
707+
708+
ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
709+
PrintStream stream = new PrintStream(outputStream);
710+
711+
// WHEN
712+
LambdaLoggingAspect.getLoggingManager(list, stream);
713+
714+
// THEN
715+
String output = outputStream.toString("UTF-8");
716+
assertThat(output)
717+
.contains("WARN. Multiple LoggingManagers were found on the classpath")
718+
.contains("WARN. Make sure to have only one of powertools-logging-log4j OR powertools-logging-logback to your dependencies")
719+
.contains("WARN. Using the first LoggingManager found on the classpath: [" + list.get(0) + "]");
720+
}
721+
722+
@Test
723+
void testNoLoggingManagers_shouldWarnAndCreateDefault() throws UnsupportedEncodingException {
724+
// GIVEN
725+
List<LoggingManager> list = new ArrayList<>();
726+
727+
ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
728+
PrintStream stream = new PrintStream(outputStream);
729+
730+
// WHEN
731+
LoggingManager loggingManager = LambdaLoggingAspect.getLoggingManager(list, stream);
732+
733+
// THEN
734+
String output = outputStream.toString("UTF-8");
735+
assertThat(output)
736+
.contains("ERROR. No LoggingManager was found on the classpath")
737+
.contains("ERROR. Applying default LoggingManager: POWERTOOLS_LOG_LEVEL variable is ignored")
738+
.contains("ERROR. Make sure to add either powertools-logging-log4j or powertools-logging-logback to your dependencies");
739+
740+
assertThat(loggingManager).isExactlyInstanceOf(DefautlLoggingManager.class);
741+
}
742+
697743
private void setupContext() {
698744
when(context.getFunctionName()).thenReturn("testFunction");
699745
when(context.getInvokedFunctionArn()).thenReturn("testArn");

0 commit comments

Comments
 (0)