You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/04/27 03:01:06 UTC
[GitHub] [skywalking] chenhaipeng opened a new pull request, #8959: refactor: es query optimize
chenhaipeng opened a new pull request, #8959:
URL: https://github.com/apache/skywalking/pull/8959
### Improve the performance of TermsAggregation
When skywalking queries the topology map, it uses queryTopoInfo to query the corresponding information. When it finds that it takes a long time, the main time-consuming statements are as follows,add ` "execution_hint": "map",
"collect_mode": "breadth_first",`there is about a 5-10x performance difference.
**My index table sw89_metrics-percent size is about 300GB/day**
```
POST /sw89_metrics-percent/_search
{
"size": 0,
"query": {
"bool": {
"must": [
{
"range": {
"time_bucket": {
"gte": 202204212336,
"lte": 202204212351
}
}
},
{
"terms": {
"entity_id": [
"xxx1"
]
}
},
{
"term": {
"metric_table": "service_sla"
}
}
]
}
},
"aggregations": {
"entity_id": {
"terms": {
"field": "entity_id",
"size": 1
},
"aggregations": {
"percentage": {
"avg": {
"field": "percentage"
}
}
}
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng merged pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #8959:
URL: https://github.com/apache/skywalking/pull/8959
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #8959:
URL: https://github.com/apache/skywalking/pull/8959#discussion_r864969089
##########
docs/en/changes/changes.md:
##########
@@ -39,6 +39,7 @@
* Add the support eBPF profiling field into the process entity.
* E2E: fix log test miss verify LAL and metrics.
* Enhance Converter mechanism in kernel level to make BanyanDB native feature more effective.
+* Add TermsAggregation properties collect_mode and execution_hint.
Review Comment:
```suggestion
* Add TermsAggregation properties collect_mode and execution_hint.
* add "execution_hint": "map", "collect_mode": "breadth_first" for aggregation and topology query to improve 5-10x performance.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #8959:
URL: https://github.com/apache/skywalking/pull/8959#discussion_r864838810
##########
oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/requests/search/CollectMode.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.library.elasticsearch.requests.search;
+
+import lombok.AccessLevel;
+import lombok.Getter;
+import lombok.RequiredArgsConstructor;
+
+@Getter
+@RequiredArgsConstructor(access = AccessLevel.PACKAGE)
+public final class CollectMode {
Review Comment:
What is this `CollectMode`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #8959:
URL: https://github.com/apache/skywalking/pull/8959#issuecomment-1110594519
Do you check why `breadth_first` is better than `depth_first`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #8959:
URL: https://github.com/apache/skywalking/pull/8959#discussion_r864969089
##########
docs/en/changes/changes.md:
##########
@@ -39,6 +39,7 @@
* Add the support eBPF profiling field into the process entity.
* E2E: fix log test miss verify LAL and metrics.
* Enhance Converter mechanism in kernel level to make BanyanDB native feature more effective.
+* Add TermsAggregation properties collect_mode and execution_hint.
Review Comment:
```suggestion
* Add TermsAggregation properties collect_mode and execution_hint.
* Add "execution_hint": "map", "collect_mode": "breadth_first" for aggregation and topology query to improve 5-10x performance.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #8959:
URL: https://github.com/apache/skywalking/pull/8959#discussion_r859438057
##########
oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/requests/search/aggregation/TermsAggregation.java:
##########
@@ -58,6 +58,8 @@ public void serialize(final TermsAggregation value, final JsonGenerator gen,
if (value.getOrder() != null) {
writeOrder(value, gen);
}
+ gen.writeStringField("execution_hint", "map");
+ gen.writeStringField("collect_mode", "breadth_first");
Review Comment:
You put these changes in the `TermsAggregation`, I think this would affect not just topology, it could affect `TopN` query, such as slow instance latency list.
I could find the doc of ElasticSearch, https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-terms-aggregation.html#search-aggregations-bucket-terms-aggregation-collect
So, are you meaning to do level-one aggregation first always provides better performance?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #8959:
URL: https://github.com/apache/skywalking/pull/8959#discussion_r860376121
##########
oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/requests/search/aggregation/TermsAggregation.java:
##########
@@ -58,6 +58,8 @@ public void serialize(final TermsAggregation value, final JsonGenerator gen,
if (value.getOrder() != null) {
writeOrder(value, gen);
}
+ gen.writeStringField("execution_hint", "map");
+ gen.writeStringField("collect_mode", "breadth_first");
Review Comment:
Please don't do this in the codec level.
- Add fields `executionHint`, `collectMode` in `TermsAggregation` (you can make them enum type considering their possible values are limited in ES's spec).
- Add fields `executionHint`, `collectMode` in `TermsAggregationBuilder` and corresponding builder methods, pass the two to `TermsAggregation` constructor in `build` method.
- Write the 2 fields here only when they are not empty/blank.
Then you should change set the 2 fields in topology query.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] chenhaipeng commented on a diff in pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
chenhaipeng commented on code in PR #8959:
URL: https://github.com/apache/skywalking/pull/8959#discussion_r859713284
##########
oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/requests/search/aggregation/TermsAggregation.java:
##########
@@ -58,6 +58,8 @@ public void serialize(final TermsAggregation value, final JsonGenerator gen,
if (value.getOrder() != null) {
writeOrder(value, gen);
}
+ gen.writeStringField("execution_hint", "map");
+ gen.writeStringField("collect_mode", "breadth_first");
Review Comment:
>
this elasticsearch version 6.8.0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #8959:
URL: https://github.com/apache/skywalking/pull/8959#discussion_r864849438
##########
oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/requests/search/aggregation/TermsAggregation.java:
##########
@@ -58,6 +60,12 @@ public void serialize(final TermsAggregation value, final JsonGenerator gen,
if (value.getOrder() != null) {
writeOrder(value, gen);
}
+ if (value.getCollectMode() != null){
Review Comment:
```suggestion
if (value.getCollectMode() != null) {
```
##########
oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/requests/search/aggregation/TermsAggregation.java:
##########
@@ -58,6 +60,12 @@ public void serialize(final TermsAggregation value, final JsonGenerator gen,
if (value.getOrder() != null) {
writeOrder(value, gen);
}
+ if (value.getCollectMode() != null){
+ gen.writeStringField("collect_mode", value.getCollectMode().value);
+ }
+ if (value.getExecutionHint() != null){
Review Comment:
```suggestion
if (value.getExecutionHint() != null) {
```
##########
oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/requests/search/aggregation/TermsAggregationBuilder.java:
##########
@@ -31,7 +31,10 @@ public final class TermsAggregationBuilder implements AggregationBuilder {
private Integer size;
private ImmutableMap.Builder<String, Aggregation> subAggregations;
- TermsAggregationBuilder(final String name) {
+ private CollectMode collectMode;
+ private ExecutionHint executionHint;
+
+ public TermsAggregationBuilder(final String name) {
Review Comment:
```suggestion
TermsAggregationBuilder(final String name) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #8959:
URL: https://github.com/apache/skywalking/pull/8959#discussion_r864839387
##########
docs/en/changes/changes.md:
##########
@@ -35,7 +35,7 @@
* Add the support eBPF profiling field into the process entity.
* E2E: fix log test miss verify LAL and metrics.
* Enhance Converter mechanism in kernel level to make BanyanDB native feature more effective.
-
+* Add TermsAggregation properties collect_mode and execution_hint.
Review Comment:
```suggestion
* Add TermsAggregation properties collect_mode and execution_hint.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #8959:
URL: https://github.com/apache/skywalking/pull/8959#issuecomment-1110956261
Please update the change log, this improvement looks good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] chenhaipeng commented on a diff in pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
chenhaipeng commented on code in PR #8959:
URL: https://github.com/apache/skywalking/pull/8959#discussion_r859712662
##########
oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/requests/search/aggregation/TermsAggregation.java:
##########
@@ -58,6 +58,8 @@ public void serialize(final TermsAggregation value, final JsonGenerator gen,
if (value.getOrder() != null) {
writeOrder(value, gen);
}
+ gen.writeStringField("execution_hint", "map");
+ gen.writeStringField("collect_mode", "breadth_first");
Review Comment:
please see here https://www.elastic.co/guide/en/elasticsearch/reference/6.8/search-aggregations-bucket-terms-aggregation.html#CO114-1
Because the metrics data is relatively small, using breadth_first, and execution_hint map will have a huge query improvement。
I did a simple test
When the index data is empty, "execution_hint" is not added, "map" took 30209ms, and the query timeout
When adding "execution_hint ""map" took 556ms
There are also similar question by other https://stackoverflow.com/questions/50893653/elasticsearch-slow-terms-aggregation/50895792#50895792
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #8959:
URL: https://github.com/apache/skywalking/pull/8959#discussion_r860376121
##########
oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/requests/search/aggregation/TermsAggregation.java:
##########
@@ -58,6 +58,8 @@ public void serialize(final TermsAggregation value, final JsonGenerator gen,
if (value.getOrder() != null) {
writeOrder(value, gen);
}
+ gen.writeStringField("execution_hint", "map");
+ gen.writeStringField("collect_mode", "breadth_first");
Review Comment:
Please don't do this in the codec level.
- Add fields `executionHint`, `collectMode` in `TermsAggregation` (you can make them enum type considering their possible values are limited in ES's spec).
- Add fields `executionHint`, `collectMode` in `TermsAggregationBuilder` and corresponding builder methods, pass the two to `TermsAggregation` constructor in `build` method.
- Write the 2 fields here only when they are not empty/blank.
Then you should change the 2 fields in topology query.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #8959:
URL: https://github.com/apache/skywalking/pull/8959#discussion_r860358827
##########
oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/requests/search/aggregation/TermsAggregation.java:
##########
@@ -58,6 +58,8 @@ public void serialize(final TermsAggregation value, final JsonGenerator gen,
if (value.getOrder() != null) {
writeOrder(value, gen);
}
+ gen.writeStringField("execution_hint", "map");
+ gen.writeStringField("collect_mode", "breadth_first");
Review Comment:
Make sense. 2 things
1. Please update `changes.md` in the doc, we have ref in the PR template(but you deleted it)
2. Moving this change into ElasticSearch DAO. `TermsAggregation` is our version of the ElasticSearch client, it should not include OAP's internal logic.
Others are good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] chenhaipeng commented on pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
chenhaipeng commented on PR #8959:
URL: https://github.com/apache/skywalking/pull/8959#issuecomment-1117380207
> @chenhaipeng It seems you still have code style issues.
ok,I fix it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #8959:
URL: https://github.com/apache/skywalking/pull/8959#discussion_r864837605
##########
oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/requests/search/aggregation/TermsAggregationBuilder.java:
##########
@@ -84,4 +99,34 @@ private ImmutableMap.Builder<String, Aggregation> subAggregations() {
}
return subAggregations;
}
+
+ public enum CollectMode {
+ BREADTH_FIRST("breadth_first"), DEPTH_FIRST("depth_first");
+
+ final String value;
+
+ CollectMode(final String value) {
+ this.value = value;
+ }
+
+ @Override
+ public String toString() {
+ return value;
+ }
+ }
+
+ public enum ExecutionHint {
+ GLOBAL_ORDINALS("global_ordinals"), map("map");
Review Comment:
```suggestion
GLOBAL_ORDINALS("global_ordinals"), MAP("map");
```
I think code style will fail due to lower case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #8959: Improve the performance of TermsAggregation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #8959:
URL: https://github.com/apache/skywalking/pull/8959#issuecomment-1117346596
@chenhaipeng It seems you still have code style issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org