Rq: The future of RQ and Sentry

[untitaker]

Hi, I'm an employee of Sentry.

Our new SDK for Sentry will soon have a RQ integration released. I wonder if it makes sense to deprecate the integration in RQ itself, as it's effectively unmaintained anyway

[selwin]

Would be happy to :). Is there any docs about this new integration? The only thing I can find on docs.sentry.io is https://docs.sentry.io/clients/python/integrations/rq/ which is already deprecated.

[untitaker]

It's not released yet

[untitaker]

If there's consensus for this within RQ's core dev group I'll prepare a PR to add a similar deprecation notice to RQ's docs as soon as I get around to releasing the new integration

[selwin]

No objections from me. @samuelcolvin any thoughts?

With the new Sentry SDK, can we keep the API the same at rqworker --sentry-dsn="https://<key>:<secret>@sentry.io/<project>"?

[untitaker]

I would not keep that API. The idea is that you have to provide the DSN once, for both webserver and workers, in your call to sentry_sdk.init

I would really just have a long deprecation period like the Raven SDK itself does

[untitaker]

Docs here: https://docs.sentry.io/platforms/python/rq/

[untitaker]

I opened a PR at https://github.com/rq/rq/pull/1005

[selwin]

I read the docs you provided. If we were to deprecate the --sentry-dsn argument, the setup would be a bit more complicated in that users would need to add this bit of code in their config file (docs here).

import sentry_sdk
sentry_sdk.init("https://<key>@sentry.io/<project>")

Wouldn't it be better if we directly call that bit of code in worker.py if --sentry-dsn is passed in? If for some reason this is discouraged, could you please add this workaround in your pull request?

I have another question, does Sentry's new SDK work well with RQ's default Worker class? I'm asking because Raven's async transport doesn't work reliably with with RQ's default Worker class.

[untitaker]

@selwin You are going to call this for your webapp anyway, which I assume shares its config code with the workers. The expectation is that most code will look like this:

sentry_sdk.init(DSN, integrations=[RqIntegration(), FlaskIntegration(), ...])

We'd like to have this interface consistent across integrations. What exactly should I add to the PR?

I will look into fixing the transport issue but we want to avoid having to add multiple transports to the new SDK

[selwin]

While I'm sure a good portion of RQ users are web developers, there are cases where RQ is not used with a web framework (not sure how big of a percentage this is).

I think the best way forward would be to:

1. Support --sentry-dsn for RQ's worker script, this would keep backward compatibility for existing RQ users
2. Update the docs in https://github.com/rq/rq/blob/master/docs/patterns/sentry.md to note that Django and Flask users should follow integration instructions on https://docs.sentry.io/platforms/python/
3. Deprecate --sentry-dsn on https://github.com/rq/django-rq and https://github.com/rq/Flask-RQ2

What do you think?

[untitaker]

Switching --sentry-dsn to the new SDK is not backwards compatible. Users have to at least change their requirements.txt to install the new SDK.

I would just keep the option as-is. If you want to use RQ without a webframework you can still do that, just drop the init call in any python code. It just was my strongest argument for not having multiple ways to configure the DSN.

[samuelcolvin]

broadly speaking, sounds good from my point of view.

Regarding setting the DSN, I'd always prefer to use environment variables than a command line switch, I assume that would be possible too?

[untitaker]

Yes, SENTRY_DSN is also read. But the init call has to be there regardless (just without a DSN)

[samuelcolvin]

Would be useful if we could include extra information relevant to rq.

eg.:

