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

feat: better exception handling #40

Merged
merged 18 commits into from
Nov 16, 2023
Merged

Conversation

brianreichtcs
Copy link
Collaborator

@brianreichtcs brianreichtcs commented Nov 15, 2023

If approved this PR:

  1. Adds a dependency on psr/log
  2. Adds a private method logFailedRequest to Client which, if a logger is present, logs the failed request/response.
  3. Adds a call to logFailedRequest() everywhere a SmarterUException is thrown.

With this change in place, we can actually capture information we need to troubleshoot integration issues.

One thing you might wonder about is, why am I not testing that the logger gets data when a failure occurs? The reason is because psr/log doesn't ship a test logger, only NullLogger, which is a "no-op" implementation that I cannot expect to validate log events.

Troubleshooting issues with SmarterU is made several times more complicated than it needs to be because we don't capture useful information in the exception and log it anywhere.  I am to fix this by adding request and response to the exception details.  I don't want sensitive credentials going in our logs so I made sure to sanitize the request.
@brianreichtcs brianreichtcs self-assigned this Nov 15, 2023
@brianreichtcs brianreichtcs requested review from a team and tomegantcs and removed request for a team November 15, 2023 20:29
@brianreichtcs brianreichtcs marked this pull request as draft November 16, 2023 14:00
@brianreichtcs
Copy link
Collaborator Author

Closing this until I'm done making changes

@brianreichtcs brianreichtcs reopened this Nov 16, 2023
@brianreichtcs brianreichtcs marked this pull request as ready for review November 16, 2023 15:16
@brianreichtcs brianreichtcs requested a review from a team November 16, 2023 15:16
Copy link
Contributor

@tomegantcs tomegantcs left a comment

Choose a reason for hiding this comment

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

I don't think it would be difficult to add tests to see that something was sent to a logger. In tests/Client/AddUsersToGroupClientTest.php for instance line 287 would be changed to:

$logger = $this->createMock(\Psr\Log\LoggerInterface::class);
$logger->expects($this->once())->method('error')->with(
    $this->identicalTo('Failed to make request to SmarterU API. See context for request/response details.'),
    $this->identicalTo([
        'request' => // I'm not sure but maybe you put 'cat' here and see what the failure looks like to get the text?
        'response' => $body
    ])
);
$client
    ->setLogger($logger)
    ->setHttpClient($httpClient);

src/Client.php Outdated Show resolved Hide resolved
@brianreichtcs brianreichtcs merged commit 00be9bd into main Nov 16, 2023
2 checks passed
@brianreichtcs brianreichtcs deleted the feature/better-exceptions branch November 16, 2023 19:24
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