[VANotify] - Update VANotify::DefaultCallback to handle hash or array statsd_tags #20235
Conversation
| 'function:function description'] | ||
| end | ||
|
|
||
| context 'metadata is provided with statsd_tags as hash' do |
There was a problem hiding this comment.
I debated trying to DRY up some of these specs a bit by reusing the notification variable or something similar, but kept it a little more separate for now to keep some of the existing patterns.
There was a problem hiding this comment.
Yeah, I like specs to be clear to read and also clear when they fail - so I'm good with the duplication.
| tags = ["service:#{service}", "function:#{function}"] | ||
|
|
||
| tags = if statsd_tags.is_a?(Hash) | ||
| statsd_tags.map { |key, value| "#{key}:#{value}" } |
There was a problem hiding this comment.
This should allow any key:value pair in the same format as we use when incrementing statsd metrics. This does allow for other tags, as well as not forcing the service and function tags. If we need to require those tags I can update it to validate that those are present.
There was a problem hiding this comment.
Yeah, I think it would be good to make it more obvious that we need both the service and function tags.
There was a problem hiding this comment.
Yeah, that makes sense. I'll take a look at this tomorrow and see if I can make a nice way to show the two required tags. Thanks for taking a look!
There was a problem hiding this comment.
@nathanbwright when you have some time, I updated the default_callbacks to require the service and function keys, as well as raise an exception when the metadata type is invalid.
|
I will wait until VA Notify has a chance to look. The changes look straightforward to me, but want to make sure this has some eyes on it from the VA Notify team |
coope93
dismissed stale reviews from LindseySaari and nathanbwright
via
40bef0f
Generated by 🚫 Danger |
| @@ -1434,6 +1434,9 @@ features: | |||
| va_notify_custom_errors: | |||
| actor_type: user | |||
| description: Custom error classes instead of the generic Common::Exceptions::BackendServiceException | |||
| va_notify_metadata_statsd_tags: | |||
There was a problem hiding this comment.
I realized I wanted this behind a flipper toggle, so I added a new one. The spec diff is larger since I have a context for on/off, and we'll be able to delete the off side after we validate everything is fine in prod for it.
coope93
requested review from
LindseySaari
and removed request for
LindseySaari
Summary
VANotify::DefaultCallbackto handle receiving thestatsd_tagsin thecallback_metadataas either a hash or an array. Currently, it only supports a hash. When calling theStatsD.increment('stats_d_metric_name', tags:), statsD expects the tags as an array of'key:value'strings, and this PR adds the ability to pass thestatsd_tagsin the same format while keeping the existing hash functionality. It also allows for passing any tags that will be added to thestatsdzero silent failure metrics, and not just thefunctionandservicetag. If that is not desired I can update this PR, but it seemed to add value.functionandservicekeys are provided in themetadata, as well as validates that themetadatais either a hash or array.va_notifyemails, but in adding them I was passing the tags in thestatsdformat of an array, as opposed to a hash.va_notify_metadata_statsd_tagsflipper toggleRelated issue(s)
Testing done
Screenshots
Note: Optional
What areas of the site does it impact?
VANotify callbacks and anywhere they are currently being used
Acceptance criteria
Requested Feedback
Would you prefer I add this behind a flipper toggle, and is it alright that now all tags are able to be passed as part of the metadata.