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 2020/01/15 09:14:08 UTC
[GitHub] [skywalking] Liu-XinYuan opened a new pull request #4236: Optimize
the elasticsearch-based query interface provided to UI
Liu-XinYuan opened a new pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236
Please answer these questions before submitting pull request
- Why submit this pull request?
- [ ] Bug fix
- [ ] New feature provided
- [x] Improve performance
- Related issues
___
### Bug fix
- Bug description.
#4194
- How to fix?
___
### New feature or improvement
- Describe the details and related test reports.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] Liu-XinYuan commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
Liu-XinYuan commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574610942
> You are talking about timestamp, But why we need that? All data and index names include the time bucket only.
Essentially we want to take the intersection of the query time interval and the actual time interval. This calculation needs to be compared with the current time and incremental calculation of time, so the bucket needs to be converted to timestamp.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] Liu-XinYuan commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
Liu-XinYuan commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-575009867
Structurally, I certainly hope that the logic of time conversion is put into the ElasticSearchClient class, so that the change area is small. And I want to put the time conversion logic into DurationUtils in order to keep the same logic.
In this case, the ElasticSearchClient class will depend on the DurationUtils class, causing a circular dependency. This caused each class in the server-storage-plugin to be added with the time conversion logic.
But for the time being, I can put the time conversion logic into ElasticSearchClient without relying on DurationUtils. I will try
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] Liu-XinYuan commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
Liu-XinYuan commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574581817
This pr has two important methods. The first is formatIndexNames. It determines the index list based on aliases and restrictions. The second method is convertBucketTotIimestamp. It converts bucket time to timestamp as a parameter of formatIndexNames
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] Liu-XinYuan commented on a change in pull request
#4236: Optimize the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
Liu-XinYuan commented on a change in pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#discussion_r366773430
##########
File path: oap-server/server-library/library-client/src/main/java/org/apache/skywalking/oap/server/library/client/elasticsearch/ElasticSearchClient.java
##########
@@ -404,4 +434,65 @@ public String formatIndexName(String indexName) {
}
return indexName;
}
+
+ private String[] formatIndexNames(String indexName, long startTimestamp, long endTimestamp, String traceId) throws IOException {
Review comment:
The logic of determining the time according to the traceId can be removed, and different implementations of agents in other languages were not considered at the time
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4236:
Optimize the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#discussion_r367921235
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/DurationUtils.java
##########
@@ -216,4 +216,59 @@ private DateTime parseToDateTime(Downsampling downsampling, long time) throws Pa
return dateTime;
}
+
+ public long convertBucketTotIimestamp(boolean isStart, long bucket) throws ParseException {
Review comment:
First, timeBucket isn't in the format you are demonstrating. Then, I am not talking who can put what codes in any place. My point is always being, put the storage optimization codes inside the storage implementation. Don't change the storage model interface unless you have to.
> why can't I put it into DurationUtils so that he can reuse this logic?
This question is really not a good way to discuss the question. I hope you wouldn't do this again. The things we discussed is not about whom, @dmsolr is showing something he is working on, his PR is in the WIP status, no review/merge.
Also, you should understand, as an official committer, he actually really has some privileges to change APIs, it could be easier than you. Because he has been involved in source codes level a long time, and be familiar with the workflow (You could simply check his GitHub repo changelog). But, even there is a privilege, back to this case, I or he never use this priority. So, being an open source project, please don't consider discussion(arguing between you and others) in this way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] Liu-XinYuan commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
Liu-XinYuan commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574998585
I know some code can continue to optimize
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] Liu-XinYuan commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
Liu-XinYuan commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574598069
Mainly because the MetadataQuery class uses the StartTimeToTimestamp and endTimeToTimestamp methods of DurationUtils to convert the time passed by the front end.
So in order to maintain consistency, the new convertBucketTotIimestamp method in the DurationUtils class uses startTimeToTimestamp and endTimeToTimestamp to convert time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4236:
Optimize the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#discussion_r366770871
##########
File path: oap-server/server-library/library-client/src/main/java/org/apache/skywalking/oap/server/library/client/elasticsearch/ElasticSearchClient.java
##########
@@ -303,6 +322,17 @@ public SearchResponse ids(String indexName, String[] ids) throws IOException {
return client.search(searchRequest);
}
+ public SearchResponse ids(String indexName, String[] ids, long startTimestamp, long endTimestamp) throws IOException {
Review comment:
Where is the old `ids`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] Liu-XinYuan commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
Liu-XinYuan commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574594286
What I mean by determining the index list is to determine the position of the query data according to the time window, in order to reduce unnecessary index queries.
Do you mean time by bucket?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] Liu-XinYuan commented on a change in pull request
#4236: Optimize the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
Liu-XinYuan commented on a change in pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#discussion_r366776473
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/DurationUtils.java
##########
@@ -216,4 +216,59 @@ private DateTime parseToDateTime(Downsampling downsampling, long time) throws Pa
return dateTime;
}
+
+ public long convertBucketTotIimestamp(boolean isStart, long bucket) throws ParseException {
+ String bucketStr = String.valueOf(bucket);
+ if (bucketStr.length() < 14) {
Review comment:
According to the existing conversion rules, there is a short time such as 20200115
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574620039
That is my point, we don't. The es ttl is based on day and month only. By considering downsampling, we have enough information already. Please rethink again, converting to timestamp is not helping.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574604694
You are talking about timestamp, But why we need that? All data and index names include the time bucket only.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] Liu-XinYuan commented on a change in pull request
#4236: Optimize the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
Liu-XinYuan commented on a change in pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#discussion_r367752137
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/DurationUtils.java
##########
@@ -216,4 +216,59 @@ private DateTime parseToDateTime(Downsampling downsampling, long time) throws Pa
return dateTime;
}
+
+ public long convertBucketTotIimestamp(boolean isStart, long bucket) throws ParseException {
Review comment:
So my conversion logic remains in DurationUtils?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] Liu-XinYuan commented on a change in pull request
#4236: Optimize the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
Liu-XinYuan commented on a change in pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#discussion_r367754149
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/DurationUtils.java
##########
@@ -216,4 +216,59 @@ private DateTime parseToDateTime(Downsampling downsampling, long time) throws Pa
return dateTime;
}
+
+ public long convertBucketTotIimestamp(boolean isStart, long bucket) throws ParseException {
Review comment:
I know that to optimize the elasticsearch query, you should put the code on the elasticsearch implementation class. @dmsolr will use the same conversion logic as me, why can't I put it into DurationUtils so that he can reuse this logic?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] Liu-XinYuan removed a comment on issue #4236: Optimize
the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
Liu-XinYuan removed a comment on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574998585
I know some code can continue to optimize
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574583389
> This pr has two important methods. The first is formatIndexNames. It determines the index list based on aliases and restrictions. The second method is convertBucketTotIimestamp. It converts bucket time to timestamp as a parameter of formatIndexNames
First, why timestamp as a parameter of index name, rather than timeBucket? I remember timeBucket should be suffix of index name. What changed?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] dmsolr commented on a change in pull request #4236:
Optimize the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
dmsolr commented on a change in pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#discussion_r367755968
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/DurationUtils.java
##########
@@ -216,4 +216,59 @@ private DateTime parseToDateTime(Downsampling downsampling, long time) throws Pa
return dateTime;
}
+
+ public long convertBucketTotIimestamp(boolean isStart, long bucket) throws ParseException {
Review comment:
In fact, I don't understand why we need this? On ES, TB or TS, have any difference?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574966988
I know the codes are different. My point is, logically, they are doing very similar things. Again, I am not saying your codes are wrong, I just hope you could keep the changes as less as possible, and as clear as possible. Now, your codes include a lot of changes, mixed with existing logic. Some of these changes are not necessary for the way today.
Maybe this makes you feel frustrating, but I have to set these requirements, as I also set the same requirements at #4220 review. All codes we accepted are the legacy of all contributors and users. We must keep them friendly to ready. Think in this way, you could simply change codes to finish what you want because all former contributors accepted these requirements.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574967177
Your change should be clearly using time range and unit(downsampling) to calulate the indexes, right? But look at your codes, they are not clear.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574573373
You are doing a lot convert but no clear meaning and assumpting traceId is meaningful, which isn't.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] Liu-XinYuan commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
Liu-XinYuan commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574965601
TTL and index positioning have these differences:
1.ttl is an internal timing task. It traverses all models, calculates s according to the predefined index retention time, then deletes the lower boundary of the index, and calculates the upper boundary according to the current time. Then delete the index outside this interval. Only the two parameters of downsampling (carried in the modle) and the defined data ttl time are needed here.
2. Index positioning needs to calculate the interval within a user-defined time. The user-defined time will be converted into other forms in the query-graphql-plugin. I must unify the time form in order to use it in index positioning.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] Liu-XinYuan commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
Liu-XinYuan commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574590290
> First, why timestamp as a parameter of index name, rather than timeBucket? I remember timeBucket should be suffix of index name. What changed?
Buckets have different forms, but it is not a timestamp. The timestamp after this conversion is needed to determine the index list.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] Liu-XinYuan commented on a change in pull request
#4236: Optimize the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
Liu-XinYuan commented on a change in pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#discussion_r367775866
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/DurationUtils.java
##########
@@ -216,4 +216,59 @@ private DateTime parseToDateTime(Downsampling downsampling, long time) throws Pa
return dateTime;
}
+
+ public long convertBucketTotIimestamp(boolean isStart, long bucket) throws ParseException {
Review comment:
TB represents a time bucket, such as 2020-01-17 1331 or 2020-01-17 13. TS is a time stamp such as 1579239147000. I need to convert TB to TS in order to unify time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574591688
That is not I expected and should be unnecessary. The index list could be determined through time directly as TTL is a solid rule.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] dmsolr commented on a change in pull request #4236:
Optimize the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
dmsolr commented on a change in pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#discussion_r367751274
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/DurationUtils.java
##########
@@ -216,4 +216,59 @@ private DateTime parseToDateTime(Downsampling downsampling, long time) throws Pa
return dateTime;
}
+
+ public long convertBucketTotIimestamp(boolean isStart, long bucket) throws ParseException {
Review comment:
I have the same requirement. Need to convert the TB to Timestamp to make InfluxDB RelentationPolicy work.
https://github.com/apache/skywalking/blob/484e078399f90e2b11d9bc16a83c76bbc1d4921b/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/TimeBucket.java#L53-L67
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4236:
Optimize the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#discussion_r366768356
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/DurationUtils.java
##########
@@ -216,4 +216,59 @@ private DateTime parseToDateTime(Downsampling downsampling, long time) throws Pa
return dateTime;
}
+
+ public long convertBucketTotIimestamp(boolean isStart, long bucket) throws ParseException {
Review comment:
First, you have a typo `Timestampe`.
Then, this method should be separated into 2 methods, by removing `isStart` as a parameter. It is very unclear.
1. startBucketTime2Timestamp
1. endBucketTime2Timestamp
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4236:
Optimize the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#discussion_r366765622
##########
File path: oap-server/server-library/library-client/src/main/java/org/apache/skywalking/oap/server/library/client/elasticsearch/ElasticSearchClient.java
##########
@@ -404,4 +434,65 @@ public String formatIndexName(String indexName) {
}
return indexName;
}
+
+ private String[] formatIndexNames(String indexName, long startTimestamp, long endTimestamp, String traceId) throws IOException {
Review comment:
Why include `traceId`? TraceId generation rule is only suitable in Java codebase. We shouldn't count on it at any time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574580973
Also, I noticed the e2e fails.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] Liu-XinYuan commented on a change in pull request
#4236: Optimize the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
Liu-XinYuan commented on a change in pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#discussion_r366773815
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/DurationUtils.java
##########
@@ -216,4 +216,59 @@ private DateTime parseToDateTime(Downsampling downsampling, long time) throws Pa
return dateTime;
}
+
+ public long convertBucketTotIimestamp(boolean isStart, long bucket) throws ParseException {
Review comment:
This is a good suggestion
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4236:
Optimize the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#discussion_r366768474
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/DurationUtils.java
##########
@@ -216,4 +216,59 @@ private DateTime parseToDateTime(Downsampling downsampling, long time) throws Pa
return dateTime;
}
+
+ public long convertBucketTotIimestamp(boolean isStart, long bucket) throws ParseException {
+ String bucketStr = String.valueOf(bucket);
+ if (bucketStr.length() < 14) {
Review comment:
What is this for?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] dmsolr commented on a change in pull request #4236:
Optimize the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
dmsolr commented on a change in pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#discussion_r367798768
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/DurationUtils.java
##########
@@ -216,4 +216,59 @@ private DateTime parseToDateTime(Downsampling downsampling, long time) throws Pa
return dateTime;
}
+
+ public long convertBucketTotIimestamp(boolean isStart, long bucket) throws ParseException {
Review comment:
I see TB means TimeBucket and its form.
I am confused about why we need to uniform them. The form(TB/TS) is not determined by the storage layer. So you covert the TB to TS on write and covert it again on the query.
In other words, whether ES depends on TS. (Maybe I miss something.)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4236:
Optimize the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#discussion_r366777682
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/DurationUtils.java
##########
@@ -216,4 +216,59 @@ private DateTime parseToDateTime(Downsampling downsampling, long time) throws Pa
return dateTime;
}
+
+ public long convertBucketTotIimestamp(boolean isStart, long bucket) throws ParseException {
+ String bucketStr = String.valueOf(bucket);
+ if (bucketStr.length() < 14) {
Review comment:
Still not following. Timebucket always could be determined by length, why separate by 14?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] Liu-XinYuan commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
Liu-XinYuan commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-574570386
Time conversion is placed in server-storage-plugin module
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4236:
Optimize the elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#discussion_r367752310
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/DurationUtils.java
##########
@@ -216,4 +216,59 @@ private DateTime parseToDateTime(Downsampling downsampling, long time) throws Pa
return dateTime;
}
+
+ public long convertBucketTotIimestamp(boolean isStart, long bucket) throws ParseException {
Review comment:
Again, logic or util is not the issue. Today, the issue is the codes are not clear and separated and being injected out of ES implementation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] codecov-io commented on issue #4236: Optimize the
elasticsearch-based query interface provided to UI
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #4236: Optimize the elasticsearch-based query interface provided to UI
URL: https://github.com/apache/skywalking/pull/4236#issuecomment-575008923
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4236?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@c9732ec`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/skywalking/pull/4236?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4236 +/- ##
=========================================
Coverage ? 26.16%
=========================================
Files ? 1179
Lines ? 26069
Branches ? 3797
=========================================
Hits ? 6820
Misses ? 18646
Partials ? 603
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4236?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/skywalking/pull/4236?src=pr&el=footer). Last update [c9732ec...9ac94d6](https://codecov.io/gh/apache/skywalking/pull/4236?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services