wu-sheng commented on a change in pull request #8193:
URL: https://github.com/apache/skywalking/pull/8193#discussion_r757829119
##########
File path:
oap-server/server-receiver-plugin/envoy-metrics-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/envoy/AccessLogServiceGRPCHandler.java
##########
@@ -105,7 +110,7 @@ public AccessLogServiceGRPCHandler(ModuleManager manager,
public void onNext(StreamAccessLogsMessage message) {
HistogramMetrics.Timer timer = histogram.createTimer();
try {
- if (isFirst) {
+ if (isFirst || (alwaysAnalyzeIdentity &&
message.getIdentifier() != null)) {
Review comment:
I think I know what is the gap here. Let me put in this way
1. `Envoy -> OAP` direct linking
All envoy open unclosed streaming because the connection is always open,
then the identifier only gets parsed once. This costs minimal in identifier
parsing, but we have been facing load not balanced. Then we were introducing
Satellite.
2. `Envoy->Satellite->OAP`, current satellite implementation.
It maintains the context for a bulk of ALS message(such as 100 per bulk),
then it created stream every time. With this solution, the load is balanced
like you have seen in previous Satellite PR.
This change doesn't require any OAP side change, but actually, it already
introduce the extra identifier parsing logic, you raised here(which is why I
can't understand for a while, this is never introduced by this change).
One side, maintaining unclosed streaming in Satellite is impossible(costing
more CPUs), also, keeping open/close streaming(current status) is also not good.
Then, we get the idea of this new protocol and new PR.
We are merging these streamings as one new streaming(unclosed this time) but
carrying different identifiers.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
