Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Single Span Sampling] Add single span parser #2095
[Single Span Sampling] Add single span parser #2095
Changes from all commits
8645e4c
d00ea3a
de3b4a3
a03ae7f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think this is needed, even with
return super unless other.is_a?(Matcher)
being checked?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Yes, I wasn't clear about that part -- my suggestion would mean replacing the
is_a?(Matcher)
and relying only on therespond_to?
instead. (But I marked this as minor, so definitely no need to hold anything back just for it)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)