Skip to content

Conversation

@DiegoTavares
Copy link
Collaborator

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.

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.
@DiegoTavares DiegoTavares marked this pull request as draft July 25, 2024 23:22
@DiegoTavares DiegoTavares self-assigned this Jul 25, 2024
@DiegoTavares DiegoTavares marked this pull request as ready for review July 25, 2024 23:43
Copy link
Collaborator

@ramonfigueiredo ramonfigueiredo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DiegoTavares

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

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

@lithorus
Copy link
Collaborator

Isn't there a better way to have the sentry-sdk added than in the main requirements.txt? I sorta have the same problem with the loki integration. I don't think it should be in requirements.txt. Perhaps an idea to split up requirements.txt a bit?

@ramonfigueiredo
Copy link
Collaborator

Isn't there a better way to have the sentry-sdk added than in the main requirements.txt? I sorta have the same problem with the loki integration. I don't think it should be in requirements.txt. Perhaps an idea to split up requirements.txt a bit?

Good point, @lithorus ! I thought about that yesterday as well.

My suggestion to you and Diego is to split the requirements.txt file into more files for optional dependencies. This can help keep the main dependencies clean and manage optional integrations like Sentry and Loki more effectively. This approach allows users to install only the dependencies they need. The same approach is currently applied to the CueGUI dependencies, using the extra requirements_gui.txt.

@DiegoTavares and @lithorus

I recommend creating separate files for optional dependencies like requirements-sentry.txt and requirements-loki.txt.

@DiegoTavares

# requirements-sentry.txt
sentry-sdk==2.11.0
  1. Update the documentation (opencue.io) and README.md file to inform users how to install these optional dependencies if they need them.

Example:

## Optional dependencies

### Sentry Integration
To enable Sentry support, install the additional requirements:
```bash
pip install -r requirements-sentry.txt

@DiegoTavares
Copy link
Collaborator Author

Isn't there a better way to have the sentry-sdk added than in the main requirements.txt? I sorta have the same problem with the loki integration. I don't think it should be in requirements.txt. Perhaps an idea to split up requirements.txt a bit?

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:

  • we'll have to update docs, cicd pipeline and dependency monitor every time a new dependency file is added;
  • dependency conflicts will be harder to catch;
  • there's not really a big difference for users, as they could simply comment out dependencies on the optional list instead of having to run multiple pip install commands
  • If users have already generated their packages and installed with optional features disabled and want to enable them, they would have to re-generate the package and reinstall to get the optional dependencies, instead of simply changing a config file to activate the new feature.
  • when/if we get to the point of generating a pip package on a public repo, we would need all the optional dependencies listed as mandatory anyways.

@lithorus
Copy link
Collaborator

lithorus commented Jul 29, 2024

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? :
requirements.txt (Core libraries used by outline and opencue modules)
requirements_rqd.txt (Additional used by rqd)
requirements_gui.txt (Additional used by cuegui and cuesubmit)

If we also use this as sort of a dependency tree (rqd can depend on things in opencue, but not the other way), we can centralize some of the functions, like the loki/sentry functions or a centralized way of handling config files.

It might even make testing more straightforward, since we have a clear sequence of when modules are tested/installed.

Edit:
Either way it makes for a great topic on next meeting :)

@DiegoTavares
Copy link
Collaborator Author

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? : requirements.txt (Core libraries used by outline and opencue modules) requirements_rqd.txt (Additional used by rqd) requirements_gui.txt (Additional used by cuegui and cuesubmit)

If we also use this as sort of a dependency tree (rqd can depend on things in opencue, but not the other way), we can centralize some of the functions, like the loki/sentry functions or a centralized way of handling config files.

It might even make testing more straightforward, since we have a clear sequence of when modules are tested/installed.

Edit: Either way it makes for a great topic on next meeting :)

Sounds reasonable to me. Let's discuss this further on Wednesday on the TSC meeting.

@DiegoTavares DiegoTavares merged commit 96cc3a3 into AcademySoftwareFoundation:master Aug 7, 2024
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.

3 participants