Fix error_status_codes options
#3344
Conversation
| @ranges.any? do |e| | ||
| case e | ||
| when Range | ||
| e.include? status |
There was a problem hiding this comment.
How about using === for both integer and range? I think it works for both.
There was a problem hiding this comment.
Compared to current code I feel like it kind of obscures the reading though: personally I prefer reading the current version which is immediately obvious, whereas I had to check that 2 === 2 and (2..4) === 2 both worked.
| values.compact! | ||
| values | ||
| o.setter do |v| | ||
| Tracing::Contrib::StatusRangeMatcher.new(v) if v |
There was a problem hiding this comment.
I would add a runtime type check here, to enforce that either one of these is true:
- v is an
Integer - v is a
Rangewith.first(and last, but that's internally checked byRange) beingInteger
| Datadog.logger.debug( | ||
| "Invalid error_status_codes configuration: Unable to parse #{value}, containing #{v}." | ||
| ) |
There was a problem hiding this comment.
Somehow I feel like this logging should happen in o.env_parser. That, or this class should at least be in Datadog::Tracing::Contrib::Configuration::Settings::StatusRangeEnvParser.
Also, I think we might be inconsistent and in some places env parse fails / conf type checks raise and in others they log but IIRC we mostly raise? I might be wrong on that one but I'd rather be consistent.
If we chose to always log, I feel like we should raise EnvValueError / ParseError / TypeError / whatever exceptions in such pieces of code, and then the caller (the config system) of o.env_parser / o.setter / ... should handle that kind of exception uniformly and log.
TonyCTHsu
force-pushed
the
tonycthsu/fix-error-status-codes
branch
from
3326f26
to
4c68809
Compare
|
Going to refactor later |
2.0 Upgrade Guide notes
What does this PR do?
Fix and standardize the way to configure an array of ranges for
error_status_codesfrom environment variables.