search

freelawproject / doctor

A microservice for document conversion at scale
https://free.law/projects/doctor
BSD 2-Clause "Simplified" License
54 stars 14 forks source link

138 Added sentry to doctor #142

Closed albertisfu closed 2 years ago

albertisfu commented 2 years ago

@mlissner added Sentry to Doctor, it was necessary to add environ so that we can retrieve the SENTRY_DSN env variable.

I deleted sentry-sdk[flask] from requirements.txt, I think it was outdated right?

I tested it locally and it worked by adding the env variable when running the container:

docker run -d -p 5050:5050 -e SENTRY_DSN=https://the_sentry_dsn freelawproject/doctor:latest

would this be enough so you can add the SENTRY_DSN as an env variable on Kubernetes?

I had to fork the repository since seems I don't have permission to add a new branch to the original doctor repository. I also couldn't assign reviewers.

mlissner commented 2 years ago

This looks about right. Can you check/update the readme and release notes too please?

Also, what happens when the env isn't set?

A few replies:

I had to fork the repository since seems I don't have permission to add a new branch to the original doctor repository. I also couldn't assign reviewers.

Should be fixed.

I deleted sentry-sdk[flask] from requirements.txt, I think it was outdated right?

Yes.

would this be enough so you can add the SENTRY_DSN as an env variable on Kubernetes?

Yes.

albertisfu commented 2 years ago

Thanks! I've updated the Readme and Changelog.

Also, what happens when the env isn't set?

The sentry env is retrieved as follows:

SENTRY_DSN = env("SENTRY_DSN", default="")

So if it's not set it will be empty. Also, there is an if statement if SENTRY_DSN: so that if no SENTRY_DSN env is set no events would be sent to Sentry.

albertisfu commented 2 years ago

Thanks, I've fixed the mistake. Seems that merging needs your approval.