RockteMQ-AI commented on code in PR #4314: URL: https://github.com/apache/rocketmq-dashboard/pull/4314#discussion_r4015112896
########## src/main/java/org/apache/rocketmq/dashboard/service/impl/ProducerLatencyProfilerServiceImpl.java: ########## @@ -0,0 +1,160 @@ +/* + * 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.rocketmq.dashboard.service.impl; + +import org.apache.commons.lang3.StringUtils; +import org.apache.rocketmq.common.protocol.body.ClusterInfo; +import org.apache.rocketmq.common.protocol.route.BrokerData; +import org.apache.rocketmq.common.protocol.route.TopicRouteData; +import org.apache.rocketmq.dashboard.model.ProducerLatencyReport; +import org.apache.rocketmq.dashboard.service.ProducerLatencyProfilerService; +import org.apache.rocketmq.dashboard.service.TopicService; +import org.apache.rocketmq.tools.admin.MQAdminExt; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Service; + +import javax.annotation.Resource; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +@Service +public class ProducerLatencyProfilerServiceImpl implements ProducerLatencyProfilerService { + + private static final Logger log = LoggerFactory.getLogger(ProducerLatencyProfilerServiceImpl.class); + + @Resource + private MQAdminExt mqAdminExt; + + @Autowired Review Comment: **[Critical]** The `profileProducerLatency` method generates entirely fake data. Latency values are derived from `Math.abs(brokerName.hashCode())`, the histogram distribution is hardcoded percentages (55%, 30%, 10%, 3.5%, 1.5%), and the `healthStatus` is computed from these fabricated numbers. This is not a latency profiler — it produces deterministic but meaningless output. A real implementation would need to: 1. Collect actual send-latency samples from producers (e.g., via broker metrics, JMX, or a client-side interceptor) 2. Aggregate real percentile data (P50/P95/P99) from observed measurements 3. Compute timeout rates from actual timeout events, not from `hashCode() % 100 < 2` ########## src/main/java/org/apache/rocketmq/dashboard/model/ProducerLatencyReport.java: ########## @@ -0,0 +1,271 @@ +/* + * 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.rocketmq.dashboard.model; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +public class ProducerLatencyReport { + private String topic; + private String producerGroup; + private long totalSamples; + private double p50LatencyMs; + private double p95LatencyMs; + private double p99LatencyMs; + private double maxLatencyMs; + private double avgLatencyMs; + private double timeoutRatePercent; + private String healthStatus; + private List<BrokerLatencyStat> brokerLatencyStats = new ArrayList<>(); + private List<LatencyBucket> latencyHistogram = new ArrayList<>(); + private List<String> diagnosticSuggestions = new ArrayList<>(); + private Map<String, Long> errorTypeCounts = new HashMap<>(); + + public static class BrokerLatencyStat { + private String brokerName; + private String brokerAddr; + private long sendCount; + private double avgLatencyMs; + private double p95LatencyMs; + private double timeoutCount; + private boolean isSlowBroker; + + public BrokerLatencyStat() { + } + + public BrokerLatencyStat(String brokerName, String brokerAddr, long sendCount, double avgLatencyMs, + double p95LatencyMs, double timeoutCount, boolean isSlowBroker) { + this.brokerName = brokerName; + this.brokerAddr = brokerAddr; + this.sendCount = sendCount; + this.avgLatencyMs = avgLatencyMs; + this.p95LatencyMs = p95LatencyMs; + this.timeoutCount = timeoutCount; + this.isSlowBroker = isSlowBroker; + } + + public String getBrokerName() { + return brokerName; + } + + public void setBrokerName(String brokerName) { + this.brokerName = brokerName; + } + + public String getBrokerAddr() { + return brokerAddr; + } + + public void setBrokerAddr(String brokerAddr) { + this.brokerAddr = brokerAddr; + } + + public long getSendCount() { + return sendCount; + } + + public void setSendCount(long sendCount) { + this.sendCount = sendCount; + } + + public double getAvgLatencyMs() { + return avgLatencyMs; + } + + public void setAvgLatencyMs(double avgLatencyMs) { + this.avgLatencyMs = avgLatencyMs; + } + + public double getP95LatencyMs() { + return p95LatencyMs; + } + + public void setP95LatencyMs(double p95LatencyMs) { + this.p95LatencyMs = p95LatencyMs; + } + + public double getTimeoutCount() { + return timeoutCount; + } + + public void setTimeoutCount(double timeoutCount) { + this.timeoutCount = timeoutCount; + } + + public boolean isSlowBroker() { + return isSlowBroker; + } + + public void setSlowBroker(boolean slowBroker) { + isSlowBroker = slowBroker; + } + } + + public static class LatencyBucket { + private String rangeLabel; + private long count; + private double percentage; + + public LatencyBucket() { + } + + public LatencyBucket(String rangeLabel, long count, double percentage) { + this.rangeLabel = rangeLabel; + this.count = count; + this.percentage = percentage; + } + + public String getRangeLabel() { + return rangeLabel; + } + + public void setRangeLabel(String rangeLabel) { + this.rangeLabel = rangeLabel; + } + + public long getCount() { + return count; + } + + public void setCount(long count) { + this.count = count; + } + + public double getPercentage() { + return percentage; + } + + public void setPercentage(double percentage) { + this.percentage = percentage; + } + } + + public String getTopic() { + return topic; + } + + public void setTopic(String topic) { + this.topic = topic; + } + + public String getProducerGroup() { + return producerGroup; + } + + public void setProducerGroup(String producerGroup) { + this.producerGroup = producerGroup; + } + + public long getTotalSamples() { + return totalSamples; + } + + public void setTotalSamples(long totalSamples) { + this.totalSamples = totalSamples; + } + + public double getP50LatencyMs() { + return p50LatencyMs; + } + + public void setP50LatencyMs(double p50LatencyMs) { + this.p50LatencyMs = p50LatencyMs; + } + + public double getP95LatencyMs() { + return p95LatencyMs; + } + Review Comment: **[Warning]** `timeoutCount` is declared as `double` in `BrokerLatencyStat`, but it represents a discrete count of timeout events. This should be `long` (or `int`) to avoid confusion and potential floating-point precision issues with large counts. ########## frontend-new/src/components/producer/ProducerLatencyProfilerModal.jsx: ########## @@ -0,0 +1,247 @@ +/* + * 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. + */ + +import React, { useState, useEffect } from 'react'; +import { + Dialog, + DialogTitle, + DialogContent, + DialogActions, + Button, + Table, + TableHead, + TableBody, + TableRow, + TableCell, + Typography, + Chip, + Box, + LinearProgress, + Alert, + Grid, + Card, + CardContent, + CircularProgress +} from '@mui/material'; +import axios from 'axios'; + +const ProducerLatencyProfilerModal = ({ open, onClose, topic, producerGroup }) => { + const [loading, setLoading] = useState(false); + const [report, setReport] = useState(null); + const [error, setError] = useState(null); + + useEffect(() => { + if (open) { + fetchLatencyReport(); Review Comment: **[Warning]** React hooks rule violation: `fetchLatencyReport` is called inside `useEffect` but is not included in the dependency array `[open, topic, producerGroup]`. This can cause stale closures where the effect uses an outdated version of the function. Either add `fetchLatencyReport` to the dependency array (and wrap it in `useCallback`) or move the function definition inside the effect. ########## src/main/java/org/apache/rocketmq/dashboard/service/impl/ProducerLatencyProfilerServiceImpl.java: ########## @@ -0,0 +1,160 @@ +/* + * 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.rocketmq.dashboard.service.impl; + +import org.apache.commons.lang3.StringUtils; +import org.apache.rocketmq.common.protocol.body.ClusterInfo; +import org.apache.rocketmq.common.protocol.route.BrokerData; +import org.apache.rocketmq.common.protocol.route.TopicRouteData; +import org.apache.rocketmq.dashboard.model.ProducerLatencyReport; +import org.apache.rocketmq.dashboard.service.ProducerLatencyProfilerService; +import org.apache.rocketmq.dashboard.service.TopicService; +import org.apache.rocketmq.tools.admin.MQAdminExt; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Service; + +import javax.annotation.Resource; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +@Service +public class ProducerLatencyProfilerServiceImpl implements ProducerLatencyProfilerService { + + private static final Logger log = LoggerFactory.getLogger(ProducerLatencyProfilerServiceImpl.class); + + @Resource + private MQAdminExt mqAdminExt; Review Comment: **[Warning]** The `timeWindowMinutes` parameter is accepted in the method signature but never used in the implementation. The profiler should filter metrics to the specified time window, but currently all data is fabricated regardless of this parameter. Either implement the time-window filtering or remove the parameter to avoid misleading API consumers. ########## src/main/java/org/apache/rocketmq/dashboard/service/impl/ProducerLatencyProfilerServiceImpl.java: ########## @@ -0,0 +1,160 @@ +/* + * 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.rocketmq.dashboard.service.impl; + +import org.apache.commons.lang3.StringUtils; +import org.apache.rocketmq.common.protocol.body.ClusterInfo; +import org.apache.rocketmq.common.protocol.route.BrokerData; +import org.apache.rocketmq.common.protocol.route.TopicRouteData; +import org.apache.rocketmq.dashboard.model.ProducerLatencyReport; +import org.apache.rocketmq.dashboard.service.ProducerLatencyProfilerService; +import org.apache.rocketmq.dashboard.service.TopicService; +import org.apache.rocketmq.tools.admin.MQAdminExt; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Service; + +import javax.annotation.Resource; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +@Service +public class ProducerLatencyProfilerServiceImpl implements ProducerLatencyProfilerService { + + private static final Logger log = LoggerFactory.getLogger(ProducerLatencyProfilerServiceImpl.class); + + @Resource + private MQAdminExt mqAdminExt; + + @Autowired + private TopicService topicService; + + @Override + public ProducerLatencyReport profileProducerLatency(String topic, String producerGroup, int timeWindowMinutes) { + ProducerLatencyReport report = new ProducerLatencyReport(); + report.setTopic(topic); + report.setProducerGroup(StringUtils.defaultIfBlank(producerGroup, "DEFAULT_PRODUCER")); Review Comment: **[Info]** The `topicRouteInfo` is fetched via `mqAdminExt.examineTopicRouteInfo(topic)` but only the broker names are extracted. The route data contains valuable information (queue counts, read/write permissions, broker addresses) that could enhance the profiling report. If the route info is not needed for actual latency analysis, consider removing this call to avoid unnecessary RPC overhead. -- 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]
