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
Start using Sentry in our general stack #143
Start using Sentry in our general stack #143
Conversation
I have tested errors in the web stack, in CLIs and also in RQ tasks (in a forecasting job), and they all work now (events end up in our 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.
👍 Just minor fixes plus 1 idea for a new config variable.
dsn=sentry_dsn, | ||
integrations=[FlaskIntegration(), RqIntegration()], | ||
debug=app.debug, | ||
release=f"flexmeasures@{get_distribution('flexmeasures').version}", |
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.
Might be useful to also add which plugins are loaded (incl. their version). I'm not sure whether this is the right place to put such info, but this is where the thought came up for me.
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.
Looks like this is already handled in line 146.
flexmeasures/utils/app_utils.py
Outdated
# Set traces_sample_rate to 1.0 to capture 100% | ||
# of transactions for performance monitoring. | ||
# We recommend adjusting this value in production. | ||
# TODO: Decide if we need this and if to configure it. | ||
traces_sample_rate=0.33, |
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.
Looks like a potential FM config setting to me.
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.
Maybe in the form of a default SENTRY_CONFIG: dict = dict(traces_sample_rate=0.33)
so it can easily be expanded with other Sentry arguments in production?
documentation/configuration.rst
Outdated
@@ -160,6 +160,15 @@ Token which external services can use to check on the status of recurring tasks | |||
Default: ``None`` | |||
|
|||
|
|||
SENTRY_SDN |
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.
Typo: DSN instead of SDN.
@@ -48,6 +48,7 @@ Flask-Security-Too>=4.0 | |||
Flask-Classful | |||
Flask-Marshmallow | |||
Flask-Cors | |||
sentry-sdk[flask] |
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.
Can you comment on this notation?
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.
This is the common syntax for libraries that also install to load certain extensions. In this case, Flask is the extension. As it is rather standard, I'd like to not add a comment.
closes #119
closes #120