Copilot commented on code in PR #7882:
URL: https://github.com/apache/incubator-seata/pull/7882#discussion_r2642577052
##########
namingserver/src/main/java/org/apache/seata/namingserver/manager/ClusterWatcherManager.java:
##########
@@ -113,6 +128,9 @@ public void registryWatcher(Watcher<?> watcher) {
if (term == null || watcher.getTerm() >= term) {
WATCHERS.computeIfAbsent(group, value -> new
ConcurrentLinkedQueue<>())
.add(watcher);
+
+ // Immediately refresh watcher count metrics
+ metricsManager.refreshWatcherCountMetrics();
Review Comment:
The metrics are being refreshed on every watcher registration, which could
happen very frequently in a high-traffic scenario. Calling
refreshWatcherCountMetrics on every single registration could create
unnecessary overhead. Consider batching these updates or using a scheduled
refresh instead of immediate refresh on every operation.
##########
namingserver/src/main/java/org/apache/seata/namingserver/metrics/NamingServerTagsContributor.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.seata.namingserver.metrics;
+
+import io.micrometer.common.KeyValue;
+import io.micrometer.common.KeyValues;
+import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
+import
org.springframework.http.server.observation.DefaultServerRequestObservationConvention;
+import
org.springframework.http.server.observation.ServerRequestObservationContext;
+import org.springframework.stereotype.Component;
+
+/**
+ * Custom tags contributor for HTTP server request metrics.
+ * Adds Seata business dimensions (namespace, cluster, vgroup) to
+ * http.server.requests metrics.
+ */
+@Component
+@ConditionalOnProperty(name = "seata.namingserver.metrics.enabled",
havingValue = "true")
+public class NamingServerTagsContributor extends
DefaultServerRequestObservationConvention {
+
+ private static final String TAG_NAMESPACE = "namespace";
+ private static final String TAG_CLUSTER = "cluster";
+ private static final String TAG_VGROUP = "vgroup";
+ private static final String UNKNOWN = "unknown";
+
+ @Override
+ public KeyValues
getLowCardinalityKeyValues(ServerRequestObservationContext context) {
+ KeyValues keyValues = super.getLowCardinalityKeyValues(context);
+
+ // Add namespace tag
+ String namespace = context.getCarrier().getParameter(TAG_NAMESPACE);
+ keyValues = keyValues.and(KeyValue.of(TAG_NAMESPACE, namespace != null
? namespace : UNKNOWN));
+
+ // Add cluster tag
+ String cluster = context.getCarrier().getParameter(TAG_CLUSTER);
+ if (cluster == null) {
+ cluster = context.getCarrier().getParameter("clusterName");
+ }
+ keyValues = keyValues.and(KeyValue.of(TAG_CLUSTER, cluster != null ?
cluster : UNKNOWN));
+
+ // Add vgroup tag
+ String vgroup = context.getCarrier().getParameter(TAG_VGROUP);
+ if (vgroup == null) {
+ vgroup = context.getCarrier().getParameter("group");
+ }
+ keyValues = keyValues.and(KeyValue.of(TAG_VGROUP, vgroup != null ?
vgroup : UNKNOWN));
Review Comment:
Adding tags with value "unknown" for every HTTP request will create high
cardinality metrics. When these parameters are not present in requests, it
would be better to either not add the tag at all, or use a constant shared
"unknown" value. This could lead to performance issues and excessive memory
usage in the metrics registry as each unique combination of tag values creates
a separate time series. Consider only adding tags when they have meaningful
values.
##########
namingserver/src/main/java/org/apache/seata/namingserver/manager/NamingManager.java:
##########
@@ -346,6 +355,9 @@ public boolean registerInstance(NamingServerNode node,
String namespace, String
node.getTransaction().getHost(),
node.getTransaction().getPort()),
System.currentTimeMillis());
+
+ // Immediately refresh cluster node count metrics
+ metricsManager.refreshClusterNodeCountMetrics();
Review Comment:
The metrics are being refreshed on every instance registration and
unregistration. In high-traffic scenarios with frequent
registrations/unregistrations, this could create significant overhead. Consider
batching these updates or using a scheduled refresh strategy instead of
immediately refreshing on every operation.
##########
namingserver/src/main/java/org/apache/seata/namingserver/manager/NamingManager.java:
##########
@@ -378,6 +390,9 @@ public boolean unregisterInstance(String namespace, String
clusterName, String u
node.getTransaction().getPort()));
}
}
+
+ // Immediately refresh cluster node count metrics
+ metricsManager.refreshClusterNodeCountMetrics();
Review Comment:
The metrics are being refreshed on every instance registration and
unregistration. In high-traffic scenarios with frequent
registrations/unregistrations, this could create significant overhead. Consider
batching these updates or using a scheduled refresh strategy instead of
immediately refreshing on every operation.
##########
namingserver/src/main/resources/application.yml:
##########
@@ -33,9 +33,25 @@ heartbeat:
threshold: 90000
period: 60000
seata:
+ namingserver:
+ metrics:
+ enabled: true
security:
secretKey: SeataSecretKey0c382ef121d778043159209298fd40bf3850a017
tokenValidityInMilliseconds: 1800000
csrf-ignore-urls: /naming/v1/**,/api/v1/naming/**
ignore:
urls:
/,/**/*.css,/**/*.js,/**/*.html,/**/*.map,/**/*.svg,/**/*.png,/**/*.jpeg,/**/*.ico,/api/v1/auth/login,/version.json,/naming/v1/health,/error
+management:
+ endpoints:
+ web:
+ exposure:
+ include: "prometheus,health,info,metrics"
+ metrics:
+ tags:
+ application: { spring.application.name }
Review Comment:
The syntax used here is incorrect for YAML. The correct syntax to reference
another property value in Spring Boot YAML should use `${}` without curly
braces around the reference, like this: `application:
${spring.application.name}`
```suggestion
application: ${spring.application.name}
```
##########
namingserver/src/test/resources/application.yml:
##########
@@ -29,8 +29,24 @@ heartbeat:
threshold: 5000
period: 5000
seata:
+ namingserver:
+ metrics:
+ enabled: true
security:
secretKey: SeataSecretKey0c382ef121d778043159209298fd40bf3850a017
tokenValidityInMilliseconds: 1800000
ignore:
urls:
/,/**/*.css,/**/*.js,/**/*.html,/**/*.map,/**/*.svg,/**/*.png,/**/*.jpeg,/**/*.ico,/api/v1/auth/login,/version.json,/health,/error,/naming/v1/**
+management:
+ endpoints:
+ web:
+ exposure:
+ include: "prometheus,health,info,metrics"
+ metrics:
+ tags:
+ application: { spring.application.name }
Review Comment:
The syntax used here is incorrect for YAML. The correct syntax to reference
another property value in Spring Boot YAML should use `${}` without curly
braces around the reference, like this: `application:
${spring.application.name}`
```suggestion
application: ${spring.application.name}
```
##########
namingserver/src/main/java/org/apache/seata/namingserver/manager/NamingManager.java:
##########
@@ -77,21 +78,23 @@ public class NamingManager {
private static final Logger LOGGER =
LoggerFactory.getLogger(NamingManager.class);
private final ConcurrentMap<InetSocketAddress, Long> instanceLiveTable;
private volatile LoadingCache<String /* VGroup */, ConcurrentMap<String /*
namespace */, NamespaceBO>> vGroupMap;
- private final ConcurrentMap<String /* namespace */, ConcurrentMap<String
/* clusterName */, ClusterData>>
- namespaceClusterDataMap;
+ private final ConcurrentMap<String /* namespace */, ConcurrentMap<String
/* clusterName */, ClusterData>> namespaceClusterDataMap;
@Value("${heartbeat.threshold:90000}")
private int heartbeatTimeThreshold;
@Value("${heartbeat.period:60000}")
private int heartbeatCheckTimePeriod;
- protected final ScheduledExecutorService heartBeatCheckService =
- new ScheduledThreadPoolExecutor(1, new
CustomizableThreadFactory("heartBeatCheckExcuter"));
+ protected final ScheduledExecutorService heartBeatCheckService = new
ScheduledThreadPoolExecutor(1,
+ new CustomizableThreadFactory("heartBeatCheckExcuter"));
Review Comment:
The thread factory name contains a typo: "heartBeatCheckExcuter" should be
spelled "heartBeatCheckExecutor".
```suggestion
new CustomizableThreadFactory("heartBeatCheckExecutor"));
```
##########
namingserver/src/main/java/org/apache/seata/namingserver/metrics/NamingServerTagsContributor.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.seata.namingserver.metrics;
+
+import io.micrometer.common.KeyValue;
+import io.micrometer.common.KeyValues;
+import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
+import
org.springframework.http.server.observation.DefaultServerRequestObservationConvention;
+import
org.springframework.http.server.observation.ServerRequestObservationContext;
+import org.springframework.stereotype.Component;
+
+/**
+ * Custom tags contributor for HTTP server request metrics.
+ * Adds Seata business dimensions (namespace, cluster, vgroup) to
+ * http.server.requests metrics.
+ */
+@Component
+@ConditionalOnProperty(name = "seata.namingserver.metrics.enabled",
havingValue = "true")
+public class NamingServerTagsContributor extends
DefaultServerRequestObservationConvention {
+
+ private static final String TAG_NAMESPACE = "namespace";
+ private static final String TAG_CLUSTER = "cluster";
+ private static final String TAG_VGROUP = "vgroup";
+ private static final String UNKNOWN = "unknown";
+
+ @Override
+ public KeyValues
getLowCardinalityKeyValues(ServerRequestObservationContext context) {
+ KeyValues keyValues = super.getLowCardinalityKeyValues(context);
+
+ // Add namespace tag
+ String namespace = context.getCarrier().getParameter(TAG_NAMESPACE);
+ keyValues = keyValues.and(KeyValue.of(TAG_NAMESPACE, namespace != null
? namespace : UNKNOWN));
+
+ // Add cluster tag
+ String cluster = context.getCarrier().getParameter(TAG_CLUSTER);
+ if (cluster == null) {
+ cluster = context.getCarrier().getParameter("clusterName");
+ }
+ keyValues = keyValues.and(KeyValue.of(TAG_CLUSTER, cluster != null ?
cluster : UNKNOWN));
+
+ // Add vgroup tag
+ String vgroup = context.getCarrier().getParameter(TAG_VGROUP);
+ if (vgroup == null) {
+ vgroup = context.getCarrier().getParameter("group");
+ }
+ keyValues = keyValues.and(KeyValue.of(TAG_VGROUP, vgroup != null ?
vgroup : UNKNOWN));
+
+ return keyValues;
+ }
+}
Review Comment:
The NamingServerTagsContributor class lacks test coverage. This component
extends DefaultServerRequestObservationConvention and adds custom tag logic
that should be tested to ensure tags are correctly extracted from request
parameters and handled when missing.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
