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

fix(sentry): remove discouraged configureScope #417

Closed namoscato closed 3 years ago

namoscato commented 3 years ago
thinhvo0108 commented 3 years ago

I wonder when this PR can be merged, then another version can be released!

Actually my company is waiting for this, our product is scheduled to be released soon this month. However, we cannot release it without having this conflict with Sentry solved!

If this fix cannot come in time, maybe I will need to fork this repo, then release another hot-fix library to NPM, then use it in the mean time (but I totally don't want to over-complicate things)

Thanks!

thinhvo0108 commented 3 years ago

Seems that we still have the issue, Not on the hub.startSession (fixed by your previous commit @namoscato ) but the hub.captureSession

Thanks for your work!

Screen Shot 2021-02-16 at 13 34 50
thinhvo0108 commented 3 years ago

@namoscato @pcholuj I created another PR for this: https://github.com/filestack/filestack-js/pull/419 (just simply using newer Sentry version v6.1.0 and everything works)

Thanks

lobsterkatie commented 3 years ago

@namoscato - this change is fine, but not necessary in the way that I believe you're thinking about it. (Unless you're trying to use tags or other context with the same keys as are used here, those methods add data rather than overwrite data.)

That said, your change also has the effect of restricting this context data to errors generated by filestack's upload method, rather than all events captured by your Sentry instance. I can see arguments on both sides there, so I won't make a recommendation one way or another.

What I would definitely recommend (especially if you revert to configureScope) is to prefix the key names with filestack-. This prevents any possible conflicts, and also makes much clearer to the eventual viewer of the issue what data they're actually looking at.

(Separately (and this is probably aimed more at @pcholuj), you might add a sentence to the Sentry section of the README along the lines of, "If you're using Sentry to monitor your application, Filestack will automatically report upload errors to Sentry, and tag them with helpful diagnostic information." That makes it clearer that you're not yourself using Sentry to monitor errors in the filestack library, but rather adding onto a user's existing Sentry setup.)

namoscato commented 3 years ago

but not necessary in the way that I believe you're thinking about it

Thanks for dropping in and clearing this up, @lobsterkatie!

Unless you're trying to use tags or other context with the same keys as are used here

This is not the case for my application, but your recommendation to prefix keys in this library sounds like a good one.

That said, your change also has the effect of restricting this context data to errors generated by filestack's upload method, rather than all events captured by your Sentry instance.

This does seem preferable from my perspective (i.e. Filestack is only used in a part of our larger application, and I imagine that's the case for others as well).

@pcholuj – if it's cool with you, I can take another pass at this per the above? Thanks in advance for cooperating with us on this one.