-
Notifications
You must be signed in to change notification settings - Fork 233
[rqd] Add sentry support #1433
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
[rqd] Add sentry support #1433
Conversation
Add sentry as an optional integration to collect events from the rqd module. If SENTRY_DSN_PATH is configured on rqd.conf, a sentry collector will be initialized.
ramonfigueiredo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved! Just a minor change!
| CHECK_INTERVAL_LOCKED = 60 | ||
| # Seconds of idle time required before nimby unlocks. | ||
| MINIMUM_IDLE = 900 | ||
| # Url to the rqd project on sentry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be helpful to add a brief explanation of what the Sentry DSN is for users who might not be familiar with Sentry. For example:
# Url to the rqd project on Sentry. This is used to send error and performance data to Sentry.
# SENTRY_DSN_PATH=http://sentry.yourdomain.com/40
|
Isn't there a better way to have the sentry-sdk added than in the main |
Good point, @lithorus ! I thought about that yesterday as well. My suggestion to you and Diego is to split the I recommend creating separate files for optional dependencies like
Example: |
I confess I spend some time pondering the options and I still haven't found one that I'm completely satisfied with. If a new requirements file is added for each new optional dependency as suggested by @ramonfigueiredo we get the benefit of giving users the option to install what they need, but it makes our life harder down the line. Here are my cons:
|
|
All good points, however I would still suggest that some of it was split up a bit more. Some of the modules are only needed for testing, some are only needed for rqd similar to how some only used for the GUI apps. Someting like this perhaps? : If we also use this as sort of a dependency tree ( It might even make testing more straightforward, since we have a clear sequence of when modules are tested/installed. Edit: |
Sounds reasonable to me. Let's discuss this further on Wednesday on the TSC meeting. |
Add sentry as an optional integration to collect events from the rqd module.
If SENTRY_DSN_PATH is configured on rqd.conf, a sentry collector will be initialized.