search

cloudfoundry / notifications

The notifications service component of Cloud Foundry
Apache License 2.0
17 stars 22 forks source link

Delivery Handler: Add extra logging, and attach delivery handler to the job struct to handle exceptions. #30

Closed jamespollard8 closed 5 years ago

cfdreddbot commented 5 years ago

:white_check_mark: Hey jamespollard8! The commit authors and yourself have already signed the CLA.

osis commented 5 years ago

I'm a bit hesitant to merge do to the fact that this might put sensitive/private data in the logs.

Do you have an example of what the logs look like when this error occurs @jamespollard8?

niall-byrne commented 5 years ago

@osis I have an example here:

2019/06/18 17:59:52 Error: Could not unmarshal &{ID:1 WorkerID:worker-4-3516 Payload: Version:4 RetryCount:1 ActiveAt:2019-06-18 17:59:52.245624 -0400 EDT m=+61.312760603 ShouldRetry:false}

Other log messages will contain the contents of the incoming email template from newrelic, showing where the JSON is truncated: {"MessageID":"b1e0bfc6-2c57-e884-0e56-7731e12a1605","Options":{"ReplyTo":"Holly from New Relic \u003cholly.ross@new-relic-121baf9687a1.intercom-mail.com\u003e","Subject":"Never Stop Learning: More New Relic Resources for you","KindDescription":"","SourceDescription":"3rd Party Services","Text":"[Image \"LightBannerEmail.png\"]\n\n Hi Space, \n You’re on a quest to gain the skills and knowledge you need to deliver value to your organization. And the Explorers Hub Community is here to connect you with the community members and New Relic staff who will help you on that journey!  \n With that goal in mind here are some great places to get started: \n - Onboarding - Learn New Relic with others who are just getting started. \n- Get Certified - Our Certified Performance Pro program will help you build the foundation to become an expert New Relic user. \n- Level Up: Relic Solutions - Enjoy posts created by us, for you. \n- Best Practice Guides - Our best tips and tricks for making the most of New Relic. \n As you explore, if you see something helpful like it! [Image \"RedHeart.png\"] This lets others in the community know when content is valuable and they might want to take a closer

It's my belief that there is no PI being produced by these email templates.

osis commented 5 years ago

@niall-byrne @jamespollard8 Thanks for the example. I really like this level of visibility in the logs but I do think it's too much of a rick to output the custom templates into the log stream default.

I think to debug this properly we would want to introduce/use log levels that help us protect against the amount of visibility a user wants with the service.

The first debug message is definitely okay to merge in. And there should be an active investigation around what failing gracefully looks like in this situation.

niall-byrne commented 5 years ago

@osis I have removed the additional logging. Please give this another look.

osis commented 5 years ago

@jamespollard8 @niall-byrne Thanks for much for this fix. Really appreciate it. 🙇

jamespollard8 commented 5 years ago

@niall-byrne I'll leave the fork deletion to you