-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
Pyic 6966 lambda catch all logging #2691
Conversation
# Conflicts: # lambdas/build-cri-oauth-request/src/test/java/uk/gov/di/ipv/core/buildcrioauthrequest/BuildCriOauthRequestHandlerTest.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/addressCri/CredentialTests.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/bavCri/ContractTest.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/cicCri/ContractTest.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/dcmawCri/ContractTest.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/drivingLicenceCri/CredentialTests.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/experianKbvCri/CredentialTests.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/fraudCri/CredentialTests.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/hmrcKbvCri/CredentialTests.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/ninoCri/CredentialTests.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/passportCri/CredentialTests.java # settings.gradle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the log appending logic of validateDoesNotLogUserIdWhenFailingValidation
be brought in line as well
...st/java/uk/gov/di/ipv/core/buildclientoauthresponse/BuildClientOauthResponseHandlerTest.java
Show resolved
Hide resolved
import static org.mockito.Mockito.verify; | ||
import static org.mockito.Mockito.when; | ||
import static uk.gov.di.ipv.core.library.config.ConfigurationVariable.AUTH_CODE_EXPIRY_SECONDS; | ||
|
||
@ExtendWith(MockitoExtension.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
|
||
@lombok.Getter | ||
public class LogCollector extends AbstractAppender { | ||
List<String> logMessages = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Minor) could make this private
|
||
public static LogCollector getLogCollectorFor(Class<?> clazz) { | ||
var logCollector = new LogCollector(); | ||
var logger = (org.apache.logging.log4j.core.Logger) LogManager.getLogger(clazz); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Minor) could shorten this cast with an import
…6-lambda-catch-all-logging # Conflicts: # lambdas/issue-client-access-token/src/test/java/uk/gov/di/ipv/core/issueclientaccesstoken/pact/IssueClientAccessTokenHandlerTest.java
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
Proposed changes
What changed
Add runtime exception logging to lambdas
Why did it change
To improve logging.
Initially I tried doing this as per the idea on the ticket, but that turned out to be impossible for the reasons explained here: https://stackoverflow.com/questions/54098144/aws-lambda-handler-throws-a-classcastexception-with-scala-generics
Note that I did not change the following lambdas as they are only run manually:
Issue tracking