Support error reporting via napari-error-monitor#4040
Support error reporting via napari-error-monitor#4040alisterburt merged 6 commits intonapari:mainfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4040 +/- ##
=======================================
Coverage 83.25% 83.25%
=======================================
Files 588 588
Lines 48517 48542 +25
=======================================
+ Hits 40392 40413 +21
- Misses 8125 8129 +4
Continue to review full report at Codecov.
|
sofroniewn
left a comment
There was a problem hiding this comment.
Wow super exciting! I will give this a spin asap. Flagging for @neuromusic too who has been interested in this as well
DragaDoncila
left a comment
There was a problem hiding this comment.
I just pulled this down and tried it and it seems to work as expected! I totally love it ❤️
The formatting of the example bug report is a bit wonky but I don't know how much that's in your control @tlambert03.
Code looks OK to me but I'm not overly familiar with this section of the codebase.
| pydantic[dotenv] | ||
| check-manifest>=0.42 | ||
| rich | ||
| napari-error-monitor |
There was a problem hiding this comment.
I'm not sure if we want to install this by default? I think at least it should be a big notice to the community across most channels before we do. Even if most people would opt in anyway, I think it'll be nice for them to know it's coming in the next release/once we merge this PR.
There was a problem hiding this comment.
I'm not sure if we want to install this by default?
you might have missed that this is in the dev section not the install requires. so this only applies for (mostly) core developers who clone, and then specifically run pip install -e .[dev] (and even still they need to click yes on the next launch)
The formatting of the example bug report is a bit wonky but I don't know how much that's in your control @tlambert03.
it's an object meant for serialization and transmission over the internet, not so much human consumption. given that it's a one-time thing, I'm not sure the time it would take to "beautify" that bundle is worth it. Really, it's there as a courtesy for those concerned about the type of information that might be transferred... This isn't going to pop up every time an exception occurs
I think it'll be nice for them to know it's coming in the next release/once we merge this PR.
don't forget, nothing is "coming" for anyone unless they manually go seek out the napari-error-monitor package and install it. That doesn't ship with napari, so there are two steps of opt in. 1) decide you care about this and go seek out that plugin, 2) relaunch napari, and specifically click the yes button. otherwise nothing happens at all.
There was a problem hiding this comment.
you might have missed that this is in the dev section
Oops you're right I did miss that it was in dev. That's totally fine then - as long as it doesn't ship with napari. Still would be nice to get some messaging out there that it's available for use.
given that it's a one-time thing, I'm not sure the time it would take to "beautify" that bundle is worth it.
Agreed.
There was a problem hiding this comment.
Still would be nice to get some messaging out there that it's available for use.
yep, once it's in I'd like developers to use it a bit. start getting some bug reports, then once we have a feel for it, and how we can use it, start announcing on image.sc for those who want to help napari out
alisterburt
left a comment
There was a problem hiding this comment.
Very cool - gave it a whirl and everything worked as expected! The example of info sent over and the option to include local variables are really nice touches.
I don't see any reason not to add this as-is and your path to more general usage seems reasonable. Could you add me to the sentry team so I can see what these things look like from their platform?
|
I'm going to merge this now so that it lands in 0.4.14 - will happily revert if anyone objects, cheers! |
|
I think @jni and @sofroniewn wanted this to wait until after 0.4.14... sorry that private zulip conversation didn't make it here |
|
I did comment earlier in the public zulip that I'd like to wait to merge this one https://napari.zulipchat.com/#narrow/stream/215289-release/topic/0.2E4.2E14/near/270628243, but I should have commented here too. I know this is all very opt in, but it really feels like a much bigger conversation is needed before we add. Can we please revert before the release |
This reverts commit 28be576.
Description
I made a package
napari-error-monitorthat will send bug reports to sentry.io. If someone installs that package, this PR will detect it and hook into our sys.excepthook. The first time you run napari with that package installed, an opt-in appears, and you have to click "yes please send error reports...". It also shows you an example event that would be sent for your machine. You can revise your opt in/out later by going to the Help menu.best way to try is probably to
pip install napari-error-monitor, and then pull this PR. I can add any core devs interested to the sentry team to access the reports.edit: just to clarify. All this PR itself does is say:
napari-error-monitorask_opt_in: to execute the opt in processinstall_error_monitor: to install the monitor (this checks for opt in and asks if unknown)capture_exception: actually send the exceptionthis PR/interface makes no assumptions about whether sentry is used, etc... that can all be done elsewhere.
Type of change
References
How has this been tested?
Final checklist:
trans.to make them localizable.For more information see our translations guide.