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(sentry): setup before_send to ignore global id errors by message #461

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nikomakela
Copy link
Contributor

KK-1395.

The ignore_messages option in sentry_sdk would be great, but since the Graphene raises a general Exception from invalid global ids, we need to follow the exception message instead of class.

@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr461.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is success for https://kukkuu-pr461.api.dev.hel.ninja 😆🎉🎉🎉

@nikomakela nikomakela force-pushed the KK-1395-sentry-ignore-messages branch from 33305b1 to d4b9074 Compare January 10, 2025 14:57
@nikomakela nikomakela changed the base branch from master to KK-1396-sentry-ignore-expired-signature January 10, 2025 14:58
@nikomakela nikomakela marked this pull request as ready for review January 10, 2025 15:17
@nikomakela
Copy link
Contributor Author

NOTE: We may not want to have this feature enabled, because then we cannot follow these "unknown" issues in Sentry.

@nikomakela nikomakela force-pushed the KK-1396-sentry-ignore-expired-signature branch from 24b03ca to a45948b Compare January 13, 2025 07:43
@nikomakela nikomakela force-pushed the KK-1395-sentry-ignore-messages branch from d4b9074 to 59b5bb9 Compare January 13, 2025 09:08
Copy link
Contributor Author

@nikomakela nikomakela left a comment

Choose a reason for hiding this comment

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

As discussed with @karisal-anders , something like this could be an improvement?

Copy link
Contributor

@karisal-anders karisal-anders left a comment

Choose a reason for hiding this comment

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

With the str(exc_value) change (it's clearer that way) looks ok to me, adding a comment about what the "Unable to parse global ID" error is about, and why it is ok to ignore it (i.e. if it is akin to a 404 but thrown by Graphene, and that we have not found a systematic problem with our system creating such problematic non-parseable global ID values) would add clarity/transparency to this still.

@nikomakela nikomakela force-pushed the KK-1396-sentry-ignore-expired-signature branch from a45948b to e3c4e4c Compare January 13, 2025 11:24
@nikomakela nikomakela force-pushed the KK-1395-sentry-ignore-messages branch 2 times, most recently from 6be690a to 72042da Compare January 13, 2025 11:35
@nikomakela nikomakela force-pushed the KK-1396-sentry-ignore-expired-signature branch from e3c4e4c to bc6e627 Compare January 13, 2025 11:35
Base automatically changed from KK-1396-sentry-ignore-expired-signature to master January 13, 2025 12:33
KK-1395.

The ignore_messages option in sentry_sdk would be great, but since the
Graphene raises a general Exception from invalid global ids, we need to
follow the exception message instead of class.
@nikomakela nikomakela force-pushed the KK-1395-sentry-ignore-messages branch from 72042da to b4706a0 Compare January 13, 2025 12:34
@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr461.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is success for https://kukkuu-pr461.api.dev.hel.ninja 😆🎉🎉🎉

@nikomakela
Copy link
Contributor Author

We looked into this 30.1.2025 and there were still some valid sentry issues related to this, so we should keep following the issue and not merge this PR yet.

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.

3 participants