-
Notifications
You must be signed in to change notification settings - Fork 2
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
So/review sentry exception logging in the api #1498
So/review sentry exception logging in the api #1498
Conversation
GetIntoTeachingApi/appsettings.json
Outdated
@@ -10,9 +10,18 @@ | |||
"WriteTo": [ | |||
{ | |||
"Name": "Console" | |||
}, | |||
{ | |||
"Name": "File", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this be a local dev setting, rather than in the main app settings? I don't think this will work on production
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this should be removed. Thought I had, leave it with me and I'll sort.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM, tested locally 👍
I've limited the use of the correlation ID to the teacher advisor sign-up endpoint (as discussed) to limit the potential for issues if any problems occur. I've also not been able to best the filter attribute, it is possible but I cannot mock the hangfire context because they've neglected to apply interfaces, or make class methods generic. We could use a library like Typemock but not sure if this will play nicely with the CICD pipeline out of the box.
If theses changes prove stable we can raise a second, follow-on ticket to complete the work to apply this logging behaviour across the remaining endpoints.