Only allow certain types of Numeric values as attribute values. #1173
Conversation
plantfansam
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin and
ahayworth
as code owners
| @@ -20,8 +20,12 @@ def valid_key?(key) | |||
| key.instance_of?(String) | |||
| end | |||
|
|
|||
| def valid_numeric?(value) | |||
There was a problem hiding this comment.
Maybe try to be consistent with boolean? above?
| def valid_numeric?(value) | |
| def numeric?(value) |
| @@ -20,8 +20,12 @@ def valid_key?(key) | |||
| key.instance_of?(String) | |||
| end | |||
|
|
|||
| def valid_numeric?(value) | |||
| value.is_a?(Integer) || value.is_a?(Float) | |||
There was a problem hiding this comment.
Two things:
- I wonder if we should explicitly use
.instance_of?to restrict values to instances of these specific classes, in case someone tries to subclass them & that 💥 inprotobufserialization later. - What's the current state of
BigInteger? IIRC it was merged withIntegersome releases of MRI ago. I'm not sure whether we should check for it here or if we can now comfortably assumevalue.is_a?(Integer)covers that.
There was a problem hiding this comment.
100% agree with moving to instance_of? - that's a great idea.
There was a problem hiding this comment.
I think Bignum and Fixnum are kaput as of 2.4.0: https://github.com/ruby/ruby/blob/master/doc/ChangeLog-2.4.0#L4927-L4963
There was a problem hiding this comment.
changed from is_a? to instance_of? here 0c1f281
| def valid_simple_value?(value) | ||
| value.instance_of?(String) || value == false || value == true || value.is_a?(Numeric) | ||
| value.instance_of?(String) || value == false || value == true || valid_numeric?(value) |
There was a problem hiding this comment.
Further to the instance_of? vs is_a? question, we are using == here for booleans, which may be too loose a constraint. We also have boolean? defined above in terms of is_a? and used in valid_array_value? below. Should we instead use instance_of? in boolean? and use boolean?(value) here?
There was a problem hiding this comment.
I can't think of a single reason why not, hence f5755d07f734b3f9bbcce65ed53bc36cc07cba2a
There was a problem hiding this comment.
I think this lgtm, as a followup (but not blocking imo, this is a good incremental improvement) we can think about how similar work for Strings might look, see: #1146
Nice work!
TY! I will make a note to see about giving the ol' 👁️ 👁️ to our |
We ran into a pretty odd failure mode, which this patch will hopefully help address (h/t @ericmustin for sussing this out):
In an app, we set a span attribute value to an
ActiveSupport::Durationinstance. So imagine something like.in_span("foobar", { "baz" => 1.minute }. When we viewed the span in various destinations, it the attribute showed up with the value asnil(ornullor what have you). We saw the symptom (null attribute value, which is not allowed per the otel spec) before we sussed out the problem.Turns out that, when exporting, the otlp exporter checks for a whitelist of exportable value classes that are pretty basic (code here). As a result, a value with class
ActiveSupport::Durationwill benil.This patch should ensure that users will get a warning if they use an unsupported
Numericclass, and that the invalid attribute k/v pair will not be exported.I didn't see the need to do this, but we could also potentially: