search

errbotio / errbot

Errbot is a chatbot, a daemon that connects to your favorite chat service and bring your tools and some fun into the conversation.
http://errbot.io
GNU General Public License v3.0
3.12k stars 612 forks source link

feat: add new config `SENTRY_OPTIONS` for more flexible Sentry configuration #1597

Closed browniebroke closed 1 year ago

browniebroke commented 1 year ago

The Sentry SDK has several options, but right now we are limited by errbot as to which one can be configured.

This adds a new SENTRY_OPTIONS config, which enable users to specify any option supported by the SDK in the sentry_sdk.init() call.

Our use case is that we have 2 instances of a given bot running, one for testing changes to our plugins and one which we use (like staging & prod) and we use the environment option to tell which bot caused the error. I assume some people might find the release option useful too.

sijis commented 1 year ago

Is there a way to consolidate all the sentry options into this new option, basically removing SENTRY_TRANSPORT?

browniebroke commented 1 year ago

Sure, can do that. That would be a breaking change, is it something that we should deprecate first?

sijis commented 1 year ago

@browniebroke that's a fair concern. There is some planned work that may cause other breaks too.

Would you be ok with creating a separate PR with the separate breaking changes?

browniebroke commented 1 year ago

Yes sure :+1:

sijis commented 1 year ago

@browniebroke With the single SENTRY_OPTIONS option now, how could someone specify a different transport?

browniebroke commented 1 year ago

@browniebroke With the single SENTRY_OPTIONS option now, how could someone specify a different transport?

They would specify it as per the Sentry docs, but to be honest, that's not super clear over there. I looked into their tests and I found an example here:

https://github.com/getsentry/sentry-python/blob/6db44a95825245b1f7c9baa54957d044f7be18eb/tests/tracing/test_integration_tests.py#L245-L254

browniebroke commented 1 year ago

I think the transport has been simplified compared to the old Sentry client (Raven) which was providing multiple transport classes: https://docs.sentry.io/clients/python/transports/

The newer client sentry-sdk is lighter on the topic: https://docs.sentry.io/platforms/python/guides/airflow/configuration/options/#transport-options

browniebroke commented 1 year ago

Let me know if I missed anything. I'm not sure it's worth expanding more on this in the docs?

sijis commented 1 year ago

@browniebroke Does the PR https://github.com/errbotio/errbot/pull/1605 superseed this one or does this one need to be merged first before #1605?

browniebroke commented 1 year ago

It builds on top of it, so it's a bit of both.

If it was my project, I would probably merge this first and then merge the other, this way the 2 PR would appear as 2 distinct changes, but it's up to you. Depends how you like to separate things.