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

Log traceback errors on model #18

Merged
merged 9 commits into from
Mar 1, 2017
Merged

Log traceback errors on model #18

merged 9 commits into from
Mar 1, 2017

Conversation

aschn
Copy link
Owner

@aschn aschn commented Apr 3, 2016

Fixed issue #16

This adds an errors field, and sets it by overwriting the handle_exception method on the view mixin. The response body is unaffected.

@aschn aschn mentioned this pull request Apr 3, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling be1fe2b on handle_500 into * on master*.

@triat
Copy link
Contributor

triat commented Jan 12, 2017

Hey, thanks for the fix. Is it going to be merged soon ?

@avelis
Copy link
Collaborator

avelis commented Jan 12, 2017

@triat It most certainly can. I believe @aschn wanted to be sure this PR actually worked for issue #16 before merging. If it does then I can merge it in. However, that was certainly a long time ago.

@triat
Copy link
Contributor

triat commented Jan 13, 2017

Sure I just wanted to know because we had the error yesterday with a malformation of a JSON causing a 500.

Changes migration file and dependency back to the initial migration file. The project moved forward with a backwards incompatible change and this PR was not updated to reflect that change. All migrations were squashed into the initial migration for simplicity.
Through user error, the indentation of this test method was incorrectly added. Likely, due to merging conflicts from the master branch using the GIthub UI.
@triat
Copy link
Contributor

triat commented Jan 27, 2017

@avelis Can I help you on this one ? as we got the errors too.

@avelis
Copy link
Collaborator

avelis commented Jan 27, 2017

@triat By all means. All contributions are appreciated.

@triat
Copy link
Contributor

triat commented Feb 2, 2017

Hey @avelis, I took the branch on local and tried to pass the tests and they are working on my machine. Could it be possible to rebase the branch from master ? Thanks

edit: ok nvm, I've reproduce the errors (finally got a working tox installation)

@triat
Copy link
Contributor

triat commented Feb 8, 2017

Hey @avelis, do you need an other PR to fix the conflicts or you can do it ? Thanks

@avelis
Copy link
Collaborator

avelis commented Feb 8, 2017

@triat probably need a PR to fix this. Been really tied up at work as of late

@triat
Copy link
Contributor

triat commented Feb 8, 2017

Sure, no problem @avelis. I just created a new PR as I can't fix the conflict here. Hope it helps you. Thanks for your reactivity

Updating the order number of the file and the migration dependency resolves the migration conflict in the PR.
@coveralls
Copy link

coveralls commented Feb 9, 2017

Coverage Status

Coverage increased (+0.003%) to 40.768% when pulling b27bc96 on handle_500 into d4cf08f on master.

2 similar comments
@coveralls
Copy link

coveralls commented Feb 9, 2017

Coverage Status

Coverage increased (+0.003%) to 40.768% when pulling b27bc96 on handle_500 into d4cf08f on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 40.768% when pulling b27bc96 on handle_500 into d4cf08f on master.

@triat
Copy link
Contributor

triat commented Feb 13, 2017

About those tests, I don't know if we should have a look or if it's normal that they are slow sometimes ?

@avelis
Copy link
Collaborator

avelis commented Feb 13, 2017

@triat There is a couple of things we could do. We could simply re-teach to the test to the new time. 42 ms I believe. We could also use flaky but I doubt that will lower the time for those scenario's. Thoughts?

@triat
Copy link
Contributor

triat commented Feb 14, 2017

@avelis I'm not a huge fan of changing the limit time, if your goal is to have an API with a response time with max 200 ms, having drf-tracking take already almost 25% of your response time.

Something that we could do to reduce this amount to 0 is to start using the async in python 3 and / or try to do something similar for python 2 (config with celery?)

@avelis
Copy link
Collaborator

avelis commented Feb 17, 2017

@triat Not against Python 3 async but am concerned with Python 2 and Celery. I like Celery to be honest but am concerned forcing people to use that technology to reduce the middleware's overhead.

@triat
Copy link
Contributor

triat commented Feb 17, 2017

@avelis I'm not talking about forcing them put maybe propose a configuration working with celery if people really do care about their API response time.
The other solution in Python 2 multithreading but from what I heard, it's far from being simple to use

@triat
Copy link
Contributor

triat commented Feb 27, 2017

@avelis Hey, can we check now if the tests pass here too by rebasing on master? Thanks

@avelis
Copy link
Collaborator

avelis commented Feb 27, 2017

@triat I was able to do merge via the Github Desktop UI. We can now observe if the checks pass.

@coveralls
Copy link

coveralls commented Feb 27, 2017

Coverage Status

Coverage increased (+0.03%) to 40.882% when pulling 597585a on handle_500 into b767aa0 on master.

3 similar comments
@coveralls
Copy link

coveralls commented Feb 27, 2017

Coverage Status

Coverage increased (+0.03%) to 40.882% when pulling 597585a on handle_500 into b767aa0 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 40.882% when pulling 597585a on handle_500 into b767aa0 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 40.882% when pulling 597585a on handle_500 into b767aa0 on master.

@triat
Copy link
Contributor

triat commented Feb 28, 2017

@avelis apparently it does 👍

@avelis avelis merged commit 210511e into master Mar 1, 2017
@triat
Copy link
Contributor

triat commented Mar 2, 2017

Hey @avelis, can I just ask you, when you have time, to trigger the build for pipy :) thanks a lot

@avelis avelis deleted the handle_500 branch September 23, 2017 22:12
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.

None yet

4 participants