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

Fixing #1231. Adding order and status to trace query. #1255

Merged
merged 8 commits into from
May 24, 2018

Conversation

ajanthan
Copy link
Contributor

Please answer these questions before submitting pull request


Bug fix

  • Bug description.

  • How to fix?


New feature or improvement

  • Describe the details and related test reports.
    Added status and order to trace query API and updated UI

@coveralls
Copy link

coveralls commented May 22, 2018

Coverage Status

Coverage decreased (-0.0006%) to 23.874% when pulling 0730159 on ajanthan:master into a835fe4 on apache:master.

Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Thanks for providing this. Have you saw #1240 ? The query protocol should follow that way, use emun to instead of int.

@ajanthan
Copy link
Contributor Author

OK. Let me change Ints to enum and resend the pull request.

@wu-sheng
Copy link
Member

Thanks and look forward to have this soon.

@ajanthan
Copy link
Contributor Author

Updated the pull request as requested.

@wu-sheng
Copy link
Member

@ajanthan Your CI fails.

@wu-sheng wu-sheng added this to the 5.0.0-beta2 milestone May 24, 2018
@wu-sheng wu-sheng added backend OAP backend related. feature New feature labels May 24, 2018
searchRequestBuilder.addSort(SegmentDurationTable.START_TIME.getName(), SortOrder.DESC);
switch (traceState) {
case ERROR:
mustQueryList.add(QueryBuilders.matchQuery(SegmentDurationTable.IS_ERROR.getName(), 1));
Copy link
Member

Choose a reason for hiding this comment

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

1 -> BooleanUtils.TRUE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it.

mustQueryList.add(QueryBuilders.matchQuery(SegmentDurationTable.IS_ERROR.getName(), 1));
break;
case SUCCESS:
mustQueryList.add(QueryBuilders.matchQuery(SegmentDurationTable.IS_ERROR.getName(), 0));
Copy link
Member

Choose a reason for hiding this comment

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

0 -> BooleanUtils.FALSE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it.

sql = sql + " and {" + paramIndex + "} = ?";
switch (traceState) {
case ERROR:
params.add(1);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

params.add(1);
break;
case SUCCESS:
params.add(0);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@@ -73,16 +75,30 @@ public TraceBrief loadTop(long startSecondTimeBucket, long endSecondTimeBucket,
boolQueryBuilder.must().add(rangeQueryBuilder);
}
if (StringUtils.isNotEmpty(operationName)) {
mustQueryList.add(QueryBuilders.matchPhraseQuery(SegmentDurationTable.SERVICE_NAME.getName(), operationName));
mustQueryList.add(QueryBuilders.matchQuery(SegmentDurationTable.SERVICE_NAME.getName(), operationName));
Copy link
Member

Choose a reason for hiding this comment

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

I forgot why chose the method matchPhraseQuery before. Please ensure these two service names can be searched by the method matchQuery.

  1. org.skywaking.apm.testcase.dubbo.services.GreetService.doBusiness()
  2. /dubbox-case/case/dubbox-rest

Copy link
Member

Choose a reason for hiding this comment

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

matchPhraseQuery is from #1194 . @ajanthan Why do you use matchQuery?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a mistake. Fixed it in the latest pull request.

@ajanthan
Copy link
Contributor Author

@wu-sheng @peng-yongsheng I sent an updated pull request. Please review it.

@wu-sheng wu-sheng merged commit 58ede1d into apache:master May 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend OAP backend related. feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants