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

Pass request logger to mailer #208

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Pass request logger to mailer #208

wants to merge 1 commit into from

Conversation

vbrown608
Copy link
Contributor

Instead of creating a new logger, pass a logger based on the current request to the Mailer.

@vbrown608 vbrown608 requested review from a team as code owners April 16, 2020 21:33
Copy link
Member

@mraerino mraerino left a comment

Choose a reason for hiding this comment

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

sadly, this is not what i had in mind.
if you implement it like this, mailme still has a central logger which will not have different fields based on the request.

you need to pass in the logger that you have as part of a request handler, e.g. where the actual email is sent.
this might mean having to change mailme to actually accept a logger in that place.
also, I don't even know if the safe http client will be able to get the logger in that way.

a much better way to do this might be to get the logger from the request context, since that can easily be added to an existing http client, but none if the components in play (safeHTTPClient or mailme) are made for this currentyl

sorry for sending you down this road 🙈
this is a good start though, because it at least adds some service fundamentals to the logline

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.

2 participants