Fix environment and service name match for unary span filter expression #234
Conversation
| validateNonDefaultPresenceOrThrow( | ||
| filter.getUnarySpanFilter(), UnarySpanFilterExpression.OPERATOR_FIELD_NUMBER); | ||
|
|
||
| final LeftOperandCase leftOperandCase = filter.getUnarySpanFilter().getLeftOperandCase(); |
There was a problem hiding this comment.
I think I'm missing something. You're saying field must be set on 106, then immediately saying if field is set it's also an error?
There was a problem hiding this comment.
Sorry my mistake here - I saw the word FIELD and thought it was checking for the field.. field. But it's checking for the operator field.
Given this though, recommend removing this from the proto rather than relying on validation since it was just added and not yet in use.
| if (spanFilter.hasRelationalSpanFilter()) { | ||
| return matchesServiceName(spanFilter.getRelationalSpanFilter(), serviceName); | ||
| } else { | ||
| if (spanFilter | ||
| .getLogicalSpanFilter() | ||
| .getOperator() | ||
| .equals(LogicalOperator.LOGICAL_OPERATOR_AND)) { | ||
| if (hasNoRelationalFilters(spanFilter.getLogicalSpanFilter())) { | ||
| return true; |
There was a problem hiding this comment.
This doesn't necessarily hold. It seems like you're making assumptions about structure here. e.g. AND(AND(serviceName = x, otherCondition)) would stop searching as soon as it got inside the first AND.
| @@ -59,6 +72,12 @@ public boolean matchesServiceName(SpanFilter spanFilter, String serviceName) { | |||
| } | |||
| } | |||
|
|
|||
| private boolean hasNoRelationalFilters(LogicalSpanFilterExpression logicalSpanFilterExpression) { | |||
| return logicalSpanFilterExpression.getOperandsCount() == 0 | |||
There was a problem hiding this comment.
This should recurse. Also the operand count is redundant, since if it was 0 none match would return true.
There was a problem hiding this comment.
Will fix in the later PR. This is known problem. Environment and servicename is not checked at more than 1 nested levels
Description
Please include a summary of the change, motivation and context.
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.