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

Add initial sentry configuration for API #44

Merged
merged 4 commits into from
Oct 2, 2023
Merged

Conversation

nadzyah
Copy link
Collaborator

@nadzyah nadzyah commented Sep 29, 2023

This PR resolves https://warthogs.atlassian.net/browse/RTW-133 and adds support for Sentry in TO API.

Here is the project for TO API on sentry: https://sentry.is.canonical.com/canonical/test-observer-api/
The changes were done according to this doc since Sentry doesn't support FastAPI specifically: https://sentry.is.canonical.com/canonical/test-observer-api/getting-started/python/

Also, I've updated the link for docker in README since installing it as snap causes issues when it's running with LXD, and we use LXD local development env with our other projects.

UPD: I've also create TO staging API project in sentry: https://sentry.is.canonical.com/canonical/test-observer-api-staging/

@nadzyah nadzyah requested a review from omar-selo September 29, 2023 13:41

SENTRY_DSN = os.getenv("SENTRY_DSN")
if SENTRY_DSN:
sentry_sdk.init(SENTRY_DSN) # type: ignore
Copy link
Collaborator Author

@nadzyah nadzyah Sep 29, 2023

Choose a reason for hiding this comment

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

Just in case, I had to tell mypy to ignore Cannot instantiate abstract class "init" with abstract attribute "__exit__" [abstract] error while initialising sentry client. It's a known bug that was fixed in later versions. But since our self-hosted Sentry instance doesn't support latest clients that include this bug fix, we have to go with this approach.

@omar-selo
Copy link
Collaborator

I'm assuming this resolves the backend part and the frontend is coming in a separate PR?

@nadzyah
Copy link
Collaborator Author

nadzyah commented Sep 29, 2023

I'm assuming this resolves the backend part and the frontend is coming in a separate PR?

Since API and frontend should be tracked separately on sentry (because they use different sentry clients), I've decided to create a PR for API first. I'll create a separate PR for the frontend

Copy link
Collaborator

@omar-selo omar-selo left a comment

Choose a reason for hiding this comment

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

Glad finally got Sentry.

Whenever I setup sentry before there was a way to catch all uncaught errors and send them to sentry. It would be great if we can setup this here.

backend/k8s/api.yaml Outdated Show resolved Hide resolved
backend/test_observer/controllers/router.py Show resolved Hide resolved
backend/test_observer/main.py Show resolved Hide resolved
Copy link
Contributor

@plars plars left a comment

Choose a reason for hiding this comment

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

Agree with Omar + question about if we should add support for injecting it through a variable that we pass to terraform

backend/test_observer/main.py Show resolved Hide resolved
backend/k8s/api.yaml Outdated Show resolved Hide resolved
@nadzyah nadzyah requested review from omar-selo and plars October 2, 2023 08:44
Copy link
Collaborator

@omar-selo omar-selo left a comment

Choose a reason for hiding this comment

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

Good work, just mainly a note on development value of sentry dsn

terraform/test-observer.tf Outdated Show resolved Hide resolved
backend/charm/config.yaml Outdated Show resolved Hide resolved
@nadzyah nadzyah requested a review from omar-selo October 2, 2023 10:04
Copy link
Collaborator

@omar-selo omar-selo left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@plars plars left a comment

Choose a reason for hiding this comment

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

Small change to suggest, which I hope actually makes it a little easier. I don't think we want to hardcode those dsn values, just make them a variable. Otherwise, looking great!

terraform/test-observer.tf Show resolved Hide resolved
Copy link
Contributor

@plars plars left a comment

Choose a reason for hiding this comment

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

After discussing this a bit more on MM, I think it would be easiest to just do this for now, since it wouldn't require setting these DSNs in the environment. I still think that it would be nice to have this as a configurable option in the future, but we could easily change that after we get the ability to deploy all of this in a more modular way. So +1

@nadzyah nadzyah merged commit d5e9f35 into main Oct 2, 2023
@nadzyah nadzyah deleted the sentry-fastapi-support branch October 2, 2023 15:54
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.

3 participants