search

MTG / freesound-datasets

A platform for the collaborative creation of open audio collections labeled by humans and based on Freesound content.
https://annotator.freesound.org/
GNU Affero General Public License v3.0
134 stars 12 forks source link

Use sentry-sdk instead of raven #182

Closed alastair closed 3 years ago

alastair commented 4 years ago

This upgrades the use of raven to the new sentry sdk for python and javascript. This also requires a change to asplab-configuration to set a different value for the SENTRY_DSN environment variable. Get it at https://logserver.mtg.upf.edu/settings/sentry/projects/freesound-datasets/keys/, or talk to me during deployment.

I've not tested the javascript side of things, and there are 2 remaining changes to be made: https://github.com/MTG/freesound-datasets/blob/59bf36fe7d6a83aaaa5a42c51ac85e2e8f4eeb45/datasets/templates/datasets/contribute_validate_annotations_category_easy.html#L39

https://github.com/MTG/freesound-datasets/blob/59bf36fe7d6a83aaaa5a42c51ac85e2e8f4eeb45/datasets/templates/datasets/contribute_validate_annotations_category.html#L43

I was unsure about the syntax of these methods, and what to change them to. The method signature for Sentry.captureMessage seems to be different to the Raven one. If we no longer need this capturing in js, we can remove it. Otherwise, we should work out how to set tags and report an exception with the new sdk.

xavierfav commented 4 years ago

Thanks @alastair. I see Travis is having problems starting the web server for testing. Perhaps we need to initiate sentry somehow even when the SENTRY_DSN env variable does not exist? You tested it locally on your computer and it worked?

For the front, we can just drop it for now.

alastair commented 4 years ago

Huh, that's weird. I had hoped that the if SENTRY_DSN: check would skip initialising sentry if not needed. In this case, if the environment variable isn't set, the variable will be set to None. I'll double-check.

Removing frontend would be the absolute quickest way to get this merged. I'll leave the decision up to you, because you know how important the functionality is. If you want it removed, tell me and I'll do it.

alastair commented 4 years ago

I removed the javascript sentry code. I also fixed a bug that was related to the loading of the sentry DSN in settings.

The tests are still failing, but this is due to not being able to find the database container when running the tests. Do you know why this might be?

alastair commented 4 years ago

ah, the issue is this, from latest postgres image:

db_1           | Error: Database is uninitialized and superuser password is not specified.
db_1           |        You must specify POSTGRES_PASSWORD to a non-empty value for the
db_1           |        superuser. For example, "-e POSTGRES_PASSWORD=password" on "docker run".
alastair commented 4 years ago

fixed!

xavierfav commented 4 years ago

Hum, I think I had a similar bug for postgres in travis in another project. I think I fixed it by setting a password in the database url in the env variables, i.e. DJANGO_DATABASE_URL=postgres://postgres:password@db/freesound_datasets

Your commit for sharing the history seems very nice xD

Thanks for the PR! I will have a closer look soon.

alastair commented 4 years ago

the docker image for postgres was recently updated to require either a password to be set, or to explicitly disable the password check. It didn't used to be like this. The change that I made was just a quick fix for development. Take a look at the docs on docker hub for postgres to see better ways to do it.