Skip to content

Send different Apify errors to Sentry based on error description #604

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.

Sign up for GitHub

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

Jump to bottom
Merged
vasconsaurus merged 2 commits into from
Nov 5, 2025
Merged

Send different Apify errors to Sentry based on error description #604

vasconsaurus merged 2 commits into from
Nov 5, 2025

Conversation

@vasconsaurus
Copy link
Contributor

@vasconsaurus vasconsaurus commented Nov 4, 2025 •
edited
Loading

Description

At first, I wanted to make this more flexible, because I thought a few errors were being caused by this. But on a second look, the errors were unrelated.

I also wanted to send specific errors to Sentry so I can have better visibility. Most important,
it will make actor memory limit errors more visible in Sentry.

So I ignored the first part, and focused on the second part.

References: CV2-6464

How has this been tested?

rails test test/models/media_apify_item_test.rb

Things to pay attention to during code review

Please describe parts of the change that require extra attention during code review, for example:

  • File FFFF, line LL: This refactoring does this and this. Is it consistent with how it’s implemented elsewhere?
  • Etc.

Checklist

  • I have performed a self-review of my own code
  • I have added unit and feature tests, if the PR implements a new feature or otherwise would benefit from additional testing
  • I have added regression tests, if the PR fixes a bug
  • I have added logging, exception reporting, and custom tracing with any additional information required for debugging
  • I considered secure coding practices when writing this code. Any security concerns are noted above.
  • I have commented my code in hard-to-understand areas, if any
  • I have made needed changes to the README
  • My changes generate no new warnings
  • If I added a third party module, I included a rationale for doing so and followed our current guidelines

@vasconsaurus
Copy link
Contributor Author

vasconsaurus commented Nov 4, 2025 •
edited
Loading

@melsawy, I have three failing tests, but they are unrelated and also failing in main.
I'll look into those in a separate PR.

Update
Fixed those here: #605

@vasconsaurus vasconsaurus marked this pull request as ready for review November 4, 2025 20:22
@vasconsaurus vasconsaurus requested a review from melsawy as a code owner November 4, 2025 20:22
@caiosba
Copy link
Contributor

caiosba commented Nov 4, 2025

@melsawy, I have three failing tests, but they are unrelated and also failing in main. I'll look into those in a separate PR.

@vasconsaurus , since those are also failing on main, can you please just file and maybe Sawy can fix them? I want to make sure you can dedicate most of your time for the Suwali work

@vasconsaurus
Copy link
Contributor Author

vasconsaurus commented Nov 4, 2025

@melsawy, I have three failing tests, but they are unrelated and also failing in main. I'll look into those in a separate PR.

@vasconsaurus , since those are also failing on main, can you please just file and maybe Sawy can fix them? I want to make sure you can dedicate most of your time for the Suwali work

I thought they looked simple, so I ended up working on them. I have a draft PR up, I'm just waiting for tests to pass.

@vasconsaurus
Send different Apify errors to Sentry based on error description
7e6d61a 
This will help keeping track of specific issues, most important,
it will make actor memory limit errors more visible in Sentry.
@vasconsaurus
Update Exception Class References in Parser Tests
ea420fb 
I updated the apify exceptions to follow the same pattern as our other exceptions. Specifically, I changed references from MediaApifyItem::ApifyError to Pender::Exception::ApifyError in the parser test files. This ensures consistency in exception handling across the codebase.
So i had to rename them here.
@vasconsaurus vasconsaurus force-pushed the 6464-make-apify-error-flexible branch from 9f7a839 to ea420fb Compare November 4, 2025 20:54
@melsawy
Copy link

melsawy commented Nov 4, 2025

@melsawy, I have three failing tests, but they are unrelated and also failing in main. I'll look into those in a separate PR.

@vasconsaurus , since those are also failing on main, can you please just file and maybe Sawy can fix them? I want to make sure you can dedicate most of your time for the Suwali work

I thought they looked simple, so I ended up working on them. I have a draft PR up, I'm just waiting for tests to pass.

np Manu I can take care about this PR, I approved this PR and will merge it here and keep an eye on tests

@vasconsaurus
Copy link
Contributor Author

vasconsaurus commented Nov 4, 2025

np Manu I can take care about this PR, I approved this PR and will merge it here and keep an eye on tests

Sorry, @melsawy I rebased and pushed before I saw your comment.

@vasconsaurus vasconsaurus merged commit 4be086a into main Nov 5, 2025
5 checks passed
@vasconsaurus vasconsaurus deleted the 6464-make-apify-error-flexible branch November 5, 2025 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@melsawy melsawy melsawy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vasconsaurus @caiosba @melsawy