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.

Sign up for GitHub

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

Jump to bottom

feat: improve Sentry logging #3086

Merged
merged 12 commits into from
Jul 30, 2024
Merged

feat: improve Sentry logging #3086

merged 12 commits into from
Jul 30, 2024

Conversation

bolinocroustibat
Copy link
Contributor

@bolinocroustibat bolinocroustibat commented Jul 11, 2024 •
edited
Loading

Closes datagouv/data.gouv.fr#1389:

  • Update Sentry module and fix deps conflicts (see note)
  • Send environment (using SITE_ID) and release info to Sentry
  • Send profiling and traces to Sentry for performance analyzing
  • Add an app-scoped config var SENTRY_SAMPLE_RATE

Note:
This PR adds sentry-sdk[flask] as an install dependency. I'm not sure why it wasn't there before - let me know if this is a mistake!
So since sentry-sdk[flask] depends on urllib3 and urllib3 depends on requests which were incompatible with newer versions of sentry-sdk, both pinned versions of urllib3 and requests have been updated in this PR.

@bolinocroustibat bolinocroustibat marked this pull request as draft July 11, 2024 09:44
@bolinocroustibat bolinocroustibat mentioned this pull request Jul 11, 2024
Closed
@bolinocroustibat bolinocroustibat force-pushed the update-sentry branch 4 times, most recently from 0040602 to 056d103 Compare July 12, 2024 08:50
@bolinocroustibat bolinocroustibat marked this pull request as ready for review July 12, 2024 08:50
@bolinocroustibat bolinocroustibat force-pushed the update-sentry branch 3 times, most recently from f90d792 to 71dc1c7 Compare July 12, 2024 12:32
maudetes
maudetes approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@maudetes maudetes left a comment •
edited
Loading

Choose a reason for hiding this comment

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

sentry.pip disappeared in #2992 somehow.

Thank you! We can probably test it on our dev envs :)

Don't forget to add a changelog entry ;)

udata/sentry.py Outdated
Comment on lines 78 to 82
traces_sample_rate=sentry_sample_rate,
# Experimental profiling
_experiments={
'profiles_sample_rate': sentry_sample_rate,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say that we should log 100% of errors by default for now (and make it configurable) but only a portion of profiling indeed. Ignore this comment if this is what it does ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes after some inquiry, this is only for perf so let's keep as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But: Sentry sample rate has been changed as an app config.

@bolinocroustibat
Copy link
Contributor Author

Thank you! We can probably test it on our dev envs :)

Don't forget to add a changelog entry ;)

Indeed. Added a changelog entry.

@bolinocroustibat bolinocroustibat mentioned this pull request Jul 16, 2024
Merged
@bolinocroustibat bolinocroustibat mentioned this pull request Jul 23, 2024
Merged
maudetes
maudetes reviewed Jul 24, 2024
View reviewed changes
requirements/install.pip
@@ -279,7 +281,7 @@ uritools==4.0.3
# via urlextract
urlextract==0.14.0
# via -r requirements/install.in
urllib3==1.25.11
urllib3==1.26.19
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should compute pip-compile on all impacted dep files.
It should have gotten triggered by the pip-tool pre-commit hook.
I've updated its version in 5982e2a.

@bolinocroustibat bolinocroustibat force-pushed the update-sentry branch 2 times, most recently from 0c27a90 to 6afe805 Compare July 25, 2024 14:55
@bolinocroustibat bolinocroustibat changed the title Improve Sentry logging feat: improve Sentry logging Jul 25, 2024
magopian
magopian approved these changes Jul 26, 2024
View reviewed changes
Copy link
Contributor

@magopian magopian left a comment

Choose a reason for hiding this comment

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

LGTM, nice work 👍

@bolinocroustibat bolinocroustibat merged commit 9065619 into master Jul 30, 2024
1 check passed
@bolinocroustibat bolinocroustibat deleted the update-sentry branch July 30, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maudetes maudetes maudetes approved these changes

@magopian magopian magopian approved these changes

@ThibaudDauce ThibaudDauce Awaiting requested review from ThibaudDauce

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Intégration dans Sentry lors d'un déploiement
3 participants
@bolinocroustibat @magopian @maudetes