You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/04/12 17:25:40 UTC
[GitHub] [pulsar] tjiuming opened a new pull request, #15140: [transaction][monitor] Add metrics for transaction
tjiuming opened a new pull request, #15140:
URL: https://github.com/apache/pulsar/pull/15140
### Motivation
Add metrics for transaction
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r902511460
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/pendingack/impl/PendingAckHandleStatsImpl.java:
##########
@@ -0,0 +1,95 @@
+/**
+ * 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.pulsar.broker.transaction.pendingack.impl;
+
+import io.prometheus.client.Counter;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pulsar.broker.transaction.pendingack.PendingAckHandleStats;
+import org.apache.pulsar.common.naming.TopicName;
+
+public class PendingAckHandleStatsImpl implements PendingAckHandleStats {
+ private static final AtomicBoolean INITIALIZED = new AtomicBoolean(false);
+ private static Counter commitTxnCounter;
+ private static Counter abortTxnCounter;
+ private static boolean exposeTopicLevelMetrics0;
+
+ private final String[] labelSucceed;
+ private final String[] labelFailed;
+
+ public PendingAckHandleStatsImpl(String topic, String subscription, boolean exposeTopicLevelMetrics) {
+ initialize(exposeTopicLevelMetrics);
+
+ String namespace;
+ if (StringUtils.isBlank(topic)) {
+ namespace = topic = "unknown";
Review Comment:
right, it's not possible that topic is blank, it just in case of unexpected behaviors.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TransactionBufferClientStatsImpl.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pulsar.broker.transaction.buffer.impl;
+
+import io.prometheus.client.CollectorRegistry;
+import io.prometheus.client.Counter;
+import io.prometheus.client.Gauge;
+import io.prometheus.client.Summary;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.pulsar.broker.transaction.buffer.TransactionBufferClientStats;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+import org.apache.pulsar.common.naming.TopicName;
+
+public final class TransactionBufferClientStatsImpl implements TransactionBufferClientStats {
+ private static final double[] QUANTILES = {0.50, 0.75, 0.95, 0.99, 0.999, 0.9999, 1};
+ private final AtomicBoolean closed = new AtomicBoolean(false);
+
+ private final Counter abortFailed;
+ private final Counter commitFailed;
+ private final Summary abortLatency;
+ private final Summary commitLatency;
+ private final Gauge pendingRequests;
+
+ private final boolean exposeTopicLevelMetrics;
+
+ private static TransactionBufferClientStats instance;
+
+ private TransactionBufferClientStatsImpl(boolean exposeTopicLevelMetrics,
+ TransactionBufferHandler handler) {
+ this.exposeTopicLevelMetrics = exposeTopicLevelMetrics;
+ String[] labelNames = exposeTopicLevelMetrics
+ ? new String[]{"namespace", "topic"} : new String[]{"namespace"};
+
+ this.abortFailed = Counter.build("pulsar_txn_tb_client_abort_failed", "-")
+ .labelNames(labelNames)
+ .register();
Review Comment:
Yes.
When run 2 brokers inside a same JVM, it just bind 2 different ports, other behaviors makes no differences.
Yes, many other metrics using the default registry.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TransactionBufferClientImpl.java:
##########
@@ -35,47 +37,88 @@
public class TransactionBufferClientImpl implements TransactionBufferClient {
private final TransactionBufferHandler tbHandler;
+ private final TransactionBufferClientStats stats;
- private TransactionBufferClientImpl(TransactionBufferHandler tbHandler) {
+ private TransactionBufferClientImpl(TransactionBufferHandler tbHandler, boolean exposeTopicLevelMetrics,
+ boolean enableTxnCoordinator) {
this.tbHandler = tbHandler;
+ this.stats = TransactionBufferClientStats.create(exposeTopicLevelMetrics, tbHandler, enableTxnCoordinator);
}
public static TransactionBufferClient create(PulsarService pulsarService, HashedWheelTimer timer,
int maxConcurrentRequests, long operationTimeoutInMills) throws PulsarServerException {
TransactionBufferHandler handler = new TransactionBufferHandlerImpl(pulsarService, timer,
maxConcurrentRequests, operationTimeoutInMills);
- return new TransactionBufferClientImpl(handler);
+
+ ServiceConfiguration config = pulsarService.getConfig();
+ boolean exposeTopicLevelMetrics = config.isExposeTopicLevelMetricsInPrometheus();
+ boolean enableTxnCoordinator = config.isTransactionCoordinatorEnabled();
+ return new TransactionBufferClientImpl(handler, exposeTopicLevelMetrics, enableTxnCoordinator);
}
@Override
public CompletableFuture<TxnID> commitTxnOnTopic(String topic, long txnIdMostBits,
long txnIdLeastBits, long lowWaterMark) {
- return tbHandler.endTxnOnTopic(topic, txnIdMostBits, txnIdLeastBits, TxnAction.COMMIT, lowWaterMark);
+ long start = System.currentTimeMillis();
Review Comment:
it makes sense, I‘ll change it.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a diff in pull request #15140: [transaction][monitor] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r849060142
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/prometheus/TopicStats.java:
##########
@@ -42,6 +42,9 @@ class TopicStats {
long bytesOutCounter;
double averageMsgSize;
+ long ongoingTxnCount;
Review Comment:
because topic transaction stats need transaction buffer stats and pending ack stats, so I think it only represent tb ongoingTxnCount
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/prometheus/TopicStats.java:
##########
@@ -127,6 +133,11 @@ static void printTopicStats(SimpleTextOutputStream stream, String cluster, Strin
metric(stream, cluster, namespace, topic, "pulsar_average_msg_size", stats.averageMsgSize,
splitTopicAndPartitionIndexLabel);
+ metric(stream, cluster, namespace, topic, "pulsar_onging_transaction_count", stats.ongoingTxnCount,
Review Comment:
pulsar_txn_ tb_active_count
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/prometheus/TopicStats.java:
##########
@@ -127,6 +133,11 @@ static void printTopicStats(SimpleTextOutputStream stream, String cluster, Strin
metric(stream, cluster, namespace, topic, "pulsar_average_msg_size", stats.averageMsgSize,
splitTopicAndPartitionIndexLabel);
+ metric(stream, cluster, namespace, topic, "pulsar_onging_transaction_count", stats.ongoingTxnCount,
+ splitTopicAndPartitionIndexLabel);
+ metric(stream, cluster, namespace, topic, "pulsar_abort_transaction_count", stats.abortTxnCount,
Review Comment:
should add pulsar_txn_tb_committed_count
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/prometheus/TopicStats.java:
##########
@@ -127,6 +133,11 @@ static void printTopicStats(SimpleTextOutputStream stream, String cluster, Strin
metric(stream, cluster, namespace, topic, "pulsar_average_msg_size", stats.averageMsgSize,
splitTopicAndPartitionIndexLabel);
+ metric(stream, cluster, namespace, topic, "pulsar_onging_transaction_count", stats.ongoingTxnCount,
+ splitTopicAndPartitionIndexLabel);
+ metric(stream, cluster, namespace, topic, "pulsar_abort_transaction_count", stats.abortTxnCount,
Review Comment:
pulsar_txn_ tb_aborted_count
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r903222836
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientStats.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.pulsar.broker.transaction.buffer;
+
+import org.apache.pulsar.broker.transaction.buffer.impl.TransactionBufferClientStatsImpl;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+
+public interface TransactionBufferClientStats {
+
+ void recordAbortFailed(String topic);
Review Comment:
I see [TransactionBuffer.java](https://github.com/apache/pulsar/pull/15140/files#diff-819cb066b87a312a85c161d7bccf5699ddd8e1aded6545a6f39a237d16e6e707) and [PendingAckHandleStatsImpl](https://github.com/apache/pulsar/pull/15140/files#diff-3e4c8d1822aa122c6b58f1b2c8227a6dac1354cbeeb1f68cab24a6587a881ea5)
The implementation is different; what is the consideration here? I missed a part of the code before, sorry
```
public void recordCommitTxn(boolean success) {
commitTxnCounter.labels(success ? labelSucceed : labelFailed).inc();
}
```
```
private final Counter abortFailed;
private final Counter commitFailed;
private final Summary abortLatency;
private final Summary commitLatency;
```
I think in TransactionBuffer the latency is managedLedger append entry latency, it also already exists. So we only need to add the count and the rate.the same is true for PendingAckStats
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r881453319
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientTest.java:
##########
@@ -148,6 +154,59 @@ public void testAbortOnSubscription() throws ExecutionException, InterruptedExce
}
}
+
+ @Test
+ public void testTransactionBufferMetrics() throws Exception {
+ //Test commit
+ for (int i = 0; i < partitions; i++) {
+ String topic = partitionedTopicName.getPartition(i).toString();
+ tbClient.commitTxnOnSubscription(topic, "test", 1L, i, -1L).get();
+ }
+
+ //test abort
+ for (int i = 0; i < partitions; i++) {
+ String topic = partitionedTopicName.getPartition(i).toString();
+ tbClient.abortTxnOnSubscription(topic, "test", 1L, i, -1L).get();
+ }
+
+ @Cleanup
+ ByteArrayOutputStream statsOut = new ByteArrayOutputStream();
+ PrometheusMetricsGenerator.generate(pulsarServiceList.get(0), true, false, false, statsOut);
+ String metricsStr = statsOut.toString();
+ Multimap<String, PrometheusMetricsTest.Metric> metricsMap = PrometheusMetricsTest.parseMetrics(metricsStr);
+
+ Collection<PrometheusMetricsTest.Metric> abortFailed = metricsMap.get("pulsar_txn_tb_client_abort_failed");
+ Collection<PrometheusMetricsTest.Metric> commitFailed = metricsMap.get("pulsar_txn_tb_client_commit_failed");
+ Collection<PrometheusMetricsTest.Metric> abortLatencyCount =
+ metricsMap.get("pulsar_txn_tb_client_abort_latency_count");
+ Collection<PrometheusMetricsTest.Metric> commitLatencyCount =
+ metricsMap.get("pulsar_txn_tb_client_commit_latency_count");
+ Collection<PrometheusMetricsTest.Metric> pending = metricsMap.get("pulsar_txn_tb_client_pending_requests");
+
+ assertEquals(abortFailed.stream().mapToDouble(metric -> metric.value).sum(), 0);
+ assertEquals(commitFailed.stream().mapToDouble(metric -> metric.value).sum(), 0);
+
+ for (int i = 0; i < partitions; i++) {
+ String topic = partitionedTopicName.getPartition(i).toString();
+ Optional<PrometheusMetricsTest.Metric> optional = abortLatencyCount.stream()
+ .filter(metric -> metric.tags.get("topic").equals(topic)).findFirst();
+
+ assertTrue(optional.isPresent());
+ assertEquals(optional.get().value, 1D);
+
+ Optional<PrometheusMetricsTest.Metric> optional1 = commitLatencyCount.stream()
+ .filter(metric -> metric.tags.get("topic").equals(topic)).findFirst();
+ assertTrue(optional1.isPresent());
+ assertEquals(optional1.get().value, 1D);
+ }
+
+ assertEquals(pending.size(), 1);
+
+ for (PrometheusMetricsTest.Metric metric : pending) {
+ assertTrue(metric.value >= 0D);
Review Comment:
should add the request to pending and check the value > 0D is better
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r881453319
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientTest.java:
##########
@@ -148,6 +154,59 @@ public void testAbortOnSubscription() throws ExecutionException, InterruptedExce
}
}
+
+ @Test
+ public void testTransactionBufferMetrics() throws Exception {
+ //Test commit
+ for (int i = 0; i < partitions; i++) {
+ String topic = partitionedTopicName.getPartition(i).toString();
+ tbClient.commitTxnOnSubscription(topic, "test", 1L, i, -1L).get();
+ }
+
+ //test abort
+ for (int i = 0; i < partitions; i++) {
+ String topic = partitionedTopicName.getPartition(i).toString();
+ tbClient.abortTxnOnSubscription(topic, "test", 1L, i, -1L).get();
+ }
+
+ @Cleanup
+ ByteArrayOutputStream statsOut = new ByteArrayOutputStream();
+ PrometheusMetricsGenerator.generate(pulsarServiceList.get(0), true, false, false, statsOut);
+ String metricsStr = statsOut.toString();
+ Multimap<String, PrometheusMetricsTest.Metric> metricsMap = PrometheusMetricsTest.parseMetrics(metricsStr);
+
+ Collection<PrometheusMetricsTest.Metric> abortFailed = metricsMap.get("pulsar_txn_tb_client_abort_failed");
+ Collection<PrometheusMetricsTest.Metric> commitFailed = metricsMap.get("pulsar_txn_tb_client_commit_failed");
+ Collection<PrometheusMetricsTest.Metric> abortLatencyCount =
+ metricsMap.get("pulsar_txn_tb_client_abort_latency_count");
+ Collection<PrometheusMetricsTest.Metric> commitLatencyCount =
+ metricsMap.get("pulsar_txn_tb_client_commit_latency_count");
+ Collection<PrometheusMetricsTest.Metric> pending = metricsMap.get("pulsar_txn_tb_client_pending_requests");
+
+ assertEquals(abortFailed.stream().mapToDouble(metric -> metric.value).sum(), 0);
+ assertEquals(commitFailed.stream().mapToDouble(metric -> metric.value).sum(), 0);
+
+ for (int i = 0; i < partitions; i++) {
+ String topic = partitionedTopicName.getPartition(i).toString();
+ Optional<PrometheusMetricsTest.Metric> optional = abortLatencyCount.stream()
+ .filter(metric -> metric.tags.get("topic").equals(topic)).findFirst();
+
+ assertTrue(optional.isPresent());
+ assertEquals(optional.get().value, 1D);
+
+ Optional<PrometheusMetricsTest.Metric> optional1 = commitLatencyCount.stream()
+ .filter(metric -> metric.tags.get("topic").equals(topic)).findFirst();
+ assertTrue(optional1.isPresent());
+ assertEquals(optional1.get().value, 1D);
+ }
+
+ assertEquals(pending.size(), 1);
+
+ for (PrometheusMetricsTest.Metric metric : pending) {
+ assertTrue(metric.value >= 0D);
Review Comment:
should add the request to pending and check the value > 0D is better
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaoran10 commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r895971996
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientStats.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.pulsar.broker.transaction.buffer;
+
+import org.apache.pulsar.broker.transaction.buffer.impl.TransactionBufferClientStatsImpl;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+
+public interface TransactionBufferClientStats {
+
+ void recordAbortFailed(String topic);
Review Comment:
@congbobo184 @codelipenghui Do we need to distinguish between commit(abort) TB or commit(abort) PendingAck? Same as operation latency.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1105074216
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r902388392
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientStats.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.pulsar.broker.transaction.buffer;
+
+import org.apache.pulsar.broker.transaction.buffer.impl.TransactionBufferClientStatsImpl;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+
+public interface TransactionBufferClientStats {
+
+ void recordAbortFailed(String topic);
Review Comment:
@congbobo184 @congbobo184 Could you please describe it more clearly? I mean, there were already distinguished between TB and TP, please see: [TransactionBuffer.java](https://github.com/apache/pulsar/pull/15140/files#diff-819cb066b87a312a85c161d7bccf5699ddd8e1aded6545a6f39a237d16e6e707) and [PendingAckHandleStatsImpl](https://github.com/apache/pulsar/pull/15140/files#diff-3e4c8d1822aa122c6b58f1b2c8227a6dac1354cbeeb1f68cab24a6587a881ea5)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nicoloboschi commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r902569967
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientStats.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.pulsar.broker.transaction.buffer;
+
+import org.apache.pulsar.broker.transaction.buffer.impl.TransactionBufferClientStatsImpl;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+
+public interface TransactionBufferClientStats {
+
+ void recordAbortFailed(String topic);
+
+ void recordCommitFailed(String topic);
+
+ void recordAbortLatency(String topic, long cost);
+
+ void recordCommitLatency(String topic, long cost);
+
+ void close();
Review Comment:
we can use `AutoCloseable`
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -245,6 +248,20 @@ public CompletableFuture<Void> checkIfTBRecoverCompletely(boolean isTxnEnabled)
}
}
+ @Override
+ public long getOngoingTxnCount() {
Review Comment:
IIUC `ongoingTxns` and `aborts` are real gauges because they can be increased or decreased.
`txnCommittedCounter` is only incremented and so it's a counter.
Is it correct ?
If so, I believe we have to introduce a txnAbortedCounter
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/pendingack/PendingAckHandleStats.java:
##########
@@ -0,0 +1,34 @@
+/**
+ * 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.pulsar.broker.transaction.pendingack;
+
+import org.apache.pulsar.broker.transaction.pendingack.impl.PendingAckHandleStatsImpl;
+
+public interface PendingAckHandleStats {
+
+ void recordCommitTxn(boolean success);
+
+ void recordAbortTxn(boolean success);
+
+ void close();
Review Comment:
Autocloseable?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TransactionBufferClientStatsImpl.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pulsar.broker.transaction.buffer.impl;
+
+import io.prometheus.client.CollectorRegistry;
+import io.prometheus.client.Counter;
+import io.prometheus.client.Gauge;
+import io.prometheus.client.Summary;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.pulsar.broker.transaction.buffer.TransactionBufferClientStats;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+import org.apache.pulsar.common.naming.TopicName;
+
+public final class TransactionBufferClientStatsImpl implements TransactionBufferClientStats {
+ private static final double[] QUANTILES = {0.50, 0.75, 0.95, 0.99, 0.999, 0.9999, 1};
+ private final AtomicBoolean closed = new AtomicBoolean(false);
+
+ private final Counter abortFailed;
+ private final Counter commitFailed;
+ private final Summary abortLatency;
+ private final Summary commitLatency;
+ private final Gauge pendingRequests;
+
+ private final boolean exposeTopicLevelMetrics;
+
+ private static TransactionBufferClientStats instance;
+
+ private TransactionBufferClientStatsImpl(boolean exposeTopicLevelMetrics,
+ TransactionBufferHandler handler) {
+ this.exposeTopicLevelMetrics = exposeTopicLevelMetrics;
+ String[] labelNames = exposeTopicLevelMetrics
+ ? new String[]{"namespace", "topic"} : new String[]{"namespace"};
+
+ this.abortFailed = Counter.build("pulsar_txn_tb_client_abort_failed", "-")
+ .labelNames(labelNames)
+ .register();
+ this.commitFailed = Counter.build("pulsar_txn_tb_client_commit_failed", "-")
+ .labelNames(labelNames)
+ .register();
+ this.abortLatency =
+ this.buildSummary("pulsar_txn_tb_client_abort_latency", "-", labelNames);
+ this.commitLatency =
+ this.buildSummary("pulsar_txn_tb_client_commit_latency", "-", labelNames);
+
+ this.pendingRequests = Gauge.build("pulsar_txn_tb_client_pending_requests", "-")
+ .register()
+ .setChild(new Gauge.Child() {
+ @Override
+ public double get() {
+ return null == handler ? 0 : handler.getPendingRequestsCount();
+ }
+ });
+ }
+
+ private Summary buildSummary(String name, String help, String[] labelNames) {
+ Summary.Builder builder = Summary.build(name, help)
+ .labelNames(labelNames);
+ for (double quantile : QUANTILES) {
+ builder.quantile(quantile, 0.01D);
+ }
+ return builder.register();
+ }
+
+ public static synchronized TransactionBufferClientStats getInstance(boolean exposeTopicLevelMetrics,
+ TransactionBufferHandler handler) {
+ if (null == instance) {
+ instance = new TransactionBufferClientStatsImpl(exposeTopicLevelMetrics, handler);
+ }
+
+ return instance;
+ }
+
+ @Override
+ public void recordAbortFailed(String topic) {
+ this.abortFailed.labels(labelValues(topic)).inc();
+ }
+
+ @Override
+ public void recordCommitFailed(String topic) {
+ this.commitFailed.labels(labelValues(topic)).inc();
+ }
+
+ @Override
+ public void recordAbortLatency(String topic, long cost) {
+ this.abortLatency.labels(labelValues(topic)).observe(cost);
+ }
+
+ @Override
+ public void recordCommitLatency(String topic, long cost) {
+ this.commitLatency.labels(labelValues(topic)).observe(cost);
+ }
+
+ private String[] labelValues(String topic) {
+ try {
+ TopicName topicName = TopicName.get(topic);
+ return exposeTopicLevelMetrics
+ ? new String[]{topicName.getNamespace(), topic} : new String[]{topicName.getNamespace()};
+ } catch (Throwable t) {
Review Comment:
log the exception ?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TransactionBufferClientStatsImpl.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pulsar.broker.transaction.buffer.impl;
+
+import io.prometheus.client.CollectorRegistry;
+import io.prometheus.client.Counter;
+import io.prometheus.client.Gauge;
+import io.prometheus.client.Summary;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.pulsar.broker.transaction.buffer.TransactionBufferClientStats;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+import org.apache.pulsar.common.naming.TopicName;
+
+public final class TransactionBufferClientStatsImpl implements TransactionBufferClientStats {
+ private static final double[] QUANTILES = {0.50, 0.75, 0.95, 0.99, 0.999, 0.9999, 1};
+ private final AtomicBoolean closed = new AtomicBoolean(false);
+
+ private final Counter abortFailed;
+ private final Counter commitFailed;
+ private final Summary abortLatency;
+ private final Summary commitLatency;
+ private final Gauge pendingRequests;
+
+ private final boolean exposeTopicLevelMetrics;
+
+ private static TransactionBufferClientStats instance;
+
+ private TransactionBufferClientStatsImpl(boolean exposeTopicLevelMetrics,
+ TransactionBufferHandler handler) {
+ this.exposeTopicLevelMetrics = exposeTopicLevelMetrics;
+ String[] labelNames = exposeTopicLevelMetrics
+ ? new String[]{"namespace", "topic"} : new String[]{"namespace"};
+
+ this.abortFailed = Counter.build("pulsar_txn_tb_client_abort_failed", "-")
+ .labelNames(labelNames)
+ .register();
+ this.commitFailed = Counter.build("pulsar_txn_tb_client_commit_failed", "-")
+ .labelNames(labelNames)
+ .register();
+ this.abortLatency =
+ this.buildSummary("pulsar_txn_tb_client_abort_latency", "-", labelNames);
+ this.commitLatency =
+ this.buildSummary("pulsar_txn_tb_client_commit_latency", "-", labelNames);
+
+ this.pendingRequests = Gauge.build("pulsar_txn_tb_client_pending_requests", "-")
+ .register()
+ .setChild(new Gauge.Child() {
+ @Override
+ public double get() {
+ return null == handler ? 0 : handler.getPendingRequestsCount();
+ }
+ });
+ }
+
+ private Summary buildSummary(String name, String help, String[] labelNames) {
+ Summary.Builder builder = Summary.build(name, help)
+ .labelNames(labelNames);
+ for (double quantile : QUANTILES) {
+ builder.quantile(quantile, 0.01D);
+ }
+ return builder.register();
+ }
+
+ public static synchronized TransactionBufferClientStats getInstance(boolean exposeTopicLevelMetrics,
+ TransactionBufferHandler handler) {
+ if (null == instance) {
+ instance = new TransactionBufferClientStatsImpl(exposeTopicLevelMetrics, handler);
+ }
+
+ return instance;
+ }
+
+ @Override
+ public void recordAbortFailed(String topic) {
+ this.abortFailed.labels(labelValues(topic)).inc();
+ }
+
+ @Override
+ public void recordCommitFailed(String topic) {
+ this.commitFailed.labels(labelValues(topic)).inc();
+ }
+
+ @Override
+ public void recordAbortLatency(String topic, long cost) {
+ this.abortLatency.labels(labelValues(topic)).observe(cost);
+ }
+
+ @Override
+ public void recordCommitLatency(String topic, long cost) {
+ this.commitLatency.labels(labelValues(topic)).observe(cost);
+ }
+
+ private String[] labelValues(String topic) {
+ try {
+ TopicName topicName = TopicName.get(topic);
+ return exposeTopicLevelMetrics
+ ? new String[]{topicName.getNamespace(), topic} : new String[]{topicName.getNamespace()};
+ } catch (Throwable t) {
+ return exposeTopicLevelMetrics ? new String[]{"unknown", "unknown"} : new String[]{"unknown"};
+ }
+ }
+
+ @Override
+ public void close() {
+ if (instance == this && this.closed.compareAndSet(false, true)) {
Review Comment:
why `instance == this` is needed ? if I call close() on this instance I expect to call this object even if it's not the singleton
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaoran10 commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r895971996
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientStats.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.pulsar.broker.transaction.buffer;
+
+import org.apache.pulsar.broker.transaction.buffer.impl.TransactionBufferClientStatsImpl;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+
+public interface TransactionBufferClientStats {
+
+ void recordAbortFailed(String topic);
Review Comment:
@congbobo184 @codelipenghui Do we need to distinguish between commit(abort) TB or commit(abort) PendingAck? Maybe it's useful to find problems.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1102142973
@codelipenghui @gaoran10 Could you please help review?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaoran10 commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r895971996
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientStats.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.pulsar.broker.transaction.buffer;
+
+import org.apache.pulsar.broker.transaction.buffer.impl.TransactionBufferClientStatsImpl;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+
+public interface TransactionBufferClientStats {
+
+ void recordAbortFailed(String topic);
Review Comment:
@congbobo184 @codelipenghui Do we need to distinguish between commit(abort) TB or commit(abort) PendingAck? Same as operation latency, maybe it's useful to find problems.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaoran10 commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r895971996
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientStats.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.pulsar.broker.transaction.buffer;
+
+import org.apache.pulsar.broker.transaction.buffer.impl.TransactionBufferClientStatsImpl;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+
+public interface TransactionBufferClientStats {
+
+ void recordAbortFailed(String topic);
Review Comment:
@congbobo184 @codelipenghui Do we need to distinguish between commit(abort) TB or commit(abort) PendingAck (Same as operation latency)? Maybe it's useful to find problems.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] momo-jun commented on pull request #15140: [transaction][monitor] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
momo-jun commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1099901368
Hi @tjiuming will you submit a doc PR as soon as this PR is approved and merged? Ping me if you need any review or other assistance.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on pull request #15140: [transaction][monitor] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1099972764
yes PendingAck is TP
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r903237141
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientStats.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.pulsar.broker.transaction.buffer;
+
+import org.apache.pulsar.broker.transaction.buffer.impl.TransactionBufferClientStatsImpl;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+
+public interface TransactionBufferClientStats {
+
+ void recordAbortFailed(String topic);
Review Comment:
For TransactionBuffer, I recorded TransactionBufferClient's latency, because there is RPC request existing.
> The implementation is different; what is the consideration here
In TransactionBufferClientStats, abort/commit latency only recorded when abort/commit success, so we can get `abortSuccess` and `commitSuccess` from `abortLatency` `commitLatency `.
@congbobo184
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1173344881
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1172905196
@codelipenghui @congbobo184 This PR has been blocked for a long while, please help review and merge ASAP
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r902097678
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientStats.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.pulsar.broker.transaction.buffer;
+
+import org.apache.pulsar.broker.transaction.buffer.impl.TransactionBufferClientStatsImpl;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+
+public interface TransactionBufferClientStats {
+
+ void recordAbortFailed(String topic);
Review Comment:
PendingAck is in [PendingAckHandleStats.java](https://github.com/apache/pulsar/pull/15140/files#diff-13ac31e4202c3d4048fcbac33099531393cf361e9e0ebbd74ef8680ea18b2708)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1189975785
> new metrics should ends with "_total"
>
> See https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md
>
> Also see
>
> * #13785
> * #16591
> * #16610
> * #16611
it makes sense
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #15140:
URL: https://github.com/apache/pulsar/pull/15140
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r925354507
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/pendingack/PendingAckPersistentTest.java:
##########
@@ -200,6 +205,77 @@ public void individualPendingAckReplayTest() throws Exception {
.compareTo((PositionImpl) managedCursor.getManagedLedger().getLastConfirmedEntry()) == -1);
}
+ @Test
+ public void testPendingAckMetrics() throws Exception {
+ final int messageCount = 100;
+ String subName = "testMetric" + UUID.randomUUID();
+
+ @Cleanup
+ Producer<String> producer = pulsarClient.newProducer(Schema.STRING)
+ .topic(PENDING_ACK_REPLAY_TOPIC)
+ .create();
+
+ @Cleanup
+ Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING)
+ .topic(PENDING_ACK_REPLAY_TOPIC)
+ .subscriptionName(subName)
+ .subscriptionType(SubscriptionType.Exclusive)
+ .enableBatchIndexAcknowledgment(true)
+ .subscribe();
+
+ for (int a = 0; a < messageCount; a++) {
+ producer.send(UUID.randomUUID().toString());
+ }
+
+ for (int a = 0; a < messageCount; a++) {
+ Message<String> message = consumer.receive(10, TimeUnit.SECONDS);
+ if (null == message) {
+ break;
+ }
+
+ Transaction txn = pulsarClient.newTransaction()
+ .withTransactionTimeout(10, TimeUnit.SECONDS).build().get();
+ consumer.acknowledgeCumulativeAsync(message.getMessageId(), txn).get();
+ if (a % 2 == 0) {
+ txn.abort().get();
+ } else {
+ txn.commit().get();
+ }
+ }
+
+ @Cleanup
+ ByteArrayOutputStream statsOut = new ByteArrayOutputStream();
+ PrometheusMetricsGenerator.generate(pulsarServiceList.get(0), true, false, false, statsOut);
+ String metricsStr = statsOut.toString();
+ Multimap<String, PrometheusMetricsTest.Metric> metricsMap = PrometheusMetricsTest.parseMetrics(metricsStr);
+
+ Collection<PrometheusMetricsTest.Metric> abortedCount = metricsMap.get("pulsar_txn_tp_aborted_count");
Review Comment:
this's should be total too?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1208983918
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1190010048
@Shoothzj PTAL
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #15140: [transaction][monitor] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1096997618
Tests to be completed
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #15140: [transaction][monitor] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1098929018
> > Good work! as the seem as tb, we should add tp stats
>
> I know TC means `TransactionCoordinator` and TB means `TransactionBuffer`, but what means TP?
Does it mean `TransactionPendingAckStore` ?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaoran10 commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r895971996
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientStats.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.pulsar.broker.transaction.buffer;
+
+import org.apache.pulsar.broker.transaction.buffer.impl.TransactionBufferClientStatsImpl;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+
+public interface TransactionBufferClientStats {
+
+ void recordAbortFailed(String topic);
Review Comment:
@congbobo184 @codelipenghui Do we need to distinguish between commit(abort) TB or commit(abort) PendingAck? Same as operation latency. Maybe it's useful to find problems.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r902607802
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TransactionBufferClientStatsImpl.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pulsar.broker.transaction.buffer.impl;
+
+import io.prometheus.client.CollectorRegistry;
+import io.prometheus.client.Counter;
+import io.prometheus.client.Gauge;
+import io.prometheus.client.Summary;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.pulsar.broker.transaction.buffer.TransactionBufferClientStats;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+import org.apache.pulsar.common.naming.TopicName;
+
+public final class TransactionBufferClientStatsImpl implements TransactionBufferClientStats {
+ private static final double[] QUANTILES = {0.50, 0.75, 0.95, 0.99, 0.999, 0.9999, 1};
+ private final AtomicBoolean closed = new AtomicBoolean(false);
+
+ private final Counter abortFailed;
+ private final Counter commitFailed;
+ private final Summary abortLatency;
+ private final Summary commitLatency;
+ private final Gauge pendingRequests;
+
+ private final boolean exposeTopicLevelMetrics;
+
+ private static TransactionBufferClientStats instance;
+
+ private TransactionBufferClientStatsImpl(boolean exposeTopicLevelMetrics,
+ TransactionBufferHandler handler) {
+ this.exposeTopicLevelMetrics = exposeTopicLevelMetrics;
+ String[] labelNames = exposeTopicLevelMetrics
+ ? new String[]{"namespace", "topic"} : new String[]{"namespace"};
+
+ this.abortFailed = Counter.build("pulsar_txn_tb_client_abort_failed", "-")
+ .labelNames(labelNames)
+ .register();
+ this.commitFailed = Counter.build("pulsar_txn_tb_client_commit_failed", "-")
+ .labelNames(labelNames)
+ .register();
+ this.abortLatency =
+ this.buildSummary("pulsar_txn_tb_client_abort_latency", "-", labelNames);
+ this.commitLatency =
+ this.buildSummary("pulsar_txn_tb_client_commit_latency", "-", labelNames);
+
+ this.pendingRequests = Gauge.build("pulsar_txn_tb_client_pending_requests", "-")
+ .register()
+ .setChild(new Gauge.Child() {
+ @Override
+ public double get() {
+ return null == handler ? 0 : handler.getPendingRequestsCount();
+ }
+ });
+ }
+
+ private Summary buildSummary(String name, String help, String[] labelNames) {
+ Summary.Builder builder = Summary.build(name, help)
+ .labelNames(labelNames);
+ for (double quantile : QUANTILES) {
+ builder.quantile(quantile, 0.01D);
+ }
+ return builder.register();
+ }
+
+ public static synchronized TransactionBufferClientStats getInstance(boolean exposeTopicLevelMetrics,
+ TransactionBufferHandler handler) {
+ if (null == instance) {
+ instance = new TransactionBufferClientStatsImpl(exposeTopicLevelMetrics, handler);
+ }
+
+ return instance;
+ }
+
+ @Override
+ public void recordAbortFailed(String topic) {
+ this.abortFailed.labels(labelValues(topic)).inc();
+ }
+
+ @Override
+ public void recordCommitFailed(String topic) {
+ this.commitFailed.labels(labelValues(topic)).inc();
+ }
+
+ @Override
+ public void recordAbortLatency(String topic, long cost) {
+ this.abortLatency.labels(labelValues(topic)).observe(cost);
+ }
+
+ @Override
+ public void recordCommitLatency(String topic, long cost) {
+ this.commitLatency.labels(labelValues(topic)).observe(cost);
+ }
+
+ private String[] labelValues(String topic) {
+ try {
+ TopicName topicName = TopicName.get(topic);
+ return exposeTopicLevelMetrics
+ ? new String[]{topicName.getNamespace(), topic} : new String[]{topicName.getNamespace()};
+ } catch (Throwable t) {
Review Comment:
I believe no need, it just in case of TopicName.get() throws exception.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #15140: [transaction][monitor] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1097018691
@tjiuming:Thanks for providing doc info!
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #15140: [transaction][monitor] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1098918217
> Good work! as the seem as tb, we should add tp stats
I know TC means `TransactionCoordinator` and TB means `TransactionBuffer`, but what means TP?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1133802332
The pr had no activity for 30 days, mark with Stale label.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r904446674
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientStats.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.pulsar.broker.transaction.buffer;
+
+import org.apache.pulsar.broker.transaction.buffer.impl.TransactionBufferClientStatsImpl;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+
+public interface TransactionBufferClientStats {
+
+ void recordAbortFailed(String topic);
Review Comment:
If TransactionBuffer add the latency, I think PendingAck also need to add this metrics
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r902428158
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/pendingack/impl/PendingAckHandleStatsImpl.java:
##########
@@ -0,0 +1,95 @@
+/**
+ * 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.pulsar.broker.transaction.pendingack.impl;
+
+import io.prometheus.client.Counter;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pulsar.broker.transaction.pendingack.PendingAckHandleStats;
+import org.apache.pulsar.common.naming.TopicName;
+
+public class PendingAckHandleStatsImpl implements PendingAckHandleStats {
+ private static final AtomicBoolean INITIALIZED = new AtomicBoolean(false);
+ private static Counter commitTxnCounter;
+ private static Counter abortTxnCounter;
+ private static boolean exposeTopicLevelMetrics0;
+
+ private final String[] labelSucceed;
+ private final String[] labelFailed;
+
+ public PendingAckHandleStatsImpl(String topic, String subscription, boolean exposeTopicLevelMetrics) {
+ initialize(exposeTopicLevelMetrics);
+
+ String namespace;
+ if (StringUtils.isBlank(topic)) {
+ namespace = topic = "unknown";
Review Comment:
do we have other metrics that report "unknown" as topic when it is not set ?
I think that we should throw a error in this case, it is not possible to see a blank topic name here
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TransactionBufferClientImpl.java:
##########
@@ -35,47 +37,88 @@
public class TransactionBufferClientImpl implements TransactionBufferClient {
private final TransactionBufferHandler tbHandler;
+ private final TransactionBufferClientStats stats;
- private TransactionBufferClientImpl(TransactionBufferHandler tbHandler) {
+ private TransactionBufferClientImpl(TransactionBufferHandler tbHandler, boolean exposeTopicLevelMetrics,
+ boolean enableTxnCoordinator) {
this.tbHandler = tbHandler;
+ this.stats = TransactionBufferClientStats.create(exposeTopicLevelMetrics, tbHandler, enableTxnCoordinator);
}
public static TransactionBufferClient create(PulsarService pulsarService, HashedWheelTimer timer,
int maxConcurrentRequests, long operationTimeoutInMills) throws PulsarServerException {
TransactionBufferHandler handler = new TransactionBufferHandlerImpl(pulsarService, timer,
maxConcurrentRequests, operationTimeoutInMills);
- return new TransactionBufferClientImpl(handler);
+
+ ServiceConfiguration config = pulsarService.getConfig();
+ boolean exposeTopicLevelMetrics = config.isExposeTopicLevelMetricsInPrometheus();
+ boolean enableTxnCoordinator = config.isTransactionCoordinatorEnabled();
+ return new TransactionBufferClientImpl(handler, exposeTopicLevelMetrics, enableTxnCoordinator);
}
@Override
public CompletableFuture<TxnID> commitTxnOnTopic(String topic, long txnIdMostBits,
long txnIdLeastBits, long lowWaterMark) {
- return tbHandler.endTxnOnTopic(topic, txnIdMostBits, txnIdLeastBits, TxnAction.COMMIT, lowWaterMark);
+ long start = System.currentTimeMillis();
Review Comment:
I think that we usually use "nanos", because millis will be usually 0 (hopefully)
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -1867,6 +1867,9 @@ public CompletableFuture<TopicStatsImpl> asyncGetStats(boolean getPreciseBacklog
stats.bytesOutCounter = bytesOutFromRemovedSubscriptions.longValue();
stats.msgOutCounter = msgOutFromRemovedSubscriptions.longValue();
stats.publishRateLimitedTimes = publishRateLimitedTimes;
+ stats.ongoingTxnCount = getTransactionBuffer().getOngoingTxnCount();
Review Comment:
nit: can we call getTransactionBuffer() only once ?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TransactionBufferClientStatsImpl.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pulsar.broker.transaction.buffer.impl;
+
+import io.prometheus.client.CollectorRegistry;
+import io.prometheus.client.Counter;
+import io.prometheus.client.Gauge;
+import io.prometheus.client.Summary;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.pulsar.broker.transaction.buffer.TransactionBufferClientStats;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+import org.apache.pulsar.common.naming.TopicName;
+
+public final class TransactionBufferClientStatsImpl implements TransactionBufferClientStats {
+ private static final double[] QUANTILES = {0.50, 0.75, 0.95, 0.99, 0.999, 0.9999, 1};
+ private final AtomicBoolean closed = new AtomicBoolean(false);
+
+ private final Counter abortFailed;
+ private final Counter commitFailed;
+ private final Summary abortLatency;
+ private final Summary commitLatency;
+ private final Gauge pendingRequests;
+
+ private final boolean exposeTopicLevelMetrics;
+
+ private static TransactionBufferClientStats instance;
+
+ private TransactionBufferClientStatsImpl(boolean exposeTopicLevelMetrics,
+ TransactionBufferHandler handler) {
+ this.exposeTopicLevelMetrics = exposeTopicLevelMetrics;
+ String[] labelNames = exposeTopicLevelMetrics
+ ? new String[]{"namespace", "topic"} : new String[]{"namespace"};
+
+ this.abortFailed = Counter.build("pulsar_txn_tb_client_abort_failed", "-")
+ .labelNames(labelNames)
+ .register();
Review Comment:
are we using the default global registry ?
what happens if you run two brokers inside the same JVM ?
is it a pattern that we follow for other metrics ?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1150730586
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r925363815
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/pendingack/PendingAckPersistentTest.java:
##########
@@ -200,6 +205,77 @@ public void individualPendingAckReplayTest() throws Exception {
.compareTo((PositionImpl) managedCursor.getManagedLedger().getLastConfirmedEntry()) == -1);
}
+ @Test
+ public void testPendingAckMetrics() throws Exception {
+ final int messageCount = 100;
+ String subName = "testMetric" + UUID.randomUUID();
+
+ @Cleanup
+ Producer<String> producer = pulsarClient.newProducer(Schema.STRING)
+ .topic(PENDING_ACK_REPLAY_TOPIC)
+ .create();
+
+ @Cleanup
+ Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING)
+ .topic(PENDING_ACK_REPLAY_TOPIC)
+ .subscriptionName(subName)
+ .subscriptionType(SubscriptionType.Exclusive)
+ .enableBatchIndexAcknowledgment(true)
+ .subscribe();
+
+ for (int a = 0; a < messageCount; a++) {
+ producer.send(UUID.randomUUID().toString());
+ }
+
+ for (int a = 0; a < messageCount; a++) {
+ Message<String> message = consumer.receive(10, TimeUnit.SECONDS);
+ if (null == message) {
+ break;
+ }
+
+ Transaction txn = pulsarClient.newTransaction()
+ .withTransactionTimeout(10, TimeUnit.SECONDS).build().get();
+ consumer.acknowledgeCumulativeAsync(message.getMessageId(), txn).get();
+ if (a % 2 == 0) {
+ txn.abort().get();
+ } else {
+ txn.commit().get();
+ }
+ }
+
+ @Cleanup
+ ByteArrayOutputStream statsOut = new ByteArrayOutputStream();
+ PrometheusMetricsGenerator.generate(pulsarServiceList.get(0), true, false, false, statsOut);
+ String metricsStr = statsOut.toString();
+ Multimap<String, PrometheusMetricsTest.Metric> metricsMap = PrometheusMetricsTest.parseMetrics(metricsStr);
+
+ Collection<PrometheusMetricsTest.Metric> abortedCount = metricsMap.get("pulsar_txn_tp_aborted_count");
Review Comment:
yes, sorry, I forgot it.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r912366595
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientStats.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.pulsar.broker.transaction.buffer;
+
+import org.apache.pulsar.broker.transaction.buffer.impl.TransactionBufferClientStatsImpl;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+
+public interface TransactionBufferClientStats {
+
+ void recordAbortFailed(String topic);
Review Comment:
`pulsar_txn_tp_commit_latency` added
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r902098175
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientStats.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.pulsar.broker.transaction.buffer;
+
+import org.apache.pulsar.broker.transaction.buffer.impl.TransactionBufferClientStatsImpl;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+
+public interface TransactionBufferClientStats {
+
+ void recordAbortFailed(String topic);
Review Comment:
yes, we need to distinguish TB and TP is better @tjiuming
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #15140: [transaction][monitor] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1098929011
> > Good work! as the seem as tb, we should add tp stats
>
> I know TC means `TransactionCoordinator` and TB means `TransactionBuffer`, but what means TP?
Does it mean `TransactionPendingAckStore` ?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r854009300
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/pendingack/impl/PendingAckHandleStatsImpl.java:
##########
@@ -0,0 +1,73 @@
+/**
+ * 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.pulsar.broker.transaction.pendingack.impl;
+
+import io.prometheus.client.Counter;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pulsar.broker.transaction.pendingack.PendingAckHandleStats;
+import org.apache.pulsar.common.naming.TopicName;
+
+public class PendingAckHandleStatsImpl implements PendingAckHandleStats {
+ private static final Counter COMMIT_TXN_COUNTER = Counter
+ .build("pulsar_txn_tp_committed_count", "-")
+ .labelNames("namespace", "topic", "subscription", "status")
Review Comment:
fixed
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #15140: [transaction][monitor] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1097018239
@tjiuming:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #15140: [transaction][monitor] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1097468432
@momo-jun a soft reminder: here is a PR w/ doc-required label, could u pls follow up? Thanks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r902511726
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -1867,6 +1867,9 @@ public CompletableFuture<TopicStatsImpl> asyncGetStats(boolean getPreciseBacklog
stats.bytesOutCounter = bytesOutFromRemovedSubscriptions.longValue();
stats.msgOutCounter = msgOutFromRemovedSubscriptions.longValue();
stats.publishRateLimitedTimes = publishRateLimitedTimes;
+ stats.ongoingTxnCount = getTransactionBuffer().getOngoingTxnCount();
Review Comment:
it makes sense, I‘ll change it.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #15140: [transaction][monitor] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1099928120
>
I‘ll create another PR to add doc after this PR finished
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r853093818
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/pendingack/impl/PendingAckHandleStatsImpl.java:
##########
@@ -0,0 +1,73 @@
+/**
+ * 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.pulsar.broker.transaction.pendingack.impl;
+
+import io.prometheus.client.Counter;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pulsar.broker.transaction.pendingack.PendingAckHandleStats;
+import org.apache.pulsar.common.naming.TopicName;
+
+public class PendingAckHandleStatsImpl implements PendingAckHandleStats {
+ private static final Counter COMMIT_TXN_COUNTER = Counter
+ .build("pulsar_txn_tp_committed_count", "-")
+ .labelNames("namespace", "topic", "subscription", "status")
Review Comment:
Need to check if topic level metrics is enabled, otherwise we should only expose the namespace level metrics
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r902605278
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TransactionBufferClientStatsImpl.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pulsar.broker.transaction.buffer.impl;
+
+import io.prometheus.client.CollectorRegistry;
+import io.prometheus.client.Counter;
+import io.prometheus.client.Gauge;
+import io.prometheus.client.Summary;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.pulsar.broker.transaction.buffer.TransactionBufferClientStats;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+import org.apache.pulsar.common.naming.TopicName;
+
+public final class TransactionBufferClientStatsImpl implements TransactionBufferClientStats {
+ private static final double[] QUANTILES = {0.50, 0.75, 0.95, 0.99, 0.999, 0.9999, 1};
+ private final AtomicBoolean closed = new AtomicBoolean(false);
+
+ private final Counter abortFailed;
+ private final Counter commitFailed;
+ private final Summary abortLatency;
+ private final Summary commitLatency;
+ private final Gauge pendingRequests;
+
+ private final boolean exposeTopicLevelMetrics;
+
+ private static TransactionBufferClientStats instance;
+
+ private TransactionBufferClientStatsImpl(boolean exposeTopicLevelMetrics,
+ TransactionBufferHandler handler) {
+ this.exposeTopicLevelMetrics = exposeTopicLevelMetrics;
+ String[] labelNames = exposeTopicLevelMetrics
+ ? new String[]{"namespace", "topic"} : new String[]{"namespace"};
+
+ this.abortFailed = Counter.build("pulsar_txn_tb_client_abort_failed", "-")
+ .labelNames(labelNames)
+ .register();
+ this.commitFailed = Counter.build("pulsar_txn_tb_client_commit_failed", "-")
+ .labelNames(labelNames)
+ .register();
+ this.abortLatency =
+ this.buildSummary("pulsar_txn_tb_client_abort_latency", "-", labelNames);
+ this.commitLatency =
+ this.buildSummary("pulsar_txn_tb_client_commit_latency", "-", labelNames);
+
+ this.pendingRequests = Gauge.build("pulsar_txn_tb_client_pending_requests", "-")
+ .register()
+ .setChild(new Gauge.Child() {
+ @Override
+ public double get() {
+ return null == handler ? 0 : handler.getPendingRequestsCount();
+ }
+ });
+ }
+
+ private Summary buildSummary(String name, String help, String[] labelNames) {
+ Summary.Builder builder = Summary.build(name, help)
+ .labelNames(labelNames);
+ for (double quantile : QUANTILES) {
+ builder.quantile(quantile, 0.01D);
+ }
+ return builder.register();
+ }
+
+ public static synchronized TransactionBufferClientStats getInstance(boolean exposeTopicLevelMetrics,
+ TransactionBufferHandler handler) {
+ if (null == instance) {
+ instance = new TransactionBufferClientStatsImpl(exposeTopicLevelMetrics, handler);
+ }
+
+ return instance;
+ }
+
+ @Override
+ public void recordAbortFailed(String topic) {
+ this.abortFailed.labels(labelValues(topic)).inc();
+ }
+
+ @Override
+ public void recordCommitFailed(String topic) {
+ this.commitFailed.labels(labelValues(topic)).inc();
+ }
+
+ @Override
+ public void recordAbortLatency(String topic, long cost) {
+ this.abortLatency.labels(labelValues(topic)).observe(cost);
+ }
+
+ @Override
+ public void recordCommitLatency(String topic, long cost) {
+ this.commitLatency.labels(labelValues(topic)).observe(cost);
+ }
+
+ private String[] labelValues(String topic) {
+ try {
+ TopicName topicName = TopicName.get(topic);
+ return exposeTopicLevelMetrics
+ ? new String[]{topicName.getNamespace(), topic} : new String[]{topicName.getNamespace()};
+ } catch (Throwable t) {
+ return exposeTopicLevelMetrics ? new String[]{"unknown", "unknown"} : new String[]{"unknown"};
+ }
+ }
+
+ @Override
+ public void close() {
+ if (instance == this && this.closed.compareAndSet(false, true)) {
Review Comment:
it makes sense.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r902606375
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientStats.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.pulsar.broker.transaction.buffer;
+
+import org.apache.pulsar.broker.transaction.buffer.impl.TransactionBufferClientStatsImpl;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+
+public interface TransactionBufferClientStats {
+
+ void recordAbortFailed(String topic);
+
+ void recordCommitFailed(String topic);
+
+ void recordAbortLatency(String topic, long cost);
+
+ void recordCommitLatency(String topic, long cost);
+
+ void close();
Review Comment:
AutoCloseable#close throws Exception, no need here, same as below
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r902605560
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -245,6 +248,20 @@ public CompletableFuture<Void> checkIfTBRecoverCompletely(boolean isTxnEnabled)
}
}
+ @Override
+ public long getOngoingTxnCount() {
Review Comment:
it makes sense.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a diff in pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on code in PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#discussion_r903222836
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientStats.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.pulsar.broker.transaction.buffer;
+
+import org.apache.pulsar.broker.transaction.buffer.impl.TransactionBufferClientStatsImpl;
+import org.apache.pulsar.client.impl.transaction.TransactionBufferHandler;
+
+public interface TransactionBufferClientStats {
+
+ void recordAbortFailed(String topic);
Review Comment:
I see [TransactionBuffer.java](https://github.com/apache/pulsar/pull/15140/files#diff-819cb066b87a312a85c161d7bccf5699ddd8e1aded6545a6f39a237d16e6e707) and [PendingAckHandleStatsImpl](https://github.com/apache/pulsar/pull/15140/files#diff-3e4c8d1822aa122c6b58f1b2c8227a6dac1354cbeeb1f68cab24a6587a881ea5)
The implementation is different; what is the consideration here? I missed a part of the code before, sorry
```
public void recordCommitTxn(boolean success) {
commitTxnCounter.labels(success ? labelSucceed : labelFailed).inc();
}
```
```
private final Counter abortFailed;
private final Counter commitFailed;
private final Summary abortLatency;
private final Summary commitLatency;
```
I think in TransactionBuffer the latency is managedLedger append entry latency, it also already exists. So we only need to add the count and the rate.the same as PendingAckStats
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tjiuming commented on pull request #15140: [monitor][txn] Add metrics for transaction
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #15140:
URL: https://github.com/apache/pulsar/pull/15140#issuecomment-1173021729
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org