Skip to content
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

Support for filter filtering of int type values #7636

Merged
merged 13 commits into from
Sep 2, 2021
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ Release Notes.
* Fix `LoggingConfigWatcher` return `watch.value` would not consistent with the real configuration content.
* Fix `ZookeeperConfigWatcherRegister.readConfig()` could cause `NPE` when `data.getData()` is null.
* Support nacos grouped dynamic configurations.
* Support for filter function filtering of int type values.

#### UI

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ literalExpression
;

expression
: booleanMatch | stringMatch | greaterMatch | lessMatch | greaterEqualMatch | lessEqualMatch | notEqualMatch | booleanNotEqualMatch | likeMatch | inMatch | containMatch | notContainMatch
: booleanMatch | numberMatch | stringMatch | greaterMatch | lessMatch | greaterEqualMatch | lessEqualMatch | notEqualMatch | booleanNotEqualMatch | likeMatch | inMatch | containMatch | notContainMatch
;

containMatch
Expand All @@ -108,6 +108,10 @@ booleanMatch
: conditionAttributeStmt DUALEQUALS booleanConditionValue
;

numberMatch
: conditionAttributeStmt DUALEQUALS numberConditionValue
;

stringMatch
: conditionAttributeStmt DUALEQUALS (stringConditionValue | enumConditionValue)
;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,11 @@ public void enterBooleanMatch(OALParser.BooleanMatchContext ctx) {
conditionExpression.setExpressionType("booleanMatch");
}

@Override
public void enterNumberMatch(OALParser.NumberMatchContext ctx) {
conditionExpression.setExpressionType("numberMatch");
}

