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

Start using Sentry in our general stack #143

Merged
merged 6 commits into from Jun 19, 2021

Conversation

create-issue-branch[bot]
Copy link
Contributor

@create-issue-branch create-issue-branch bot commented Jun 11, 2021

closes #119
closes #120

@nhoening nhoening marked this pull request as ready for review June 15, 2021 13:07
@nhoening nhoening requested a review from Flix6x June 15, 2021 13:07
@nhoening
Copy link
Contributor

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).

Copy link
Contributor

@Flix6x Flix6x left a 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}",
Copy link
Contributor

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.

Copy link
Contributor

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.

Comment on lines 47 to 51
# 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,
Copy link
Contributor

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.

Copy link
Contributor

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?

@@ -160,6 +160,15 @@ Token which external services can use to check on the status of recurring tasks
Default: ``None``


SENTRY_SDN
Copy link
Contributor

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

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?

Copy link
Contributor

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.

@nhoening nhoening merged commit 24cd1ef into main Jun 19, 2021
@nhoening nhoening deleted the issue-119-Start_using_Sentry_in_our_general_stack branch June 19, 2021 22:33
@Flix6x Flix6x added this to the 0.6.0 milestone Jul 20, 2021
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.

Start using sentry in our job queues Start using Sentry in our general stack
2 participants