• queue name
• job function name
• job id (some of us use meaningful job ids )
• time since the job started (if it's trivially available, I think it is)
• job queue time (if it's trivially available, I think it is)

[untitaker]

@samuelcolvin right... except for the timestamps it's all in the new integration. I am not sure if this is relevant to this discussion

[untitaker]

So what's the status here? I don't know what exactly you want me to fix in the pr

On October 24, 2018 2:41:53 PM GMT+02:00, Samuel Colvin [e-mail] wrote:

Would be useful if we could include extra information relevant to rq.

eg.:

• queue name
• job function name
• job id (some of us use meaningful job ids )
• time since the job started (if it's trivially available, I think it
  is)
• job queue time (if it's trivially available, I think it is)

--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
https://github.com/rq/rq/issues/1003#issuecomment-432638952

[selwin]

I understand @untitaker 's argument about Sentry not wanting to have multiple points of integration.

However, from RQ's point of view, removing --sentry-dsn is a step backwards in usability; this makes sending job exceptions to Sentry harder for our users.

Let's just switch our --sentry-dsn implementation to use Sentry's new SDK in version 1.0 I'll try my best to publish version 1.0 before Christmas.

[untitaker]

removing --sentry-dsn is a step backwards in usability;

You haven't told us why. I assume it's your personal preference to have it as a command-line argument.

Retaining this option makes usage across e.g. webframeworks and RQ inconsistent. Inconsistency in an API is a step backwards in usability, as is having multiple ways to do the same thing. RQ would be the only library in the Python ecosystem that would initialize the SDK instead of having the user initialize it in their code.

Modifying RQ's sentry module to use the new SDK will break backwards compatibilty, so I really don't know what you're trying to gain by this. If you go ahead with this anyway I don't think we'll recommend this option in our own docs anymore. After all it's a level of indirection that we would have to provide support for.

[threewordphrase]

rq should not read SENTRY_DSN from the environment. We just tried to implement Sentry for services running as rq tasks - but rq is using the old style (raven) and rq is not even installing raven, so we got an import error - even though our code was correctly instrumented with sentry-sdk, actually passing SENTRY_DSN to our code caused it to break.

Bottom line: rq's reading of a third-party environment variable and importing stuff it doesn't install should be considered harmful.

--sentry-dsn is explicit and will not cause surprising side effects. Read your own environment variables if you must.

[untitaker]

@crypticsymbols I think you misunderstood how this works. SENTRY_DSN is a fallback. You can also set the dsn explicitly in your code to disable the fallback. The envvar also does not enable the sdk by itself. That was never up for discussion.

Apart from that reading from the envvar is very useful behavior once you explicitly enabled the sdk. I think you are the only person I know with a different opinion.

[untitaker]

Okay, so you're talking about the current behavior. This thread is about the new Sdk which is something entirely different.

On November 2, 2018 12:45:35 AM GMT+01:00, Aaron Smith [e-mail] wrote:

@untitaker no, I don't misunderstand. We did not pass --sentry-dsn,
we only set the environment variable. We had no idea Sentry was even
integrated with rq.

--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/rq/rq/issues/1003#issuecomment-435224071

[threewordphrase]

@untitaker we did not set --sentry-dsn at all. We did not even know rq had sentry integration until setting SENTRY_DSN in the environment caused rq to fail with import errors.

At the very least, install raven as a dependency by default. But this is extremely unexpected behavior and packages should not read the same env vars as third-party packages.

[untitaker]

Again, this thread is about the new sdk which is a rewrite of raven, is no longer called raven and does not exhibit the behavior you are complaining about.

[threewordphrase]

@untitaker I commented as a response to removing the --sentry-dsn option - It is a step backwards.

[untitaker]

Please read this page: https://docs.sentry.io/platforms/python/rq/ I am proposing removing all sentry specific bits from rq as the new sdk supports rq by itself.

Again, the new sdk does not enable by itself when the envvar is set. You have to call init explicitly.

[untitaker]

To be clear, that includes the CLI option, because it is part of rq.

[threewordphrase]

@untitaker thanks for the info. My only concern is the unexpected side effects from the env var - it sounds like that's being addressed.

[untitaker]

I just read the source code of rq and wasn't aware of this current behavior. One more reason to kill it but it might make the depreciation phase more difficult (since rq will just silently ignore the dsn envvar in current setups if we just remove the logic)

[untitaker]

FYI, we will update the docs to recommend disabling the RQ integration explicitly: https://github.com/getsentry/sentry-docs/pull/565

Otherwise RQ will attempt to import raven and set it up (next to the new SDK), just because SENTRY_DSN is in the env.

[selwin]

Sorry for the late reply.

removing --sentry-dsn is a step backwards in usability;

You haven't told us why. I assume it's your personal preference to have it as a command-line argument.

Actually, in RQ, not only you can pass --sentry-dsn via the cli, you can also:

• Declare SENTRY_DSN in RQ's config file
• Pass it via environment variables

As to why, I think specifying SENTRY_DSN = my_dsn is easier than this and it allows the configuration to be specified via CLI and environment variables:

import sentry_sdk
from sentry_sdk.integrations.rq import RqIntegration
sentry_sdk.init("___PUBLIC_DSN___", integrations=[RqIntegration()])

Retaining this option makes usage across e.g. webframeworks and RQ inconsistent. Inconsistency in an API is a step backwards in usability, as is having multiple ways to do the same thing.

I disagree :). I think having the options to do something via the cli, a config file and environment variables serve different use cases. Many good frameworks and libraries allow you to do this.

RQ would be the only library in the Python ecosystem that would initialize the SDK instead of having the user initialize it in their code.

That's really too bad. I think you should aim for more libraries to support your SDK directly, it will be better for your users. Django, for example is very well supported and libraries like Gunicorn provide direct integration with Django. As a user of both libraries, I'm very happy with that.

Again, this is just my opinion and you may disagree with it but having broad ecosystem support will be good for Sentry as a business (it can't be bad, at the very least :).

Modifying RQ's sentry module to use the new SDK will break backwards compatibilty, so I really don't know what you're trying to gain by this.

My goal is to keep RQ - Sentry integration as easy as it currently is. Mind you, this backwards incompatibility is caused by Sentry's decision to discontinue support for Raven and introduce a new SDK ;)

If you go ahead with this anyway I don't think we'll recommend this option in our own docs anymore.

I really can't dictate what you recommend or not recommend in your own docs. I just try to make things easier for our users.

After all it's a level of indirection that we would have to provide support for.

Well... yes :). If you write a documented, public API, the community would expect a responsible, commercial developer to provide support for that API.

In a nutshell, I love Sentry and being a user of both RQ and Sentry, I want them both to play well with each other.

What RQ as a library has failed to do is to update Sentry's integration up to date so I understand your frustration.

I'd welcome any help in moving RQ to the new Sentry SDK. If we're worried about the breaking backward compatibility, perhaps we can use a new variable called SENTRY_SDK_DSN or something along those lines.

My schedule for the next few weeks is slammed so I may be slow in replying. Sorry about that.

[untitaker]

I would suggest we talk about this again as soon as you have played with the new SDK a bit, because claims like "specifying a envvar is easier than writing this piece of code" seem to assume things about the new SDK that are just not true. You can call init() without arguments and it will read from the envvar all the same.

I don't want to dictate what RQ does, but I would like to make it clear to users that --sentry-dsn is not something we provide support for. I would propose we at least add a link to sentry-sdk's own integration from the RQ docs, and say that they're mutually exclusive. I think that is something we can agree on in the meantime.

Mind you, this backwards incompatibility is caused by Sentry's decision to discontinue support for Raven and introduce a new SDK ;)

Right now the main reason I want to get rid of RQ's own glue code is because it does not peacefully coexist with what the new SDK offers, if you check out the PR I linked above.

I would claim that RQ is not forwards compatible by trying to import random modules based on whether some envvar is set. You have a user complaining in the same thread about that exact behavior.

[untitaker]

Again, this is just my opinion and you may disagree with it but having broad ecosystem support will be good for Sentry as a business (it can't be bad, at the very least :).

Two things:

• Implying the only valid definition of "ecosystem support" is to embed the SDK into the upstream framework is disingenuous
• What actually happens is that bugreports will end up upstream, the work of supporting the Sentry integration is outsourced to people who are not paid to work on it and as a consequence the integration will be the bare minimum of what it could be. Like in the case of RQ.

[selwin]

I would suggest we talk about this again as soon as you have played with the new SDK a bit, because claims like "specifying a envvar is easier than writing this piece of code" seem to assume things about the new SDK that are just not true. You can call init() without arguments and it will read from the envvar all the same.

Correct me if I'm wrong, so if I want to send errors to Sentry, I'd have to:

1. Specify SENTRY_DSN in my environment variables
2. Put this code somewhere in your Python code:

import sentry_sdk
from sentry_sdk.integrations.rq import RqIntegration
sentry_sdk.init(integrations=[RqIntegration()])

This looks harder than just specifying SENTRY_DSN via env var or cli argument, no? Please let me know if I'm mistaken or misunderstood what you meant.

I don't want to dictate what RQ does, but I would like to make it clear to users that --sentry-dsn is not something we provide support for.

You can put this in your own docs. I'm not sure where you get the impression that we imply this is something that Sentry supports. We simply use your library's public APIs, the same way we use redis-py and click's. that If we put a disclaimer like this for Sentry on our docs, then we'll need to add similar disclaimers to other libraries we rely on.

Bottom line is, if any of the libraries we use make breaking changes, it would be on us to fix it (e.g the recent release of redis-py 3 breaks RQ). Would it be nice to get upstream help to fix this? Yes. Are they obligated to do this? Absolutely not (at least that's what I think).

I would propose we at least add a link to sentry-sdk's own integration from the RQ docs, and say that they're mutually exclusive. I think that is something we can agree on in the meantime.

This is something we can do. The easiest way to integrate RQ with Sentry is still the existing, documented method so that's what the docs recommend. But if they want to use Sentry's new SDK, they can follow the instructions on Sentry's docs, until support for Sentry SDK lands in our code.

Right now the main reason I want to get rid of RQ's own glue code is because it does not peacefully coexist with what the new SDK offers, if you check out the PR I linked above.

I don't think removing a feature is the right solution to a bug that occurs when a third party library upgrade their SDK. This glue code makes it easier for our users to report errors to Sentry.

For instance, a sizable chunk of your SDK is glue code, including RQ integration. You wrote all these glue code because you want to make life easier for your users, right?

When this proposal to refactor our worker code lands, your glue code may break. If this happens and I ask you to remove your glue code, would you agree to that?

I would claim that RQ is not forwards compatible by trying to import random modules based on whether some envvar is set. You have a user complaining in the same thread about that exact behavior.

Then we'll make changes to make it better and more predictable, for instance to RQ_SENTRY_DSN. I wish this was used when support for Sentry was pulled in, but such is life.

Implying the only valid definition of "ecosystem support" is to embed the SDK into the upstream framework is disingenuous.

Sorry about that, I didn't mean to be disingenuous. I'm simply referring to your statement here:

RQ would be the only library in the Python ecosystem that would initialize the SDK instead of having the user initialize it in their code.

I repeat, it's too bad that no other library tries to make Sentry integration easier. Personally, I'd love my work to be used by many other people.

I'd love to be able to put DJANGO_SENTRY_DSN env var in my production server to have my Django app reporting errors to Sentry. You may feel differently, but that's ok.

What actually happens is that bugreports will end up upstream, the work of supporting the Sentry integration is outsourced to people who are not paid to work on it and as a consequence the integration will be the bare minimum of what it could be. Like in the case of RQ.

Yes, I'm spending time on this integration and I'm not paid for this. Maintaining this library is a labor of love for me, I just wish I have more time doing this. In my experience, if you maintain a library that's used by other people, they will report bugs to you. Sometimes it's your fault, sometimes it's not.

If you feel that you're unfairly inconvenienced by having to triage bug reports from mistakes that aren't yours, perhaps you should consider finding a different industry or role to work in.

I'd love to improve the state of Sentry integration in RQ. I'd probably get to it at some point, since I'm largely done with preparing for the next big release of RQ. If you're interested in helping out, it will be greatly appreciated.

To sum up this long thread:

• I'm not going to remove Sentry integration from RQ
• Do I think RQ's integration with Sentry's SDK can be improved? Definitely
• I created an issue that outlines what I think is the best path forward. Please feel free to comment on it here: https://github.com/rq/rq/issues/1017

[untitaker]

If you feel that you're unfairly inconvenienced by having to triage bug reports from mistakes that aren't yours, perhaps you should consider finding a different industry or role to work in.

I meant to argue that the glue code to integrate <SaaS> with <framework> should be maintained by the entity who actually profits from this integration, if possible. It's the right thing to do. I think Sentry was wrong in linking the RQ integration on their marketing pages without actually putting their own employees to work on the thing they were advertising. If you're not bothered by this that's great, but it is still not a good situation IMO.

The future plan at #1017 is reasonable, thanks for that. I would still prefer to have the integration removed and would also like to see other RQ maintainers chime in on that.

[gaffney]

Correct me if I'm wrong, so if I want to send errors to Sentry, I'd have to:

Specify SENTRY_DSN in my environment variables
Put this code somewhere in your Python code:
import sentry_sdk
from sentry_sdk.integrations.rq import RqIntegration
sentry_sdk.init(integrations=[RqIntegration()])

This looks harder than just specifying SENTRY_DSN via env var or cli argument, no? Please let me know if I'm mistaken or misunderstood what you meant.

I know I am a rando coming into this but I just wanted to raise a concern --

What if a service has Sentry enabled but wants to report RQ errors to a different system? Or you have multiple Sentry projects - one for your public APIs and another for your tasks?

I strongly prefer explicitly registering plugins rather than other libraries magically picking up environment variables. The code above is succinct and explicit - we are really talking only about the addition of one line - from sentry_sdk.integrations.rq import RqIntegration, which helps with readability and understandability just from skimming the top of the file.

At a higher level it feels more conceptually clean that since Sentry is the _consumer_, either they or some OSS contributor would be building plugins for the downstream library. Arrows all point in one direction - no confusion or conflicts.

Would also love to hear the opinions of other RQ maintainers.

[selwin]

Fixed in https://github.com/rq/rq/pull/1045

[selwin]

Docs here: https://python-rq.org/patterns/sentry/

[untitaker]

@gaffney I think the usecase you brought up is fixed by RQ using a separate environment variable for picking up the DSN. So RQ 1.0 is a good compromise in my opinion. I share your aesthetic preferences but it's hard to argue for aesthetics.

I'm still unhappy about the user experience when they start with the RQ way of initializing the Sentry SDK and later realize that they want pass some options to sentry_sdk.init. It means that they will have to start from scratch in initializing the SDK. If RQ also started to have new config and CLI options for passing things to the SDK, it would be worse though.

I think "how to set SDK options" deserves a mention in the docs written by @selwin (just linking to Sentry's docs of initializing the SDK), if you agree I can make a PR.

[selwin]

@untitaker I'd be open for adding a section for configuring Sentry's SDK options in RQ's own docs, in addition to linking to Sentry's own docs.

I'd appreciate it if you could also update the now inaccurate information in Sentry's docs.

Thanks!

[untitaker]

So I want to update the docs for sure (because the text at the bottom is outdated), but I also want to keep the docs applicable to all RQ versions the SDK officially supports. I think given that usage of RQ's initialization and usage of sentry_sdk.init are still mutually exclusive, it wouldn't harm to keep the --sentry-dsn in the code snippet, but I am also open to other ideas.

Instead of writing anything about this in the docs we could also add a runtime check in the Sentry SDK to see if the user happened to use both the RQ initializer code and the direct init call and raise a hard error.

I still need to test the SDK with RQ 1.0 (which it currently doesn't work with), so I will start with all of this on Monday. Let me know your preferences for both docs.

On an unrelated note, what browser and extensions are you using? That second --sentry-dsn seems horribly misrendered.

[selwin]

I can also update the docs on RQ's side to mention that if they want to initialize sentry-sdk with options, they should do something like this in their config file.

import sentry_sdk
from sentry_sdk.integrations.rq import RqIntegration
sentry_sdk.init(integrations=[RqIntegration()])

If RQ also started to have new config and CLI options for passing things to the SDK, it would be worse though.
This is not something I will do. The goal is to make things easy forr the vast majority of people (I assume) who'd just use Sentry's default config.

Instead of writing anything about this in the docs we could also add a runtime check in the Sentry SDK to see if the user happened to use both the RQ initializer code and the direct init call and raise a hard error.

That's a good point, I think you could just attach a sentry_is_enabled attribute to Worker when the SDK is successfully instantiated to indicate that the integration has been successfully performed.

On an unrelated note, what browser and extensions are you using? That second --sentry-dsn seems horribly misrendered.

I use Safari.

[untitaker]

I made a PR for updating the Sentry docs: https://github.com/getsentry/sentry-docs/pull/887

The runtime check is something I'll keep in mind, definetly an option if people run into this more often. Just a bit of work.