-
Notifications
You must be signed in to change notification settings - Fork 72.8k
Log error on device status insert & don't crash #3751
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
Conversation
|
Is there a good way to surface this to users? |
|
I think we should still return a 500 so the client deal with it. Is this mostly for device status? I was wondering it we should rename the old collection on startup if it's too big, then start using a new collection and ignore the old. |
|
At least for mLab, looks like all inserts to all collections start failing at the same time, so it’s not specific to the device status collection, other than it being the collection that seems to get the most data. |
|
@jasoncalabrese so, instead of just returning, throw the error? |
|
@jasoncalabrese @sulkaharo |
|
What's the behavior when the db is full "crashing heroku" sounds extreme... I think to return a 500, we just need to call the callback passing an error something like |
PieterGit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope either @jpcunningh or @jasoncalabrese can fix the PR so that it does what @jasoncalabrese suggest of returning a 500 (and logging info the console log). I don't get how this code works, so I would rather not touch it 😀 . Quite some people on Gitter have problems with full Mongo databases, so it would be good that for 0.11 we make sure we log it and return a 500 error to the uploader.
|
@jasoncalabrese, the app crashes when the database fails to insert any records due to a null dereference. The only way to recover is to free up space through either the Heroku dashboard link to mLab, clearing a whole collection, or some means other than Nightscout admin tools. @sulkaharo, is it best if I submit a PR to your account's |
|
@jpcunningh I don't mind if we PR this with this PR or with #3989 . I assume @jasoncalabrese and @sulkaharo are quite busy at this moment, because I haven't seen them a lot the last few weeks. I will accept any tested PR that will log an error and return 500 to the client 😄 |
|
@PieterGit, I created a new PR #4004 that returns 500 to the client. |
|
Merged #4004 instead. Closing this PR |
(Don't merge yet!)