EZR: Add VANotify callback metadata to email notify job

[dellerbie]

Summary

• This work is behind a feature toggle (flipper): YES
• Adds support for VANotify callbacks when sending the failure email to the Veteran

Related issue(s)

• [BE] 10-10EZR Zero Silent Failure: Callback for VANotify Emails va.gov-team#94083

Testing done

• New code is covered by unit tests
• We will validate the metadata is sent when QA'ing the failure email

What areas of the site does it impact?

Form 10-10 EZR

Acceptance criteria

• I fixed|updated|added unit tests and integration tests for each feature (if applicable).
• No error nor warning in the console.
• Events are being sent to the appropriate logging solution
• Documentation has been updated (link to documentation)
• No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
• Feature/bug has a monitor built into Datadog (if applicable)
• If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
• I added a screenshot of the developed feature

[coope93]

I didn't realize this before, but the statsd_tags value needs to be a hash not an array. The DefaultCallback class currently expects it as a hash since it pulls out the service/function and their values and formats them in the array of strings we expect to pass to statsd metrics. I actually have a PR open to allow for both arrays and hashes in the DefaultCallback class, but at the moment this won't correctly add the tags.

[coope93]

So, my only concern with doing it this way is that now the DD_ZSF_TAGS can't be passed directly to the statsd increment call since the formatting is different (array vs hash). If we look at the other places we set the DD_ZSF_TAGS, it will be an array of strings. Does it feel weird to map over this and format it as a hash? This was why I want to update the default behavior to allow for the array, since I think it makes it much cleaner. If we do get that merged in you can revert it back to an array, but honestly in the meantime I'm not sure which way I prefer 🤷 . Thoughts?

[coope93]

I started to do something similar for the EZ, but for now I'm waiting to see if I can get my global change in to the default_callbacks. master...93247-statsd-tags-hash for reference

[dellerbie]

I think I'm confused why you need to map the hash back to an array. You mentioned that we can't pass the DD_ZSF_TAGS as a hash to the statsd call and I removed the line where we were passing the tags to the call.

I thought it was the VANotify::EmailJob.perform_async(*params) call that needed the tags to be in hash format, which is where the DD_ZSF_TAGS are being passed to?

I see that you still have this line in your branch, but maybe you meant to remove it?

[coope93]

I left that line in for when the flipper toggle is turned off to still increment the zsf statsd metric. That was my concern here. If you needed to pass the tags to an increment call the format would be wrong. I'm probably overthinking it though.

[dellerbie]

Okay I see. This entire method is guarded by the feature flag, so I think even if it is off, this code is unaffected by the hash/array conversions.

[dellerbie]

@coope93 Any other concerns with this PR?

[coope93]

No, this looks good 👍 . Just an fyi, I merged in this PR to allow for passing the array and not just the hash for the default_callbacks. It's currently turned on in staging but turned off in prod. But once that's turned on it could make sense to switch these back to an array just for consistency with the other places we set DD_ZSF_TAGS

[coope93]

So one last comment, but after talking with Nathan Wright on the VANotify team, I don't think we want to merge this change in just yet. The default callbacks currently only work for the vets-api service, and I believe this email is in our 1010 health apps service. We discovered that when testing this, and he updated staging to include our service so we could keep moving forward on it. So you should be able to confirm the behavior in staging, but if this is deployed to production the callbacks will not work. So we may want to put this behavior behind it's own toggle or just hold off for a bit 🤷

[coope93]

No, this looks good 👍 . Just an fyi, I merged in this PR to allow for passing the array and not just the hash for the default_callbacks. It's currently turned on in staging but turned off in prod. But once that's turned on it could make sense to switch these back to an array just for consistency with the other places we set DD_ZSF_TAGS

[stevenjcumming]

@dellerbie can you have a teammate review, then I'll review

[dellerbie]

So one last comment, but after talking with Nathan Wright on the VANotify team, I don't think we want to merge this change in just yet. The default callbacks currently only work for the vets-api service, and I believe this email is in our 1010 health apps service. We discovered that when testing this, and he updated staging to include our service so we could keep moving forward on it. So you should be able to confirm the behavior in staging, but if this is deployed to production the callbacks will not work. So we may want to put this behavior behind it's own toggle or just hold off for a bit 🤷

Okay I see. It makes sense to hold off since the fix is making its way to production soon. I'll just put this back into draft state for now.