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

Add Sentry #43

Merged
merged 2 commits into from
Jan 31, 2022
Merged

Add Sentry #43

merged 2 commits into from
Jan 31, 2022

Conversation

ehanson8
Copy link
Contributor

What does this PR do?

  • Install sentry-sdk dependency
  • Add SENTRY_DSN parameter to config.py
  • Add function to config.py for verification of integration

How can a reviewer manually see the effects of these changes?

Verify that the project has been created for our org on https://sentry.io/ and that the integration works by viewing the previously triggered exception

What are the relevant tickets?

Requires Database Migrations?

NO

Includes new or updated dependencies?

YES

* Install sentry-sdk dependency
* Add SENTRY_DSN parameter to config.py
* Add function to config.py for verification of integration

* https://mitlibraries.atlassian.net/browse/DLSPP-152
awd/config.py Outdated


def check_sentry():
if SENTRY_DSN:
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the intent behind this? If SENTRY_DSN is absent, line 71 duly reports that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is called when you need to check that the actual connection to Sentry is receiving exception. It is just being stored for those situations, this is happening in the alma-scripts and dspace-submission-servicerepos as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not regularly called, it is there in for troubleshooting purposes, see the last comment here MITLibraries/alma-scripts#33

Copy link
Contributor

@osmandin osmandin Jan 31, 2022

Choose a reason for hiding this comment

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

If it is truly important to check Sentry integration, can we throw it explicitly in the smoke test or something similar?

Ideally, the program should not have any non-functional code. This is the same as having commented out code in the program. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Copy link
Contributor

@osmandin osmandin left a comment

Choose a reason for hiding this comment

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

Please refer to the comment. Thanks.

@ehanson8 ehanson8 merged commit 28d52a7 into main Jan 31, 2022
@ehanson8 ehanson8 deleted the DLSPP-152 branch January 31, 2022 17:02
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.

2 participants