[Single Span Sampling] Add single span parser #2095
Conversation
marcotc
force-pushed
the
single-span-parser
branch
from
b83d795
to
eba6923
Compare
marcotc
changed the title
| def ==(other) | ||
| return super unless other.is_a?(Matcher) | ||
|
|
||
| name == other.name && | ||
| service == other.service | ||
| end |
There was a problem hiding this comment.
Minor: Arguably this is not very duck-typing of us; perhaps we could check that other.respond_to?(:name) && other.respond_to?(:service)?
(Would apply to other similar changes in this PR)
There was a problem hiding this comment.
Do you think this is needed, even with return super unless other.is_a?(Matcher) being checked?
There was a problem hiding this comment.
I'll come back to this in the feature branch if you think this needs to be improved, merging this for now to keep me sane.
There was a problem hiding this comment.
Ah! Yes, I wasn't clear about that part -- my suggestion would mean replacing the is_a?(Matcher) and relying only on the respond_to? instead. (But I marked this as minor, so definitely no need to hold anything back just for it)
| # Parses a list of Hashes containing the parsed JSON information | ||
| # for Single Span Sampling configuration. | ||
| # In case of parsing errors, `nil` is returned. | ||
| # | ||
| # @param rules [Array<String] the JSON configuration rules to be parsed | ||
| # @return [Array<Datadog::Tracing::Sampling::Span::Rule>] a list of parsed rules | ||
| # @return [nil] if parsing failed | ||
| def parse_list(rules) | ||
| unless rules.is_a?(Array) | ||
| # Using JSON terminology for the expected error type | ||
| Datadog.logger.warn("Span Sampling Rules are not an array: #{JSON.dump(rules)}") | ||
| return nil | ||
| end | ||
|
|
||
| parsed = rules.map do |hash| | ||
| unless hash.is_a?(Hash) | ||
| # Using JSON terminology for the expected error type | ||
| Datadog.logger.warn("Span Sampling Rule is not a key-value object: #{JSON.dump(hash)}") | ||
| return nil | ||
| end | ||
|
|
||
| begin | ||
| parse_rule(hash) | ||
| rescue => e | ||
| Datadog.logger.warn("Cannot parse Span Sampling Rule #{JSON.dump(hash)}: " \ |
There was a problem hiding this comment.
I can live-ish with it I guess, but from your PR description:
In the future, we'll likely support parsing this data directly from YAML or a global datadog.conf JSON, thus a non-JSON specific method, parse_list, is also provided in additional to the main parse_json method.
Is it me or is this method still quite biased towards JSON in its current form? Should we just make it private for now, and revisit it later once we want to expand to yaml and other formats?
There was a problem hiding this comment.
Changed to non-json bias.
| rescue => e | ||
| Datadog.logger.warn("Cannot parse Span Sampling Rule #{JSON.dump(hash)}: " \ | ||
| "#{e.class.name} #{e} at #{Array(e.backtrace).first}") | ||
| nil |
There was a problem hiding this comment.
This semantics is somewhat unexpected -- we seem to be quite strict with all other checks (and bail out from this method entirely if something is off), but if parse_rule fails then we just skip over that rule.
This seems deliberate (the tests have is_expected.to be_empty), but I'm curious why we're more strict in some cases and less in others?
There was a problem hiding this comment.
This is not spec'ed out but I liked the strict parsing, I've changed the PR accordingly.
I'll make sure the spec reflects it as well. (Or if there's disagreement at spec-level, I'll make changes to the feature branch accordingly)
marcotc
force-pushed
the
single-span-parser
branch
2 times, most recently
from
ddd19e7
to
e5ba346
Compare
marcotc
force-pushed
the
single-span-parser
branch
from
e5ba346
to
de3b4a3
Compare
Follows up from #2091
This PR parses user provided JSON rules into
Datadog::Tracing::Sampling::Span::Ruleobjects.The code ensures that no errors are propagated back to the user in the form of an exception, but all errors are logged to aid in addressing them.
In the future, we'll likely support parsing this data directly from YAML or a global
datadog.confJSON, thus a non-JSON specific method,parse_list, is also provided in additional to the mainparse_jsonmethod.A few other changes:
TokenBucketnow is more strict about its inputs, which helps the validation of Single Span sample rules.RuleandMatcherto ease testing.