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

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

Merged
merged 2 commits into from
Oct 24, 2022

Conversation

browniebroke
Copy link
Contributor

@browniebroke browniebroke commented Sep 2, 2022

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
Copy link
Contributor

sijis commented Sep 3, 2022

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

@browniebroke
Copy link
Contributor Author

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

@sijis
Copy link
Contributor

sijis commented Sep 9, 2022

@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
Copy link
Contributor Author

Yes sure 👍

@sijis
Copy link
Contributor

sijis commented Sep 22, 2022

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

@browniebroke
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@sijis
Copy link
Contributor

sijis commented Oct 20, 2022

@browniebroke Does the PR #1605 superseed this one or does this one need to be merged first before #1605?

@browniebroke
Copy link
Contributor Author

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.

@sijis sijis merged commit e10db92 into errbotio:master Oct 24, 2022
@browniebroke browniebroke deleted the feat/sentry-options branch October 24, 2022 07:39
sijis added a commit to duhow/errbot that referenced this pull request Nov 25, 2022
…uration (errbotio#1597)

* feat: allow specifying any Sentry option

* docs: add info to CHANGES

Co-authored-by: Sijis Aviles <sijis.aviles@gmail.com>
sijis added a commit to sijis/errbot that referenced this pull request Jan 1, 2024
…uration (errbotio#1597)

* feat: allow specifying any Sentry option

* docs: add info to CHANGES

Co-authored-by: Sijis Aviles <sijis.aviles@gmail.com>
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

2 participants