search

filestack / filestack-js

Official Javascript SDK for the Filestack API and content ingestion system.
https://www.filestack.com
MIT License
206 stars 77 forks source link

Conflicting Sentry integration #416

Closed namoscato closed 3 years ago

namoscato commented 3 years ago

The way in which Sentry is integrated into this project is conflicting with our React application's Sentry configuration.

I think there are at least two issues, which might be related:

  1. This scope configuration seems to be overriding all of our application's Sentry context as described in https://github.com/getsentry/sentry-javascript/issues/3206.

    As mentioned in @sentry/minimal's usage documentation:

    it is discouraged to interfere with the event context. If for some reason your library needs to inject context information, beware that this might override the user's context values

    Is this scope configuration necessary? Would it be possible to add configuration to disable this behavior?

  2. Your dependency on @sentry/minimal@5.12.0 makes it impossible for us to upgrade to Sentry v6 as described in https://github.com/getsentry/sentry-javascript/issues/3234#issuecomment-772968903.

    While this is a known challenge with competing Sentry instances on a page, would it be possible to upgrade @sentry/minimal to at least v5.27.0?

I'm happy to help out with both of the issues above. Let me know what you think, and thanks in advance for your cooperation!

thinhvo0108 commented 3 years ago

I have exactly the same problem here (on using the combination Sentry + FileStack) It has been a week, any update so far?

namoscato commented 3 years ago

@thinhvo0108 – glad to hear I'm not the only one experiencing this issue.

https://github.com/filestack/filestack-js/pull/417 takes a pass that hopefully mitigates the first context issue, but I am unable to easily test that change. Hopefully we can get that merged soon, and then I was planning to followup with the @sentry/minimal dependency upgrade.

Pinging @pcholuj who seems to be familiar with the Sentry integration in this project (https://github.com/filestack/filestack-js/commit/5a53ddd75db9c41396b1c2d9c23dab5784ca2419, https://github.com/filestack/filestack-js/pull/264, https://github.com/filestack/filestack-js/pull/255)

thinhvo0108 commented 3 years ago

Thanks @namoscato , your PR is great, however there was still another issue (I already commented on your PR: https://github.com/filestack/filestack-js/pull/417#issuecomment-779575378)

I also created another PR, I increased the Sentry version: https://github.com/filestack/filestack-js/pull/419

namoscato commented 3 years ago

I also created another PR, I increased the Sentry version: #419

Thanks! Yeah, I figured that would have to happen as well.

lobsterkatie commented 3 years ago

FWIW, #419 is really the key here. The context stuff is really a separate issue. See https://github.com/filestack/filestack-js/pull/417#issuecomment-780072317 and https://github.com/getsentry/sentry-javascript/issues/3206#issuecomment-780026119.

pcholuj commented 3 years ago

Hi, this one will be added to next release

mmahalwy commented 3 years ago

@pcholuj any idea when the next release will be? :)

pcholuj commented 3 years ago

We are planning the release next week

mintplo commented 3 years ago

@pcholuj the release has postponed? :)

pcholuj commented 3 years ago

Hi, @mintplo yes we have to postpone it to Monday. Sorry for inconvenience

pcholuj commented 3 years ago

Update for this was released yesterday, can you confirm that its working ok now ?

mintplo commented 3 years ago

Update for this was released yesterday, can you confirm that its working ok now ?

Thanks for notification :) It works well!

namoscato commented 3 years ago

That resolved the issues on our end as well! Thanks for the release, @pcholuj.

lobsterkatie commented 3 years ago

Glad it's working now!