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.

Sign up for GitHub

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

EZR: Add VANotify callback metadata to email notify job #20093

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

EZR: Add VANotify callback metadata to email notify job #20093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab1812d
EZR: Add VANotify callback metadata to email notify job
dellerbie Jan 3, 2025
3edea9e
EZR: Use hash instead of array for zsf tags
dellerbie Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions app/sidekiq/hca/ezr_submission_job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@ module HCA
class EzrSubmissionJob
include Sidekiq::Job
extend SentryLogging

FORM_ID = '10-10EZR'
VALIDATION_ERROR = HCA::SOAPParser::ValidationError
STATSD_KEY_PREFIX = 'api.1010ezr'
DD_ZSF_TAGS = [
'service:healthcare-application',
'function: 10-10EZR async form submission'
].freeze
DD_ZSF_TAGS = {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@dellerbie dellerbie Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@coope93 Any other concerns with this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

service: 'healthcare-application',
function: '10-10EZR async form submission'
}.freeze

# retry for 2d 1h 47m 12s
# https://github.com/sidekiq/sidekiq/wiki/Error-Handling
Expand Down Expand Up @@ -57,10 +59,12 @@ def self.send_failure_email(parsed_form)
email,
template_id,
{ 'salutation' => salutation },
api_key
api_key,
{
callback_metadata: { notification_type: 'error', form_number: FORM_ID, statsd_tags: DD_ZSF_TAGS }
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
)
StatsD.increment("#{STATSD_KEY_PREFIX}.submission_failure_email_sent")
StatsD.increment('silent_failure_avoided_no_confirmation', tags: DD_ZSF_TAGS)
end

def perform(encrypted_form, user_uuid)
Expand Down
12 changes: 9 additions & 3 deletions spec/sidekiq/hca/ezr_submission_job_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
end
let(:ezr_service) { double }
let(:tags) { described_class::DD_ZSF_TAGS }
let(:form_id) { described_class::FORM_ID }
let(:api_key) { Settings.vanotify.services.health_apps_1010.api_key }
let(:failure_email_template_id) { Settings.vanotify.services.health_apps_1010.template_id.form1010_ezr_failure_email }
let(:failure_email_template_params) do
Expand All @@ -21,14 +22,20 @@
{
'salutation' => "Dear #{form.dig('veteranFullName', 'first')},"
},
api_key
api_key,
{
callback_metadata: {
notification_type: 'error',
form_number: form_id,
statsd_tags: tags
}
}
]
end

def expect_submission_failure_email_and_statsd_increments
expect(VANotify::EmailJob).to receive(:perform_async).with(*failure_email_template_params)
expect(StatsD).to receive(:increment).with('api.1010ezr.submission_failure_email_sent')
expect(StatsD).to receive(:increment).with('silent_failure_avoided_no_confirmation', tags:)
end

describe 'when retries are exhausted' do
Expand Down Expand Up @@ -91,7 +98,6 @@ def expect_submission_failure_email_and_statsd_increments
described_class.within_sidekiq_retries_exhausted_block(msg) do
expect(VANotify::EmailJob).not_to receive(:perform_async)
expect(StatsD).not_to receive(:increment).with('api.1010ezr.submission_failure_email_sent')
expect(StatsD).not_to receive(:increment).with('silent_failure_avoided_no_confirmation', tags:)
end
end
end
Expand Down
Loading