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

ISOM-951: Better cloudmersive logging #1323

Merged
merged 2 commits into from
Apr 19, 2024

Conversation

timotheeg
Copy link
Contributor

@timotheeg timotheeg commented Apr 19, 2024

Problem

CloudMersove is supposed to return 200 for all scan calls, and the response body will indicate if the media contains viruses or not.

Unfortunately, we see instances where CloudMersive returns a 400, which means it is OUR error, since we are the one who make the call (it's not a client error). Our app correctly returns 500 when this happens because the 400 responses are not expected.

Unfortunately, our logging for CloudMersive errors only contains the http status message like so:

Error when calling Cloudmersive Virus Scan API: Bad Request

Which is not enough information for us to troubleshoot what about the request is wrong (e.g. a rate limit error? a body error? etc.)

Sample trace here
image
image

Solution

Improve cloudmersive logs so we can (hopefully) understand the error better, and we can also have some data about the typical viruses (if any) that CM finds for us.

Documentation is unfortunately sparse (see here), but we can see that we should have access to the response object, which should allow us see everything we need about the response (e.g. headers, body).

This PR only includes non functional changes: it's just extra logging.

Tests

  • Add a new media file into a site. Find the enhanced log entry showing the detailed scan result in the meta block.

@timotheeg timotheeg requested a review from a team April 19, 2024 03:17
message: "Error when calling Cloudmersive Virus Scan API",
error,
meta: {
data,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to note: I'm not sure if data will be populated on error (as opposed to being null/undefined, and we'd have to retrieve the response body from the response object ourselves) 🤔

But I reckon it's not very important just yet, this PR already logs a lot more info that we had before, and we can iterate to get the body later.

@timotheeg timotheeg merged commit 6479814 into develop Apr 19, 2024
19 checks passed
@mergify mergify bot deleted the better-cloudmersive-error-logging branch April 19, 2024 04:34
@alexanderleegs alexanderleegs mentioned this pull request Apr 24, 2024
11 tasks
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