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.

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

Merged
merged 10 commits into from
Nov 15, 2024

Conversation

DanCorderIPV
Copy link
Contributor

@DanCorderIPV DanCorderIPV commented Nov 8, 2024

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:

  • bulk-migrate-vcs
  • delete-user-data
  • reconcile-migrated-vcs
  • replay-cimit-vcs
  • report-user-identity

Issue tracking

@DanCorderIPV DanCorderIPV requested review from a team as code owners November 8, 2024 16:05
# 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
Copy link
Contributor

@MikeCollingwood MikeCollingwood left a 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

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)
Copy link
Contributor

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<>();
Copy link
Contributor

@MikeCollingwood MikeCollingwood Nov 12, 2024

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);
Copy link
Contributor

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
Copy link

sonarcloud bot commented Nov 15, 2024

Copy link
Contributor

@MikeCollingwood MikeCollingwood left a 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

@DanCorderIPV DanCorderIPV merged commit 82ebfaa into main Nov 15, 2024
7 checks passed
@DanCorderIPV DanCorderIPV deleted the PYIC-6966-lambda-catch-all-logging branch November 15, 2024 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants