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/02/16 12:17:11 UTC
[GitHub] [skywalking] aderm opened a new pull request #4371: fix topology
node type unknown.
aderm opened a new pull request #4371: fix topology node type unknown.
URL: https://github.com/apache/skywalking/pull/4371
Please answer these questions before submitting pull request
- Why submit this pull request?
- [x] Bug fix
- [ ] New feature provided
- [ ] Improve performance
- Related issues
#4370
___
### Bug fix
- Bug description.
Interceptor uses different components to intercept in different requests, httyasyncclient ( `componentId=26` ) and rest-highlevel-client ( `componentId=77` ), but the node type cannot be identified by httyasyncclient.
- How to fix?
Update with Known Node Type
___
### 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] wu-sheng commented on issue #4371: fix topology node
type unknown.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4371: fix topology node type unknown.
URL: https://github.com/apache/skywalking/pull/4371#issuecomment-587409041
Yes. They should be. All request starts from es client jar, should be wrapped inside the es exit span.
----------------------------------------------------------------
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] aderm edited a comment on issue #4371: fix topology
node type unknown.
Posted by GitBox <gi...@apache.org>.
aderm edited a comment on issue #4371: fix topology node type unknown.
URL: https://github.com/apache/skywalking/pull/4371#issuecomment-586702666
> > Interceptor uses different components to intercept in different requests, httyasyncclient ( componentId=26 ) and rest-highlevel-client ( componentId=77 ), but the node type cannot be identified by httyasyncclient.
>
> I could understand your fix, but the above description seems that, unknown caused by `httyasyncclient` doesn't have server side component mapping. If that is set, you are facing two kinds of target components.
>
> I am not objecting this fix, I just mean, the component id still could be wrong.
not httyasyncclient, is `httpasyncclient`.
`httpasyncclient` belongs to a common network component. cannot really infer the node type through it, and its type cannot be set.
Theoretically, there should be only one node type. If there are two different types inferred from A and B, it can only be a configuration error.
----------------------------------------------------------------
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] aderm commented on issue #4371: fix topology node type
unknown.
Posted by GitBox <gi...@apache.org>.
aderm commented on issue #4371: fix topology node type unknown.
URL: https://github.com/apache/skywalking/pull/4371#issuecomment-587405183
> > For httyasyncclient (componentId = 26) the plugin scenario test healthCheck interface ClusterHealthResponse response = client.cluster (). Health (request, RequestOptions.DEFAULT) generate exitspan
>
> Then this is the root cause. We should fix this create a exist span in ES, wrapping the existing httyasyncclient span. Then this component=26 span would be overrided.
the problem is that there are many other rest-highlevel-client interfaces similar to the above. need to all wrap in rest-highlevel-client interceptor? ES another rest client `Low-Level-Rest-Client` which the underlying is httyasyncclient Implementation may also have the same problem.
----------------------------------------------------------------
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 #4371: fix topology node
type unknown.
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #4371: fix topology node type unknown.
URL: https://github.com/apache/skywalking/pull/4371#issuecomment-588570164
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4371?src=pr&el=h1) Report
> Merging [#4371](https://codecov.io/gh/apache/skywalking/pull/4371?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/9a73c59cc621ff7677dbd684860abb40b641156c?src=pr&el=desc) will **decrease** coverage by `0.76%`.
> The diff coverage is `0.94%`.
[](https://codecov.io/gh/apache/skywalking/pull/4371?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4371 +/- ##
==========================================
- Coverage 26.2% 25.43% -0.77%
==========================================
Files 1182 1205 +23
Lines 27020 27864 +844
Branches 3726 3841 +115
==========================================
+ Hits 7080 7088 +8
- Misses 19302 20137 +835
- Partials 638 639 +1
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4371?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...storage/plugin/influxdb/installer/H2Installer.java](https://codecov.io/gh/apache/skywalking/pull/4371/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItc3RvcmFnZS1wbHVnaW4vc3RvcmFnZS1pbmZsdXhkYi1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvb2FwL3NlcnZlci9zdG9yYWdlL3BsdWdpbi9pbmZsdXhkYi9pbnN0YWxsZXIvSDJJbnN0YWxsZXIuamF2YQ==) | `0% <0%> (ø)` | |
| [...r/storage/plugin/influxdb/InfluxStorageConfig.java](https://codecov.io/gh/apache/skywalking/pull/4371/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItc3RvcmFnZS1wbHVnaW4vc3RvcmFnZS1pbmZsdXhkYi1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvb2FwL3NlcnZlci9zdG9yYWdlL3BsdWdpbi9pbmZsdXhkYi9JbmZsdXhTdG9yYWdlQ29uZmlnLmphdmE=) | `0% <0%> (ø)` | |
| [.../server/storage/plugin/influxdb/base/BatchDAO.java](https://codecov.io/gh/apache/skywalking/pull/4371/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItc3RvcmFnZS1wbHVnaW4vc3RvcmFnZS1pbmZsdXhkYi1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvb2FwL3NlcnZlci9zdG9yYWdlL3BsdWdpbi9pbmZsdXhkYi9iYXNlL0JhdGNoREFPLmphdmE=) | `0% <0%> (ø)` | |
| [...er/storage/plugin/influxdb/query/MetricsQuery.java](https://codecov.io/gh/apache/skywalking/pull/4371/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItc3RvcmFnZS1wbHVnaW4vc3RvcmFnZS1pbmZsdXhkYi1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvb2FwL3NlcnZlci9zdG9yYWdlL3BsdWdpbi9pbmZsdXhkYi9xdWVyeS9NZXRyaWNzUXVlcnkuamF2YQ==) | `0% <0%> (ø)` | |
| [...torage/plugin/influxdb/query/ProfileTaskQuery.java](https://codecov.io/gh/apache/skywalking/pull/4371/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItc3RvcmFnZS1wbHVnaW4vc3RvcmFnZS1pbmZsdXhkYi1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvb2FwL3NlcnZlci9zdG9yYWdlL3BsdWdpbi9pbmZsdXhkYi9xdWVyeS9Qcm9maWxlVGFza1F1ZXJ5LmphdmE=) | `0% <0%> (ø)` | |
| [...gin/influxdb/query/ProfileThreadSnapshotQuery.java](https://codecov.io/gh/apache/skywalking/pull/4371/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItc3RvcmFnZS1wbHVnaW4vc3RvcmFnZS1pbmZsdXhkYi1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvb2FwL3NlcnZlci9zdG9yYWdlL3BsdWdpbi9pbmZsdXhkYi9xdWVyeS9Qcm9maWxlVGhyZWFkU25hcHNob3RRdWVyeS5qYXZh) | `0% <0%> (ø)` | |
| [...age/plugin/influxdb/query/ProfileTaskLogQuery.java](https://codecov.io/gh/apache/skywalking/pull/4371/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItc3RvcmFnZS1wbHVnaW4vc3RvcmFnZS1pbmZsdXhkYi1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvb2FwL3NlcnZlci9zdG9yYWdlL3BsdWdpbi9pbmZsdXhkYi9xdWVyeS9Qcm9maWxlVGFza0xvZ1F1ZXJ5LmphdmE=) | `0% <0%> (ø)` | |
| [...storage/plugin/influxdb/InfluxStorageProvider.java](https://codecov.io/gh/apache/skywalking/pull/4371/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItc3RvcmFnZS1wbHVnaW4vc3RvcmFnZS1pbmZsdXhkYi1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvb2FwL3NlcnZlci9zdG9yYWdlL3BsdWdpbi9pbmZsdXhkYi9JbmZsdXhTdG9yYWdlUHJvdmlkZXIuamF2YQ==) | `0% <0%> (ø)` | |
| [...rver/storage/plugin/influxdb/query/AlarmQuery.java](https://codecov.io/gh/apache/skywalking/pull/4371/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItc3RvcmFnZS1wbHVnaW4vc3RvcmFnZS1pbmZsdXhkYi1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvb2FwL3NlcnZlci9zdG9yYWdlL3BsdWdpbi9pbmZsdXhkYi9xdWVyeS9BbGFybVF1ZXJ5LmphdmE=) | `0% <0%> (ø)` | |
| [...server/storage/plugin/influxdb/query/LogQuery.java](https://codecov.io/gh/apache/skywalking/pull/4371/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItc3RvcmFnZS1wbHVnaW4vc3RvcmFnZS1pbmZsdXhkYi1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvb2FwL3NlcnZlci9zdG9yYWdlL3BsdWdpbi9pbmZsdXhkYi9xdWVyeS9Mb2dRdWVyeS5qYXZh) | `0% <0%> (ø)` | |
| ... and [37 more](https://codecov.io/gh/apache/skywalking/pull/4371/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4371?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/4371?src=pr&el=footer). Last update [9a73c59...01c557b](https://codecov.io/gh/apache/skywalking/pull/4371?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
[GitHub] [skywalking] aderm commented on issue #4371: fix topology node type
unknown.
Posted by GitBox <gi...@apache.org>.
aderm commented on issue #4371: fix topology node type unknown.
URL: https://github.com/apache/skywalking/pull/4371#issuecomment-586702666
> > Interceptor uses different components to intercept in different requests, httyasyncclient ( componentId=26 ) and rest-highlevel-client ( componentId=77 ), but the node type cannot be identified by httyasyncclient.
>
> I could understand your fix, but the above description seems that, unknown caused by `httyasyncclient` doesn't have server side component mapping. If that is set, you are facing two kinds of target components.
>
> I am not objecting this fix, I just mean, the component id still could be wrong.
not httyasyncclient but `httyasyncclient`.
`httpasyncclient` belongs to a common network component. cannot really infer the node type through it, and its type cannot be set.
Theoretically, there should be only one node type. If there are two different types inferred from A and B, it can only be a configuration error.
----------------------------------------------------------------
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] aderm edited a comment on issue #4371: fix topology
node type unknown.
Posted by GitBox <gi...@apache.org>.
aderm edited a comment on issue #4371: fix topology node type unknown.
URL: https://github.com/apache/skywalking/pull/4371#issuecomment-586702666
> > Interceptor uses different components to intercept in different requests, httyasyncclient ( componentId=26 ) and rest-highlevel-client ( componentId=77 ), but the node type cannot be identified by httyasyncclient.
>
> I could understand your fix, but the above description seems that, unknown caused by `httyasyncclient` doesn't have server side component mapping. If that is set, you are facing two kinds of target components.
>
> I am not objecting this fix, I just mean, the component id still could be wrong.
not httyasyncclient but `httpasyncclient`.
`httpasyncclient` belongs to a common network component. cannot really infer the node type through it, and its type cannot be set.
Theoretically, there should be only one node type. If there are two different types inferred from A and B, it can only be a configuration error.
----------------------------------------------------------------
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 #4371: fix topology node
type unknown.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4371: fix topology node type unknown.
URL: https://github.com/apache/skywalking/pull/4371#issuecomment-586700626
> Interceptor uses different components to intercept in different requests, httyasyncclient ( componentId=26 ) and rest-highlevel-client ( componentId=77 ), but the node type cannot be identified by httyasyncclient.
I could understand your fix, but the above description seems that, unknown caused by `httyasyncclient` doesn't have server side component mapping. If that is set, you are facing two kinds of target components.
I am not objecting this fix, I just mean, the component id still could be wrong.
----------------------------------------------------------------
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 #4371: fix topology node
type unknown.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4371: fix topology node type unknown.
URL: https://github.com/apache/skywalking/pull/4371#issuecomment-587353320
> For httyasyncclient (componentId = 26) the plugin scenario test healthCheck interface ClusterHealthResponse response = client.cluster (). Health (request, RequestOptions.DEFAULT) generate exitspan
Then this is the root cause. We should fix this create a exist span in ES, wrapping the existing httyasyncclient span. Then this component=26 span would be overrided.
----------------------------------------------------------------
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 #4371:
fix topology node type unknown.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4371: fix topology node type unknown.
URL: https://github.com/apache/skywalking/pull/4371#discussion_r379899569
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/TopologyBuilder.java
##########
@@ -78,11 +79,20 @@ Topology build(List<Call.CallDetail> serviceRelationClientCalls, List<Call.CallD
nodes.put(source.getSequence(), buildNode(source));
}
- if (!nodes.containsKey(target.getSequence())) {
- nodes.put(target.getSequence(), buildNode(target));
+ int sequence = target.getSequence();
+ if (nodes.containsKey(sequence)) {
+ //if node type is Unknown, but some targer know node type, so update the node type.
Review comment:
```suggestion
//if node type is Unknown, but the target may know the node type.
```
----------------------------------------------------------------
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] aderm commented on issue #4371: fix topology node type
unknown.
Posted by GitBox <gi...@apache.org>.
aderm commented on issue #4371: fix topology node type unknown.
URL: https://github.com/apache/skywalking/pull/4371#issuecomment-587104343
> My point is, someone could set that mapping, then you will be back to facing two component.
Do need to add another check mechanism to prevent duplicate configuration?
----------------------------------------------------------------
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 #4371: fix topology node
type unknown.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4371: fix topology node type unknown.
URL: https://github.com/apache/skywalking/pull/4371#issuecomment-587213383
We need to look deeper what the scenario you described. Inside `elasticsearch-7.x-scenario`, I can't find the span you described, is it missed?
----------------------------------------------------------------
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] aderm commented on issue #4371: fix topology node type
unknown.
Posted by GitBox <gi...@apache.org>.
aderm commented on issue #4371: fix topology node type unknown.
URL: https://github.com/apache/skywalking/pull/4371#issuecomment-587352319
> We need to look deeper what the scenario you described. Inside `elasticsearch-7.x-scenario`, I can't find the span you described, is it missed?
client-side info httyasyncclient ( componentId=26 ) and rest-highlevel-client ( componentId=77 ), are generated by two different endpoint Requests. For httyasyncclient (componentId = 26) the plugin scenario test healthCheck interface `
ClusterHealthResponse response = client.cluster (). Health (request, RequestOptions.DEFAULT) ` generate exitspan; rest-highlevel-client (componentId = 77) the entry span generated by other requests that intercept ES queries through rest-highlevel-client.
<img width="731" alt="WX20200218-165406@2x" src="https://user-images.githubusercontent.com/2892433/74720340-9a7e3a80-5270-11ea-9488-01485098b6e5.png">
<img width="731" alt="WX20200218-165406@2x" src="https://user-images.githubusercontent.com/2892433/74720385-ad910a80-5270-11ea-98b5-aca8e57e34e6.png">
----------------------------------------------------------------
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 #4371: fix topology node
type unknown.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4371: fix topology node type unknown.
URL: https://github.com/apache/skywalking/pull/4371#issuecomment-586717162
> not httyasyncclient, is httpasyncclient.
httpasyncclient belongs to a common network component. cannot really infer the node type through it, and its type cannot be set.
My point is, someone could set that mapping, then you will be back to facing two component.
Again, I am not saying this chance is wrong, but this could not fix the issue in some cases.
BTW, you need to fix the tests.
----------------------------------------------------------------
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 closed pull request #4371: fix topology node
type unknown.
Posted by GitBox <gi...@apache.org>.
wu-sheng closed pull request #4371: fix topology node type unknown.
URL: https://github.com/apache/skywalking/pull/4371
----------------------------------------------------------------
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 #4371: fix topology node
type unknown.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4371: fix topology node type unknown.
URL: https://github.com/apache/skywalking/pull/4371#issuecomment-587212719
> Do need to add another check mechanism to prevent duplicate configuration?
We can't. And there is another question about ES plugin, FYI @dmsolr , why ES client creates two exit spans? If two spans created, the traffic(such as CPM) of ES server seems wrong(1 request == 2 exit span == 2 request for ES server)
----------------------------------------------------------------
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