Make source builder set service name and endpoint name in right order (#6188)

CHANGES.md

@@ -61,6 +61,7 @@ Release Notes.
 * Fix `timeBucket` not taking effect in EqualsAndHashCode annotation of some relationship metrics.
 * Fix `SharingServerConfig`'s propertie is not correct in the `application.yml`, contextPath -> restConnextPath.
 * Istio control plane: remove redundant metrics and polish panel layout.
+* Fix bug endpoint name grouping not work due to setting service name and endpoint name out of order.
 * Fix receiver analysis error count metrics
 * Log collecting and query implementation
 

oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/DatabaseSlowStatementBuilder.java

+/*
+ * 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.oap.server.analyzer.provider.trace.parser.listener;
+
+import lombok.Getter;
+import lombok.RequiredArgsConstructor;
+import lombok.Setter;
+import org.apache.skywalking.oap.server.core.analysis.IDManager;
+import org.apache.skywalking.oap.server.core.analysis.NodeType;
+import org.apache.skywalking.oap.server.core.config.NamingControl;
+import org.apache.skywalking.oap.server.core.source.DatabaseSlowStatement;
+
+@RequiredArgsConstructor
+public class DatabaseSlowStatementBuilder {
+    private final NamingControl namingControl;
+
+    @Getter
+    @Setter
+    private String id;
+    @Getter
+    @Setter
+    private String traceId;
+    @Getter
+    @Setter
+    private String serviceName;
+    @Getter
+    @Setter
+    private NodeType type = NodeType.Database;
+    @Getter
+    @Setter
+    private String statement;
+    @Getter
+    @Setter
+    private long latency;
+    @Getter
+    @Setter
+    private long timeBucket;
+
+    void prepare() {
+        this.serviceName = namingControl.formatServiceName(serviceName);
+    }
+
+    DatabaseSlowStatement toDatabaseSlowStatement() {
+        DatabaseSlowStatement dbSlowStat = new DatabaseSlowStatement();
+        dbSlowStat.setId(id);
+        dbSlowStat.setTraceId(traceId);
+        dbSlowStat.setDatabaseServiceId(IDManager.ServiceID.buildId(serviceName, type));
+        dbSlowStat.setStatement(statement);
+        dbSlowStat.setLatency(latency);
+        dbSlowStat.setTimeBucket(timeBucket);
+        return dbSlowStat;
+    }
+
+}

oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/MultiScopesAnalysisListener.java

@@ -42,7 +42,6 @@ import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
 import org.apache.skywalking.oap.server.core.analysis.manual.networkalias.NetworkAddressAlias;
 import org.apache.skywalking.oap.server.core.cache.NetworkAddressAliasCache;
 import org.apache.skywalking.oap.server.core.config.NamingControl;
-import org.apache.skywalking.oap.server.core.source.DatabaseSlowStatement;
 import org.apache.skywalking.oap.server.core.source.DetectPoint;
 import org.apache.skywalking.oap.server.core.source.EndpointRelation;
 import org.apache.skywalking.oap.server.core.source.RequestType;
@@ -62,7 +61,7 @@ import static org.apache.skywalking.oap.server.analyzer.provider.trace.parser.Sp
 public class MultiScopesAnalysisListener implements EntryAnalysisListener, ExitAnalysisListener, LocalAnalysisListener {
     private final List<SourceBuilder> entrySourceBuilders = new ArrayList<>(10);
     private final List<SourceBuilder> exitSourceBuilders = new ArrayList<>(10);
-    private final List<DatabaseSlowStatement> slowDatabaseAccesses = new ArrayList<>(10);
+    private final List<DatabaseSlowStatementBuilder> dbSlowStatementBuilders = new ArrayList<>(10);
     private final List<SourceBuilder> logicEndpointBuilders = new ArrayList<>(10);
     private final Gson gson = new Gson();
     private final SourceReceiver sourceReceiver;
@@ -195,26 +194,24 @@ public class MultiScopesAnalysisListener implements EntryAnalysisListener, ExitA
         setPublicAttrs(sourceBuilder, span);
         exitSourceBuilders.add(sourceBuilder);
 
-        if (sourceBuilder.getType().equals(RequestType.DATABASE)) {
+        if (RequestType.DATABASE.equals(sourceBuilder.getType())) {
             boolean isSlowDBAccess = false;
 
-            DatabaseSlowStatement statement = new DatabaseSlowStatement();
-            statement.setId(segmentObject.getTraceSegmentId() + "-" + span.getSpanId());
-            statement.setDatabaseServiceId(
-                IDManager.ServiceID.buildId(networkAddress, NodeType.Database)
-            );
-            statement.setLatency(sourceBuilder.getLatency());
-            statement.setTimeBucket(TimeBucket.getRecordTimeBucket(span.getStartTime()));
-            statement.setTraceId(segmentObject.getTraceId());
+            DatabaseSlowStatementBuilder slowStatementBuilder = new DatabaseSlowStatementBuilder(namingControl);
+            slowStatementBuilder.setServiceName(networkAddress);
+            slowStatementBuilder.setId(segmentObject.getTraceSegmentId() + "-" + span.getSpanId());
+            slowStatementBuilder.setLatency(sourceBuilder.getLatency());
+            slowStatementBuilder.setTimeBucket(TimeBucket.getRecordTimeBucket(span.getStartTime()));
+            slowStatementBuilder.setTraceId(segmentObject.getTraceId());
             for (KeyStringValuePair tag : span.getTagsList()) {
                 if (SpanTags.DB_STATEMENT.equals(tag.getKey())) {
                     String sqlStatement = tag.getValue();
                     if (StringUtil.isEmpty(sqlStatement)) {
-                        statement.setStatement("[No statement]/" + span.getOperationName());
+                        slowStatementBuilder.setStatement("[No statement]/" + span.getOperationName());
                     } else if (sqlStatement.length() > config.getMaxSlowSQLLength()) {
-                        statement.setStatement(sqlStatement.substring(0, config.getMaxSlowSQLLength()));
+                        slowStatementBuilder.setStatement(sqlStatement.substring(0, config.getMaxSlowSQLLength()));
                     } else {
-                        statement.setStatement(sqlStatement);
+                        slowStatementBuilder.setStatement(sqlStatement);
                     }
                 } else if (SpanTags.DB_TYPE.equals(tag.getKey())) {
                     String dbType = tag.getValue();
@@ -227,7 +224,7 @@ public class MultiScopesAnalysisListener implements EntryAnalysisListener, ExitA
             }
 
             if (isSlowDBAccess) {
-                slowDatabaseAccesses.add(statement);
+                dbSlowStatementBuilders.add(slowStatementBuilder);
             }
         }
     }
@@ -271,6 +268,7 @@ public class MultiScopesAnalysisListener implements EntryAnalysisListener, ExitA
     @Override
     public void build() {
         entrySourceBuilders.forEach(entrySourceBuilder -> {
+            entrySourceBuilder.prepare();
             sourceReceiver.receive(entrySourceBuilder.toAll());
             sourceReceiver.receive(entrySourceBuilder.toService());
             sourceReceiver.receive(entrySourceBuilder.toServiceInstance());
@@ -292,6 +290,7 @@ public class MultiScopesAnalysisListener implements EntryAnalysisListener, ExitA
         });
 
         exitSourceBuilders.forEach(exitSourceBuilder -> {
+            exitSourceBuilder.prepare();
             sourceReceiver.receive(exitSourceBuilder.toServiceRelation());
 
             /*
@@ -307,9 +306,13 @@ public class MultiScopesAnalysisListener implements EntryAnalysisListener, ExitA
             }
         });
 
-        slowDatabaseAccesses.forEach(sourceReceiver::receive);
+        dbSlowStatementBuilders.forEach(dbSlowStatBuilder -> {
+            dbSlowStatBuilder.prepare();
+            sourceReceiver.receive(dbSlowStatBuilder.toDatabaseSlowStatement());
+        });
 
         logicEndpointBuilders.forEach(logicEndpointBuilder -> {
+            logicEndpointBuilder.prepare();
             sourceReceiver.receive(logicEndpointBuilder.toEndpoint());
         });
     }

oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/SourceBuilder.java

@@ -44,64 +44,36 @@ class SourceBuilder {
     private final NamingControl namingControl;
 
     @Getter
+    @Setter
     private String sourceServiceName;
-
-    public void setSourceServiceName(final String sourceServiceName) {
-        this.sourceServiceName = namingControl.formatServiceName(sourceServiceName);
-    }
-
     @Getter
     @Setter
     private NodeType sourceNodeType;
     @Getter
+    @Setter
     private String sourceServiceInstanceName;
-
-    public void setSourceServiceInstanceName(final String sourceServiceInstanceName) {
-        this.sourceServiceInstanceName = namingControl.formatInstanceName(sourceServiceInstanceName);
-    }
-
     /**
      * Source endpoint could be not owned by {@link #sourceServiceName}, such as in the MQ or un-instrumented proxy
      * cases. This service always comes from the span.ref, so it is always a normal service.
      */
     @Getter
+    @Setter
     private String sourceEndpointOwnerServiceName;
-
-    public void setSourceEndpointOwnerServiceName(final String sourceServiceName) {
-        this.sourceEndpointOwnerServiceName = namingControl.formatServiceName(sourceServiceName);
-    }
-
     @Getter
+    @Setter
     private String sourceEndpointName;
-
-    public void setSourceEndpointName(final String sourceEndpointName) {
-        this.sourceEndpointName = namingControl.formatEndpointName(sourceServiceName, sourceEndpointName);
-    }
-
     @Getter
+    @Setter
     private String destServiceName;
-
-    public void setDestServiceName(final String destServiceName) {
-        this.destServiceName = namingControl.formatServiceName(destServiceName);
-    }
-
     @Getter
     @Setter
     private NodeType destNodeType;
     @Getter
+    @Setter
     private String destServiceInstanceName;
-
-    public void setDestServiceInstanceName(final String destServiceInstanceName) {
-        this.destServiceInstanceName = namingControl.formatServiceName(destServiceInstanceName);
-    }
-
     @Getter
+    @Setter
     private String destEndpointName;
-
-    public void setDestEndpointName(final String destEndpointName) {
-        this.destEndpointName = namingControl.formatEndpointName(destServiceName, destEndpointName);
-    }
-
     @Getter
     @Setter
     private int componentId;
@@ -126,6 +98,16 @@ class SourceBuilder {
     @Getter
     private final List<String> tags = new ArrayList<>();
 
+    void prepare() {
+        this.sourceServiceName = namingControl.formatServiceName(sourceServiceName);
+        this.sourceEndpointOwnerServiceName = namingControl.formatServiceName(sourceEndpointOwnerServiceName);
+        this.sourceServiceInstanceName = namingControl.formatInstanceName(sourceServiceInstanceName);
+        this.sourceEndpointName = namingControl.formatEndpointName(sourceServiceName, sourceEndpointName);
+        this.destServiceName = namingControl.formatServiceName(destServiceName);
+        this.destServiceInstanceName = namingControl.formatInstanceName(destServiceInstanceName);
+        this.destEndpointName = namingControl.formatEndpointName(destServiceName, destEndpointName);
+    }
+
     /**
      * The global level metrics source
      */

oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/config/NamingControl.java

@@ -20,6 +20,7 @@ package org.apache.skywalking.oap.server.core.config;
 
 import lombok.RequiredArgsConstructor;
 import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.apm.util.StringUtil;
 import org.apache.skywalking.oap.server.core.config.group.EndpointNameGrouping;
 import org.apache.skywalking.oap.server.library.module.Service;
 
@@ -44,7 +45,7 @@ public class NamingControl implements Service {
      * @return the string, which length less than or equals {@link #serviceNameMaxLength};
      */
     public String formatServiceName(String serviceName) {
-        if (serviceName.length() > serviceNameMaxLength) {
+        if (serviceName != null && serviceName.length() > serviceNameMaxLength) {
             final String rename = serviceName.substring(0, serviceNameMaxLength);
             if (log.isDebugEnabled()) {
                 log.debug(
@@ -69,7 +70,7 @@ public class NamingControl implements Service {
      * @return the string, which length less than or equals {@link #instanceNameMaxLength};
      */
     public String formatInstanceName(String instanceName) {
-        if (instanceName.length() > instanceNameMaxLength) {
+        if (instanceName != null && instanceName.length() > instanceNameMaxLength) {
             final String rename = instanceName.substring(0, instanceNameMaxLength);
             if (log.isDebugEnabled()) {
                 log.debug(
@@ -95,6 +96,10 @@ public class NamingControl implements Service {
      * @return the string, which length less than or equals {@link #endpointNameMaxLength};
      */
     public String formatEndpointName(String serviceName, String endpointName) {
+        if (StringUtil.isEmpty(serviceName) || endpointName == null) {
+            return endpointName;
+        }
+
         String lengthControlledName = endpointName;
         if (endpointName.length() > endpointNameMaxLength) {
             lengthControlledName = endpointName.substring(0, endpointNameMaxLength);