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

fix(sentry): remove problematic Sentry integration #488

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

Conversation

namoscato
Copy link
Contributor

@namoscato namoscato commented Aug 6, 2022

Resolves #467
Resolves #482

  • What kind of change does this PR introduce?

    Bug fix and arguably breaking change

  • What is the current behavior? (You can also link to an open issue here)

    The current Sentry integration:

    1. records unhelpful errors (see Improve Sentry exception handling #467)
    2. is incompatible with Sentry's latest version (see Upgrade Sentry to 7.x #482) which will be a recurring problem (see Conflicting Sentry integration #416)
  • What is the new behavior (if this is a feature change)?

    This removes the Sentry integration in favor of users implementing this at their application layer, which seems simpler and more appropriate.

* Use Sentry Breadcrumbs mechanism to log information about occured errors.
* It can override global objects like console, error etc. Defaults to `true`.
*/
useSentryBreadcrumbs?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure how this is used.

This module also needs to be formatted with Prettier, but I minimized the diff for readability.

@namoscato namoscato changed the title Remove problematic Sentry integration fix(sentry): remove problematic Sentry integration Aug 6, 2022
@namoscato
Copy link
Contributor Author

@sebastian-wec, @pcholuj – do you have any thoughts on this change? This is currently blocking our ability to upgrade to Sentry's latest major version.

@BenGale
Copy link

BenGale commented Nov 15, 2022

This seems to be causing us issues too when upgrading to the latest version of sentry.

@ryan-gray-db
Copy link

@johnkoehn Hey, I see this is approved. How do we go about getting this merged?

@johnkoehn
Copy link

No clue @ryan-gray-db, I just approved it for the heck of it. All the engineers at filestack left after the company was acquired. It is a dead product.

@ryan-gray-db
Copy link

No clue @ryan-gray-db, I just approved it for the heck of it. All the engineers at filestack left after the company was acquired. It is a dead product.

Haha, that's good to know... We had no idea. Thank you!

@namoscato
Copy link
Contributor Author

@sethk4783, @Yaminim07, @prem-celestial, @hemanth-3, @RatGabi (with recent activity in this repository) – are you able to help get this change shipped?

@johnkoehn and I have been repeatedly requesting that this pull request be merged for nearly a year. Unfortunately, the delay in addressing this matter has significantly hindered our ability to upgrade Sentry to its latest version across our applications.

@artemgurzhii
Copy link

artemgurzhii commented May 15, 2024

@namoscato I have tried to use your branch in the package.json, but it was throwing some errors, so tried to create my own fork and make it work. in case anybody needs - "filestack-js": "uplisting/filestack-js#chore/remove-sentry", works for me. link - https://github.com/uplisting/filestack-js/tree/chore/remove-sentry

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.

Upgrade Sentry to 7.x Improve Sentry exception handling
5 participants