Use Sentry to report exceptions #26
Conversation
| from landoapi.dockerflow import dockerflow | ||
| from landoapi.models.storage import alembic, db | ||
| from mozlogging import MozLogFormatter | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| sentry = Sentry() |
There was a problem hiding this comment.
n00blyfe™ but why not initialize this in initialize_sentry since it's the only place it's used?
There was a problem hiding this comment.
It is a global, like logger, that can be used in other files to manually handle exceptions 😃
There was a problem hiding this comment.
Good point, will do. I was copying the docs, but you are totally right. :)
There was a problem hiding this comment.
Wait, it is not a bad thing that it is global. That will let us use it whereever we want.
There was a problem hiding this comment.
We can always extract it again if we need it.
landoapi/app.py
Outdated
| version_info = json.load(open(version_path)) | ||
| logger.info(version_info, 'app.version') | ||
|
|
||
| this_app_version = version_info['commit'] |
There was a problem hiding this comment.
Considering that there is a field specifically named 'version' I think we should use that instead of 'commit' even if they are equal right now, they could differ in the future.
There was a problem hiding this comment.
Cool, I'll change it.
landoapi/app.py
Outdated
| logger.info(version_info, 'app.version') | ||
|
|
||
| this_app_version = version_info['commit'] | ||
| environment = version_info['build'] |
There was a problem hiding this comment.
The version.json 'build' doesn't refer to the environment, it refers to the specific cirlce ci build that created this image (see https://circleci.com/docs/1.0/environment-variables/). With that in mind it cannot be used as the environment.
We could create another environment variable ('ENVIRONMENT' or something) and have @ckolos set it properly in dev, staging, and prod. Or we can switch based on the HOST_URL that is already available.
There was a problem hiding this comment.
As I understand it, the environment is one way to partition the set of exceptions thrown by Sentry. Hostname is presumably another? But if you have multiple hosts per environment, like in prod, then partitioning the pool of exceptions coming from all the different environments by hostname might be a pain.
I like the ENVIRONMENT suggestion. I'd also like to know if Ops already has a way they do this. @ckolos, are you aware of any standard-ish way to get the environment label a host lives under into the container data so Sentry can report on it?
There was a problem hiding this comment.
"environment label a host lives under"... ?
You mean like "this is running in dev/stage/prod"? If so, that's available from the env as the ENV var.
There was a problem hiding this comment.
@ckolos perfect, thanks!
landoapi/app.py
Outdated
| "environment" identifier. See the Sentry client configuration docs | ||
| for details. | ||
| """ | ||
| sentry_dsn = os.environ.get('SENTRY_DSN', None) |
There was a problem hiding this comment.
The environment variable 'SENTRY_DSN' should be present in our docker-compose file even if it is empty or just a comment because our docker-compose defines all the environment variables needed to run the application. It is not used in production of course, it is just a good way to remind us what will be in the environment.
There was a problem hiding this comment.
Nice catch, I'll add it.
Does what is says on the tin. Sentry is shut off for local development. To use it you have to give Sentry a real DSN.
I have tested this change using the lando-api Sentry project. It reports errors correctly when they occur in a local development container. I think we can ship this.