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

fixing / warning with sentry error-reporting? #16

Open
syphar opened this issue Jun 19, 2014 · 2 comments
Open

fixing / warning with sentry error-reporting? #16

syphar opened this issue Jun 19, 2014 · 2 comments

Comments

@syphar
Copy link

syphar commented Jun 19, 2014

I just stumbled on a small problem (in my config?)
question is if we should fix this in django-pq, or at least document the behavior.

  • default sentry DSNs (in docs, heroku, ...) are with scheme https
  • since getsentry/raven-python@7a69328 , seems to be in raven 3.6.0, the threaded http transport for the http or https scheme is default.
  • pqworker forks a process to do the job-work
  • the threaded raven transport uses a background thread to send the events to sentry (see the source)
  • our Worker class kills the forked process when the work is done.

So the result is, failed jobs are not sent to sentry.

What should be the best solution to fix this?

  • wait somehow until the background thread is finished?
  • on registering, raise an error when an async transport is used?
  • document the behavior?

edit: changed raven-version where this appeared.

@syphar
Copy link
Author

syphar commented Jun 27, 2014

any idea on this?
In reality, sentry integration is broken for many configurations atm.

I'll be happy to provide a PR if it's clear which solution to follow.

@bretth
Copy link
Owner

bretth commented Jun 30, 2014

I haven't had a chance to fix this, but a quick review suggests to me that changing the way pqworker loads sentry is the best course of action. Instead of using the SENTRY_DSN the pqworker management command should use a new setting PQ_SENTRY_DSN, and contrib.sentry should provide warnings or exception if a threaded transport is used. The rational for sentry to switch to async methods makes sense for the request/response cycle but has no real benefit for background tasks anyway.

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

No branches or pull requests

2 participants