ibessonov commented on code in PR #7722:
URL: https://github.com/apache/ignite-3/pull/7722#discussion_r2944872686
##########
modules/raft/src/main/java/org/apache/ignite/internal/metrics/sources/FsmCallerMetricSource.java:
##########
@@ -52,54 +51,87 @@ protected Holder createHolder() {
/**
* Called on FSM commit.
*
- * @param time Duration of the commit operation.
+ * @param duration Duration of the commit operation.
*/
- public void onFsmCommit(long time) {
+ public void onFsmCommit(long duration) {
Holder holder = holder();
if (holder != null) {
- holder.lastCommitTime.value(time);
+ holder.commitTime.add(duration);
}
}
/**
* Called on applying tasks.
*
- * @param time Duration of the apply operation.
+ * @param duration Duration of the apply operation.
* @param size Number of tasks applied.
*/
- public void onApplyTasks(long time, long size) {
+ public void onApplyTasks(long duration, long size) {
Holder holder = holder();
if (holder != null) {
- holder.lastApplyTasksTime.value(time);
+ holder.applyTasksTime.add(duration);
holder.applyTasksSize.add(size);
}
}
+ /**
+ * Called on applying task.
+ *
+ * @param type Type of the applied task.
+ * @param duration Duration of the apply operation.
+ * */
+ public void onApplyTask(TaskType type, long duration) {
+ Holder holder = holder();
+
+ if (holder != null) {
+ holder.taskDurations.get(type).add(duration);
+ }
+ }
+
/** Metric holder for FSM caller metrics. */
static class Holder implements AbstractMetricSource.Holder<Holder> {
+ private static final long[] HISTOGRAM_BUCKETS =
+ {10, 50, 100, 200, 500, 1000, 2000, 5000, 10000, 20000, 50000,
100000, 200000, 500000};
+
private final DistributionMetric applyTasksSize = new
DistributionMetric(
"ApplyTasksSize",
"Sizes of applied tasks batches",
new long[] {10, 20, 30, 40, 50}
);
- private final AtomicLongMetric lastApplyTasksTime = new
AtomicLongMetric("ApplyTasksTime", "Time to apply tasks");
+ private final DistributionMetric applyTasksTime = new
DistributionMetric(
+ "ApplyTasksTime",
+ "Duration of applying tasks in milliseconds",
+ HISTOGRAM_BUCKETS
+ );
- private final AtomicLongMetric lastCommitTime = new
AtomicLongMetric("CommitTime", "Time to apply tasks");
+ private final DistributionMetric commitTime = new DistributionMetric(
+ "CommitTime",
+ "Duration of committing in milliseconds",
+ HISTOGRAM_BUCKETS
+ );
private final List<Metric> metrics;
+ private final HashMap<TaskType, DistributionMetric> taskDurations =
new HashMap<>();
+
Holder() {
metrics = new ArrayList<>();
metrics.add(applyTasksSize);
- metrics.add(lastApplyTasksTime);
- metrics.add(lastCommitTime);
+ metrics.add(applyTasksTime);
+ metrics.add(commitTime);
Review Comment:
Could be `metrics = List.of(applyTasksSize, applyTasksTime, commitTime)`.
I wonder why you decided to write it this way, for future extensibility?
Will this list become dynamic in the near future?
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/CliIntegrationTest.java:
##########
@@ -382,10 +381,10 @@ public static MetricSource[]
getExpectedNodeMetrics(Ignite ignite) {
new MetricSource().name("raft.snapshots").enabled(true),
new MetricSource().name("messaging").enabled(true),
new MetricSource().name("log.storage").enabled(true),
- new MetricSource().name(THREAD_POOLS_METRICS_SOURCE_NAME +
"striped.messaging.inbound.default").enabled(true),
- new MetricSource().name(THREAD_POOLS_METRICS_SOURCE_NAME +
"striped.messaging.inbound.deploymentunits").enabled(true),
- new MetricSource().name(THREAD_POOLS_METRICS_SOURCE_NAME +
"striped.messaging.inbound.scalecube").enabled(true),
- new MetricSource().name(THREAD_POOLS_METRICS_SOURCE_NAME +
"messaging.outbound").enabled(true)
+ new
MetricSource().name("thread.pools.striped.messaging.inbound.default").enabled(true),
Review Comment:
Would be nice to sort them alphabetically, but maybe that's too much to ask
##########
modules/raft/src/main/java/org/apache/ignite/internal/metrics/sources/LogManagerMetricSource.java:
##########
@@ -81,20 +82,25 @@ public void onAppendLogs(int entriesCount, int writtenSize,
long duration) {
if (holder != null) {
holder.appendLogsCount.add(entriesCount);
holder.appendLogsSize.add(writtenSize);
- holder.lastAppendLogsDuration.value(duration);
+ holder.appendLogsDuration.add(duration);
}
}
/** Metric holder for log manager metrics. */
static class Holder implements AbstractMetricSource.Holder<Holder> {
- private final AtomicLongMetric lastTruncateLogSuffixTime = new
AtomicLongMetric(
+ private static final long[] HISTOGRAM_BUCKETS =
+ {10, 50, 100, 200, 500, 1000, 2000, 5000, 10000, 20000, 50000,
100000, 200000, 500000};
Review Comment:
Up to 500 thousands? Woooow, that's too much. Are these milliseconds, what
will they measure?
Some operations are better measured in microseconds, it has to be
metric-dependent. Let's discuss these bounds separately
##########
modules/raft/src/main/java/org/apache/ignite/internal/metrics/sources/FsmCallerMetricSource.java:
##########
@@ -52,54 +51,87 @@ protected Holder createHolder() {
/**
* Called on FSM commit.
*
- * @param time Duration of the commit operation.
+ * @param duration Duration of the commit operation.
*/
- public void onFsmCommit(long time) {
+ public void onFsmCommit(long duration) {
Holder holder = holder();
if (holder != null) {
- holder.lastCommitTime.value(time);
+ holder.commitTime.add(duration);
}
}
/**
* Called on applying tasks.
*
- * @param time Duration of the apply operation.
+ * @param duration Duration of the apply operation.
* @param size Number of tasks applied.
*/
- public void onApplyTasks(long time, long size) {
+ public void onApplyTasks(long duration, long size) {
Holder holder = holder();
if (holder != null) {
- holder.lastApplyTasksTime.value(time);
+ holder.applyTasksTime.add(duration);
holder.applyTasksSize.add(size);
}
}
+ /**
+ * Called on applying task.
+ *
+ * @param type Type of the applied task.
+ * @param duration Duration of the apply operation.
+ * */
+ public void onApplyTask(TaskType type, long duration) {
+ Holder holder = holder();
+
+ if (holder != null) {
+ holder.taskDurations.get(type).add(duration);
+ }
+ }
+
/** Metric holder for FSM caller metrics. */
static class Holder implements AbstractMetricSource.Holder<Holder> {
+ private static final long[] HISTOGRAM_BUCKETS =
+ {10, 50, 100, 200, 500, 1000, 2000, 5000, 10000, 20000, 50000,
100000, 200000, 500000};
+
private final DistributionMetric applyTasksSize = new
DistributionMetric(
"ApplyTasksSize",
"Sizes of applied tasks batches",
new long[] {10, 20, 30, 40, 50}
);
- private final AtomicLongMetric lastApplyTasksTime = new
AtomicLongMetric("ApplyTasksTime", "Time to apply tasks");
+ private final DistributionMetric applyTasksTime = new
DistributionMetric(
+ "ApplyTasksTime",
+ "Duration of applying tasks in milliseconds",
+ HISTOGRAM_BUCKETS
+ );
- private final AtomicLongMetric lastCommitTime = new
AtomicLongMetric("CommitTime", "Time to apply tasks");
+ private final DistributionMetric commitTime = new DistributionMetric(
+ "CommitTime",
+ "Duration of committing in milliseconds",
+ HISTOGRAM_BUCKETS
+ );
private final List<Metric> metrics;
+ private final HashMap<TaskType, DistributionMetric> taskDurations =
new HashMap<>();
Review Comment:
1. Type should be declared as `Map`, not `HashMap`. Why did you do this?
2. Please use `EnumMap` for enums. A dedicated type of map is better than a
universal one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
