From 825a7ccc817a0af30cdd8ffc4eb6dee81b4178aa Mon Sep 17 00:00:00 2001 From: Neal Huang Date: Thu, 14 Jan 2021 20:51:26 +0800 Subject: [PATCH] Make source builder set service name and endpoint name in right order (#6188) --- CHANGES.md | 1 + .../DatabaseSlowStatementBuilder.java | 70 +++++++++++++++++++ .../listener/MultiScopesAnalysisListener.java | 35 +++++----- .../trace/parser/listener/SourceBuilder.java | 52 +++++--------- .../oap/server/core/config/NamingControl.java | 9 ++- 5 files changed, 114 insertions(+), 53 deletions(-) create mode 100644 oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/DatabaseSlowStatementBuilder.java diff --git a/CHANGES.md b/CHANGES.md index 79e8a11cc3..b1b0122d40 100644 --- a/CHANGES.md +++ b/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 diff --git a/oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/DatabaseSlowStatementBuilder.java b/oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/DatabaseSlowStatementBuilder.java new file mode 100644 index 0000000000..056e9e137f --- /dev/null +++ b/oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/DatabaseSlowStatementBuilder.java @@ -0,0 +1,70 @@ +/* + * 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; + } + +} diff --git a/oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/MultiScopesAnalysisListener.java b/oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/MultiScopesAnalysisListener.java index 2c938d773e..207e53f208 100644 --- a/oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/MultiScopesAnalysisListener.java +++ b/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 entrySourceBuilders = new ArrayList<>(10); private final List exitSourceBuilders = new ArrayList<>(10); - private final List slowDatabaseAccesses = new ArrayList<>(10); + private final List dbSlowStatementBuilders = new ArrayList<>(10); private final List 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()); }); } diff --git a/oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/SourceBuilder.java b/oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/SourceBuilder.java index 21c878ed65..c5e243b424 100644 --- a/oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/SourceBuilder.java +++ b/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 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 */ diff --git a/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/config/NamingControl.java b/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/config/NamingControl.java index 3474b78e77..3a678ebe04 100644 --- a/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/config/NamingControl.java +++ b/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); -- GitLab