Update Statsd tag serializer to allow falsy values#290
Update Statsd tag serializer to allow falsy values#290joseph-hughes wants to merge 1 commit intoDataDog:masterfrom
Conversation
ad053c0 to
0c67365
Compare
| if value&.to_s.nil? | ||
| escape_tag_content(name) | ||
| else | ||
| escape_tag_content("#{name}:#{value}") |
There was a problem hiding this comment.
This gem supports Ruby as low as 2.1, so we cannot use &., sorry :/
I suggest keeping the if/else as it was, and just change the check to a nil-check explicitly:
| if value&.to_s.nil? | |
| escape_tag_content(name) | |
| else | |
| escape_tag_content("#{name}:#{value}") | |
| if !value.nil? | |
| escape_tag_content("#{name}:#{value}") | |
| else | |
| escape_tag_content(name) |
There was a problem hiding this comment.
I wish to also ignore nil when casted to a string to be extra safe, so I went with this suggestion but updated it to:
if !value.nil? && !value.to_s.nil?There was a problem hiding this comment.
(Unrelated to this PR but -- should we raise the minimum version for this gem? dd-trace-rb has moved on for a long time, so it's weird to still keep this one at 2.1)
From this helpful suggestion: DataDog#290 (comment) Co-authored-by: Marco Costa <mmarcottulio@gmail.com>
This instead now ensures that we skip including the tag value only if: - it is equal to 'nil' - it when stringified is equal to 'nil' The reasoning for this was to make it so falsy values (e.g. false) will now be included in our tag content, instead of skipped.
674ed14 to
6d60e39
Compare
|
Hi, @marcotc, @chouetz, @StephenWakely, and @rayz 👋🏼 Pinging to see if there's any feedback or blockers for this PR. Happy to make changes if needed. Thanks! |
|
Hey @joseph-hughes, so sorry for the delay. The issue totally makes sense! |
Problem
If we provide a tag which has a boolean value, we are unable to properly track the
falsecase, since the "true" string appears with the tag name, but the "false" string does not show up. This is because of this logic, which checks for tag value presence:dogstatsd-ruby/lib/datadog/statsd/serialization/tag_serializer.rb
Lines 58 to 66 in 517b830
trueis evaluated to present, so we would add that tag name and value; however,falseis evaluated to not present, so we would only add that tag name (not the value).Solution
First, check for
nilexplicitly then also check fornilafter converting to a string.This does result in other values being deemed "present" now which were not previously though, such as
[]and{}. Is this a problem though?