-
Notifications
You must be signed in to change notification settings - Fork 10
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
fix : sorting rate metrics #102
Conversation
Codecov Report
@@ Coverage Diff @@
## main #102 +/- ##
============================================
+ Coverage 82.47% 82.48% +0.01%
- Complexity 419 421 +2
============================================
Files 37 37
Lines 1603 1604 +1
Branches 168 169 +1
============================================
+ Hits 1322 1323 +1
Misses 207 207
Partials 74 74
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
return function.getArgumentsList().stream().filter(e -> e.hasColumnIdentifier()).findFirst(); | ||
} | ||
|
||
private String getAvgRateLiteral(Function function) { |
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.
getAvgRateLiteral
-> getLiteralForAvgRate
?
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.
getRateIntervalForAvgRate
?
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.
resolved
@@ -0,0 +1,28 @@ | |||
{ |
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.
Can you add tests ?
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.
added
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.
resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
lgtm
@@ -96,6 +95,18 @@ private String functionToStringForAvgRate( | |||
columnName, (double) aggregateIntervalInSeconds / rateIntervalInSeconds); | |||
} | |||
|
|||
private Optional<Expression> getColumnNameForAvgRate(Function function) { |
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.
Why are we returning an Optional here if the caller is forcefully doing get()?
Optional here makes sense if the caller needs to deal with the value being present or not.
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.
Agreed. Also, this isn't returning a column name, it's returning an expression. If I sent my args out of order, this would still work which is not what we want - we just want backwards compatibility. I would just change the old line 84 to default if empty, and leave the rest as is.
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.
resolved
@@ -80,9 +80,8 @@ private String functionToStringForAvgRate( | |||
java.util.function.Function<Expression, String> argumentConverter, | |||
ExecutionContext executionContext) { | |||
|
|||
String columnName = argumentConverter.apply(function.getArgumentsList().get(0)); | |||
String rateIntervalInIso = | |||
function.getArgumentsList().get(1).getLiteral().getValue().getString(); |
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.
That is, change this to something equivalent to
function.getArgumentsList().size() == 2
? function.getArgumentsList().get(1).getLiteral().getValue().getString() : DEFAULT_AVG_RATE_SIZE;
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.
made the change
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.
resolved
return function.getArgumentsList().stream().filter(e -> e.hasColumnIdentifier()).findFirst(); | ||
} | ||
|
||
private String getRateIntervalForAvgRate(Function function) { |
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.
We have integration-tests in query-service. We should definitely add one for this case.
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.
We already have two integration tests for testing the average rate changes. Should I add for this specific case also where average rate is in order-by
expression ?
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.
Yes. We should definitely add one.
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.
resolved
This comment has been minimized.
This comment has been minimized.
@@ -1120,14 +1242,19 @@ private QueryRequest buildSimpleQueryWithFilter(Filter filter) { | |||
return builder.build(); | |||
} | |||
|
|||
private void assertPQLQuery(QueryRequest queryRequest, String expectedQuery) { | |||
private void assertPQLQuery( | |||
QueryRequest queryRequest, String expectedQuery, ViewDefinition viewDefinition) { |
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.
if we're going to require passing this in (which is fine), we shouldn't make it a global static.
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 was because we need service view definition for testing avgrate changes. Rest all use this same view definition.
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.
resolved
private void assertPQLQuery( | ||
QueryRequest queryRequest, String expectedQuery, ViewDefinition viewDefinition) { | ||
|
||
when(mockingExecutionContext.getTenantId()).thenReturn("__default"); |
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.
An assertion shouldn't do mocking of its own. I would guess these are not used by most test cases, which means mockito would be erroring out unless we're running in lenient mode (I'm guessing we are). We don't need to switch lenient off as part of this change, but we should be strict for any new tests we write.
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.
getTenantId()
is used by most of the tests.
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.
moved others to the respective test.
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.
we shouldn't share the state though - that's the same problem you ran into with view definition. Have each test pass in their own execution context, same as view def, then they can mock that locally, according to their test parameters. For backwards compat, you could overload the assert method with the existing behavior if you want - that way no existing tests hve to be modified. In the current for, we have a mix of both things we don't want - shared state (including newly introduced shared state) and modifying the existing tests.
} | ||
|
||
@Test | ||
public void testQueryWithAverageRateInOrderBy() { |
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.
@aaron-steinfeld @avinashkolluru the integration test would be exactly the same as this. Should I keep this test also or remove this one?
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 think we should keep this as well. It is checking the expected PQL query. The integration test will be checking the output of the API.
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.
resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
approving to unblock the bug fix, but we should get the newly added tests cleaned up here or in a followup.
Mockito.when(connection.prepareStatement(any(Request.class))).thenCallRealMethod(); | ||
mockingExecutionContext = mock(ExecutionContext.class); | ||
when(mockingExecutionContext.getTenantId()).thenReturn("__default"); | ||
when(this.mockingExecutionContext.getTimeSeriesPeriod()).thenReturn(Optional.empty()); |
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 mock is not relevant to every test and would fail in strict mode. please move it such that we only mock things that are relevant to the test.
For future tests (beyond the scope of this PR), make sure to always run in strict mode. The easiest way to do that is probably use the junit extension -
@ExtendWith(MockitoExtension.class)
Raised a follow-up PR for fixing the unit tests : #103 |
On the service (backend) screen page, there is a rate column -
error/s
. After we moved the avgrate functionality to query-service, the sorting on this column is broken.The reason it was broken, that avgrate function takes two arguments (column name, size). The second size argument is missing in
order by
expression. Priori, it was working fine asavgrate
was translated tosum
at the gateway service layer. Andsum
function needs only single column name argument.As these rate columns are per second rate, for now, defaulting to
PT1S
as size for avgrate function to fix the regression.