@Override
public void enterStringMatch(OALParser.StringMatchContext ctx) {
conditionExpression.setExpressionType("stringMatch");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import java.util.List;
import org.apache.skywalking.oap.server.core.analysis.metrics.expression.BooleanMatch;
import org.apache.skywalking.oap.server.core.analysis.metrics.expression.BooleanNotEqualMatch;
import org.apache.skywalking.oap.server.core.analysis.metrics.expression.EqualMatch;
import org.apache.skywalking.oap.server.core.analysis.metrics.expression.StringMatch;
import org.apache.skywalking.oap.server.core.analysis.metrics.expression.NotEqualMatch;
import org.apache.skywalking.oap.server.core.annotation.AnnotationScan;
import org.apache.skywalking.oap.server.core.source.DefaultScopeDefine;
Expand Down Expand Up @@ -127,7 +127,7 @@ public void testFilterAnalysis() {
List<Expression> filterExpressions = result.getFilterExpressions();
Assert.assertEquals(1, filterExpressions.size());
Expression filterExpression = filterExpressions.get(0);
Assert.assertEquals(EqualMatch.class.getName(), filterExpression.getExpressionObject());
Assert.assertEquals(StringMatch.class.getName(), filterExpression.getExpressionObject());
Assert.assertEquals("source.getName()", filterExpression.getLeft());
Assert.assertEquals("\"/service/prod/save\"", filterExpression.getRight());
}
Expand Down Expand Up @@ -157,7 +157,7 @@ public void shouldUseCorrectMatcher() {
result.addFilterExpressionsParserResult(new ConditionExpression("stringMatch", "type", ""));
result = analysis.analysis(result);
assertTrue(result.getFilterExpressions().size() > 0);
assertEquals(EqualMatch.class.getName(), result.getFilterExpressions().get(0).getExpressionObject());
assertEquals(StringMatch.class.getName(), result.getFilterExpressions().get(0).getExpressionObject());
assertEquals("source.getType()", result.getFilterExpressions().get(0).getLeft());

result.setFilterExpressions(null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,24 @@ public void testParse9() throws IOException {
Assert.assertEquals(1, methodArgsExpressions.size());
}

@Test
public void testParse10() throws IOException {
ScriptParser parser = ScriptParser.createFromScriptText(
"ClientCpm = from(ServiceInstanceRelation.*).filter(componentId == 7).cpm();", TEST_SOURCE_PACKAGE);
List<AnalysisResult> results = parser.parse().getMetricsStmts();
AnalysisResult clientCpm = results.get(0);
Assert.assertEquals("ClientCpm", clientCpm.getMetricsName());
Assert.assertEquals("ServiceInstanceRelation", clientCpm.getSourceName());
Assert.assertEquals("[*]", clientCpm.getSourceAttribute().toString());
final List<Expression> filterExpressions = clientCpm.getFilterExpressions();
Assert.assertEquals(1, filterExpressions.size());
Assert.assertEquals("source.getComponentId()", filterExpressions.get(0).getLeft());
Assert.assertEquals("cpm", clientCpm.getAggregationFunctionName());
EntryMethod entryMethod = clientCpm.getEntryMethod();
List<Object> methodArgsExpressions = entryMethod.getArgsExpressions();
Assert.assertEquals(1, methodArgsExpressions.size());
}

@Test
public void testDisable() throws IOException {
ScriptParser parser = ScriptParser.createFromScriptText("disable(segment);", TEST_SOURCE_PACKAGE);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.skywalking.oap.server.core.analysis.metrics.expression;

import org.apache.skywalking.oap.server.core.analysis.metrics.annotation.FilterMatcher;

@FilterMatcher
public class NumberMatch {

public boolean match(int left, int right) {
return left == right;
}

public boolean match(long left, long right) {
return left == right;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import org.apache.skywalking.oap.server.core.analysis.metrics.annotation.FilterMatcher;

@FilterMatcher("stringMatch")
public class EqualMatch {
public class StringMatch {

public boolean match(String left, String right) {
if (left.startsWith("\"") && left.endsWith("\"")) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

package org.apache.skywalking.oap.server.core.analysis.metrics;

import org.apache.skywalking.oap.server.core.analysis.metrics.expression.EqualMatch;
import org.apache.skywalking.oap.server.core.analysis.metrics.expression.StringMatch;
import org.apache.skywalking.oap.server.core.remote.grpc.proto.RemoteData;
import org.junit.Assert;
import org.junit.Test;
Expand All @@ -27,18 +27,18 @@ public class PercentMetricsTest {
@Test
public void testEntranceCombine() {
PercentMetricsImpl impl = new PercentMetricsImpl();
impl.combine(new EqualMatch().match(true, true));
impl.combine(new EqualMatch().match(true, false));
impl.combine(new EqualMatch().match(true, false));
impl.combine(new StringMatch().match(true, true));
impl.combine(new StringMatch().match(true, false));
impl.combine(new StringMatch().match(true, false));

impl.calculate();

Assert.assertEquals(3333, impl.getValue());

impl = new PercentMetricsImpl();
impl.combine(new EqualMatch().match(true, true));
impl.combine(new EqualMatch().match(true, true));
impl.combine(new EqualMatch().match(true, false));
impl.combine(new StringMatch().match(true, true));
impl.combine(new StringMatch().match(true, true));
impl.combine(new StringMatch().match(true, false));

impl.calculate();

Expand All @@ -48,14 +48,14 @@ public void testEntranceCombine() {
@Test
public void testSelfCombine() {
PercentMetricsImpl impl = new PercentMetricsImpl();
impl.combine(new EqualMatch().match(true, true));
impl.combine(new EqualMatch().match(true, false));
impl.combine(new EqualMatch().match(true, false));
impl.combine(new StringMatch().match(true, true));
impl.combine(new StringMatch().match(true, false));
impl.combine(new StringMatch().match(true, false));

PercentMetricsImpl impl2 = new PercentMetricsImpl();
impl2.combine(new EqualMatch().match(true, true));
impl2.combine(new EqualMatch().match(true, true));
impl2.combine(new EqualMatch().match(true, false));
impl2.combine(new StringMatch().match(true, true));
impl2.combine(new StringMatch().match(true, true));
impl2.combine(new StringMatch().match(true, false));

impl.combine(impl2);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.skywalking.oap.server.core.analysis.metrics.expression;

import org.junit.Test;

import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertFalse;

public class NumberMatchTest {

@Test
public void integerShouldEqual() {
Integer a = 334;
Integer b = 334;
boolean match = new NumberMatch().match(a, b);
assertTrue(match);

a = -123;
b = -123;
match = new NumberMatch().match(a, b);
assertTrue(match);

a = -122;
b = -123;
match = new NumberMatch().match(a, b);
assertFalse(match);

a = -123;
b = -122;
match = new NumberMatch().match(a, b);
assertFalse(match);
}

@Test
public void intShouldEqual() {
int a = 334;
int b = 334;
boolean match = new NumberMatch().match(a, b);
assertTrue(match);

a = -123;
b = -123;
match = new NumberMatch().match(a, b);
assertTrue(match);

a = -122;
b = -123;
match = new NumberMatch().match(a, b);
assertFalse(match);

a = -123;
b = -122;
match = new NumberMatch().match(a, b);
assertFalse(match);
}

@Test
public void longShouldEqual() {
long a = 21474836478L;
long b = 21474836478L;
boolean match = new NumberMatch().match(a, b);
assertTrue(match);

a = -21474836478L;
b = -21474836479L;
match = new NumberMatch().match(a, b);
assertFalse(match);

Long c = -123L;
Long d = -123L;
match = new NumberMatch().match(c, d);
assertTrue(match);

c = -21474836478L;
d = -21474836479L;
match = new NumberMatch().match(c, d);
assertFalse(match);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -23,44 +23,44 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

public class EqualMatchTest {
public class StringMatchTest {

@Test
public void integerShouldEqualWhenLargerThan128() {
Integer a = 334;
Integer b = 334;
boolean match = new EqualMatch().match(a, b);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From these tests, the original marcher should already support numeric types, just need to wrap primitives such as int to boxed type Integrr or add method signatures with primitive types

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reason as #7636 (comment). And discussed at #7517

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I trying below whether adding the method signature can be directly supported

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a question about what the regression tests need to do every time you modify the oap part of the code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I trying below whether adding the method signature can be directly supported

What is this? A question or something?

There is a question about what the regression tests need to do every time you modify the oap part of the code

There is no guidance or standards about this, because this is rarely to change, and has been changed for a long time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this problem kezhenxu94 raised a better solution I m trying

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
As kezhenxu94 said this can be solved by simply the matching rules support numbers in stringmatch I think this is a better solution and is the class name restored to EqualMatch, I think EqualMatch is more reasonable in design and functionality

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whether it is working or not, is not important at all. If you want to more method, consider int and long. But you would not compare number, especially double and float. Because simply in Java 1.01 == 1.01 is not guaranteed.

boolean match = new StringMatch().match(a, b);
assertTrue(match);
}

@Test
public void longShouldEqualWhenLargerThan128() {
Long a = 334L;
Long b = 334L;
boolean match = new EqualMatch().match(a, b);
boolean match = new StringMatch().match(a, b);
assertTrue(match);
}

@Test
public void doubleShouldEqualWhenLargerThan128() {
Double a = 334.0;
Double b = 334.0;
boolean match = new EqualMatch().match(a, b);
boolean match = new StringMatch().match(a, b);
assertTrue(match);
}

@Test
public void floatShouldEqualWhenLargerThan128() {
Float a = 334.0F;
Float b = 334.0F;
boolean match = new EqualMatch().match(a, b);
boolean match = new StringMatch().match(a, b);
assertTrue(match);
}

@Test
public void stringShouldEqual() {
assertTrue(new EqualMatch().match("\"a\"", "a"));
assertTrue(new EqualMatch().match("a", "a"));
assertFalse(new EqualMatch().match("\"a\"", "ab"));
assertTrue(new StringMatch().match("\"a\"", "a"));
assertTrue(new StringMatch().match("a", "a"));
assertFalse(new StringMatch().match("\"a\"", "ab"));
}
}