Re: [PR] OAK-10519 - Export metrics related to reindexing [jackrabbit-oak]
nfsantos merged PR #1182: URL: https://github.com/apache/jackrabbit-oak/pull/1182 -- 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: dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] OAK-10519 - Export metrics related to reindexing [jackrabbit-oak]
nfsantos commented on code in PR #1182:
URL: https://github.com/apache/jackrabbit-oak/pull/1182#discussion_r1395840722
##
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/pipelined/MetricsUtils.java:
##
@@ -0,0 +1,41 @@
+/*
+ * 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.jackrabbit.oak.index.indexer.document.flatfile.pipelined;
+
+interface PipelinedMetrics {
Review Comment:
I changed it to a class, following the advice here:
https://www.baeldung.com/java-constants-good-practices
I prefer to keep all the metrics in a single place instead of spreading them
over all the classes in the pipelined package, this makes it easier to get an
overview of all the metrics. And in a way, which class publishes which metric
is an implementation detail, I prefer to see it as the pipelined strategy
publishing 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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10519 - Export metrics related to reindexing [jackrabbit-oak]
fabriziofortino commented on code in PR #1182:
URL: https://github.com/apache/jackrabbit-oak/pull/1182#discussion_r1395720389
##
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/pipelined/MetricsUtils.java:
##
@@ -0,0 +1,41 @@
+/*
+ * 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.jackrabbit.oak.index.indexer.document.flatfile.pipelined;
+
+interface PipelinedMetrics {
Review Comment:
this is considered by many an antipattern (see Constant interface
anti-pattern). In general, I tend to have the constant in the classes where
they are used most. Arguments are mainly stylistic, so feel free to find
alternatives.
##
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/pipelined/PipelinedStrategy.java:
##
@@ -222,9 +226,10 @@ private static void
prettyPrintTransformStatisticsHistograms(TransformStageStati
/**
- * @param pathPredicate Used by the transform stage to test if a node
should be kept or discarded.
- * @param pathFilters If non-empty, the download stage will use these
filters to try to create a query that downloads
- * only the matching MongoDB documents.
+ * @param pathPredicate Used by the transform stage to test if a node
should be kept or discarded.
+ * @param pathFiltersIf non-empty, the download stage will use
these filters to try to create a query that downloads
+ * only the matching MongoDB documents.
+ * @param statisticsProvider
Review Comment:
add a description or remove the `@param`
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10519 - Export metrics related to reindexing [jackrabbit-oak]
nfsantos commented on code in PR #1182:
URL: https://github.com/apache/jackrabbit-oak/pull/1182#discussion_r1394411438
##
oak-run-commons/src/test/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/pipelined/PipelinedMergeSortTaskTest.java:
##
@@ -89,27 +94,41 @@ private PipelinedMergeSortTask.Result runTest(Compression
algorithm, Path... fil
Path sortRoot = sortFolder.getRoot().toPath();
// +1 for the Sentinel.
ArrayBlockingQueue sortedFilesQueue = new
ArrayBlockingQueue<>(files.length + 1);
-PipelinedMergeSortTask mergeSortTask = new
PipelinedMergeSortTask(sortRoot, pathComparator, algorithm, sortedFilesQueue);
-// Enqueue all the files that are to be merged
-for (Path file : files) {
-// The intermediate files are deleted after being merged, so we
should copy them to the temporary sort root folder
-Path workDirCopy = Files.copy(file,
sortRoot.resolve(file.getFileName()));
-sortedFilesQueue.put(workDirCopy);
+ScheduledExecutorService executor =
Executors.newSingleThreadScheduledExecutor();
+try (MetricStatisticsProvider metricStatisticsProvider = new
MetricStatisticsProvider(null, executor)) {
Review Comment:
If we pass null to the constructor, the `MetricStatisticsProvider` will use
the `ManagementFactory.getPlatformMBeanServer()`. I think the problem here is
lack of documentation on the construtor of `MetricStatisticsProvider`. I don't
think this PR should correct the missing documentation.
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10519 - Export metrics related to reindexing [jackrabbit-oak]
nfsantos commented on code in PR #1182:
URL: https://github.com/apache/jackrabbit-oak/pull/1182#discussion_r1394404746
##
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/MetricsUtils.java:
##
@@ -0,0 +1,40 @@
+/*
+ * 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.jackrabbit.oak.plugins.index;
+
+import org.apache.jackrabbit.oak.stats.CounterStats;
+import org.apache.jackrabbit.oak.stats.StatisticsProvider;
+import org.apache.jackrabbit.oak.stats.StatsOptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class MetricsUtils {
+private final static Logger LOG =
LoggerFactory.getLogger(MetricsUtils.class);
+
+public static void setCounter(StatisticsProvider statisticsProvider,
String name, long value) {
+CounterStats metric = statisticsProvider.getCounterStats(name,
StatsOptions.METRICS_ONLY);
+LOG.debug("Adding metric: {} {}", name, value);
+if (metric.getCount() != 0) {
+LOG.warn("Counter was not 0: {} {}", name, metric.getCount());
Review Comment:
I added documentation explaining the intended use of this method and renamed
it to `setCounterOnce`. I did not rename the class as that seems excessive. And
in the future we may want to have other util methods in this class that do not
have the restriction of this method.
I'm not totally satisfied with this design, maybe this can be revised in a
future PR. One option would be to update the metrics directly when the events
occur instead of having internal counters. But for the time being, I think this
will be enough.
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10519 - Export metrics related to reindexing [jackrabbit-oak]
nfsantos commented on code in PR #1182:
URL: https://github.com/apache/jackrabbit-oak/pull/1182#discussion_r1394377109
##
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/pipelined/PipelinedMongoDownloadTask.java:
##
@@ -191,15 +196,19 @@ public Result call() throws Exception {
} else {
downloadWithNaturalOrdering();
}
-String enqueueingDelayPercentage = String.format("%1.2f", (100.0 *
totalEnqueueWaitTimeMillis) /
downloadStartWatch.elapsed(TimeUnit.MILLISECONDS));
+double enqueueingDelayPercentage = (100.0 *
totalEnqueueWaitTimeMillis) / downloadStartWatch.elapsed(TimeUnit.MILLISECONDS);
Review Comment:
This is fixed in https://github.com/apache/jackrabbit-oak/pull/1202
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10519 - Export metrics related to reindexing [jackrabbit-oak]
thomasmueller commented on code in PR #1182:
URL: https://github.com/apache/jackrabbit-oak/pull/1182#discussion_r1394235775
##
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/pipelined/PipelinedMongoDownloadTask.java:
##
@@ -191,15 +196,19 @@ public Result call() throws Exception {
} else {
downloadWithNaturalOrdering();
}
-String enqueueingDelayPercentage = String.format("%1.2f", (100.0 *
totalEnqueueWaitTimeMillis) /
downloadStartWatch.elapsed(TimeUnit.MILLISECONDS));
+double enqueueingDelayPercentage = (100.0 *
totalEnqueueWaitTimeMillis) / downloadStartWatch.elapsed(TimeUnit.MILLISECONDS);
Review Comment:
Possible division by zero
##
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/MetricsUtils.java:
##
@@ -0,0 +1,40 @@
+/*
+ * 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.jackrabbit.oak.plugins.index;
+
+import org.apache.jackrabbit.oak.stats.CounterStats;
+import org.apache.jackrabbit.oak.stats.StatisticsProvider;
+import org.apache.jackrabbit.oak.stats.StatsOptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class MetricsUtils {
+private final static Logger LOG =
LoggerFactory.getLogger(MetricsUtils.class);
+
+public static void setCounter(StatisticsProvider statisticsProvider,
String name, long value) {
+CounterStats metric = statisticsProvider.getCounterStats(name,
StatsOptions.METRICS_ONLY);
+LOG.debug("Adding metric: {} {}", name, value);
+if (metric.getCount() != 0) {
+LOG.warn("Counter was not 0: {} {}", name, metric.getCount());
Review Comment:
I understand that metrics are used for logging in this case, so each metric
is only set once, and then it is not supposed to be called. But that's not the
normal usage of metrics. In my view, it is using metrics for something that
metrics are not designed for: to log. The main issue, of course, is that
delivering the metrics update may not work. It is fine if metrics are not sent
from time to time. I understand you do this on purpose, kind of a "best effort".
But someone else that sees this class doesn't know that. He will think
"MetricsUtils.setCounter... nice, I'll use that". And then we fill the log with
warnings...
What about adding Javadocs to the class, then renaming the class to
"SetOnceMetricsUtils" and the method to "setCounterOnce"? That way, it is more
clear about the intended usage, and avoid confusion (and so unnecessarily
filling the log file).
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10519 - Export metrics related to reindexing [jackrabbit-oak]
steffenvan commented on code in PR #1182:
URL: https://github.com/apache/jackrabbit-oak/pull/1182#discussion_r1394234364
##
oak-run-commons/src/test/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/pipelined/PipelinedMergeSortTaskTest.java:
##
@@ -89,27 +94,41 @@ private PipelinedMergeSortTask.Result runTest(Compression
algorithm, Path... fil
Path sortRoot = sortFolder.getRoot().toPath();
// +1 for the Sentinel.
ArrayBlockingQueue sortedFilesQueue = new
ArrayBlockingQueue<>(files.length + 1);
-PipelinedMergeSortTask mergeSortTask = new
PipelinedMergeSortTask(sortRoot, pathComparator, algorithm, sortedFilesQueue);
-// Enqueue all the files that are to be merged
-for (Path file : files) {
-// The intermediate files are deleted after being merged, so we
should copy them to the temporary sort root folder
-Path workDirCopy = Files.copy(file,
sortRoot.resolve(file.getFileName()));
-sortedFilesQueue.put(workDirCopy);
+ScheduledExecutorService executor =
Executors.newSingleThreadScheduledExecutor();
+try (MetricStatisticsProvider metricStatisticsProvider = new
MetricStatisticsProvider(null, executor)) {
Review Comment:
Is that because you are not interested in testing whether the values of the
metrics? To me it's not 100 % clear why the tests don't need an MBean server -
in other tests, I see they are there. E.g. I remember it was necessary in the
tests for the `NodeCounter` a while ago.
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10519 - Export metrics related to reindexing [jackrabbit-oak]
nfsantos commented on code in PR #1182:
URL: https://github.com/apache/jackrabbit-oak/pull/1182#discussion_r1394017086
##
oak-run/src/test/java/org/apache/jackrabbit/oak/index/DocumentStoreIndexerIT.java:
##
@@ -380,6 +387,95 @@ protected CompositeIndexer prepareIndexers(NodeStore
nodeStore, NodeBuilder buil
}
+@Test
+public void metrics() throws Exception {
+MongoConnection mongoConnection = getConnection();
+DocumentNodeStoreBuilder docBuilder = builderProvider.newBuilder()
+.setMongoDB(mongoConnection.getMongoClient(),
mongoConnection.getDBName());
+DocumentNodeStore store = docBuilder.build();
+
+Whiteboard wb = new DefaultWhiteboard();
+MongoDocumentStore ds = (MongoDocumentStore)
docBuilder.getDocumentStore();
+Registration r1 = wb.register(MongoDocumentStore.class, ds,
emptyMap());
+
+ScheduledExecutorService executor =
Executors.newSingleThreadScheduledExecutor();
+MetricStatisticsProvider metricsStatisticsProvider = new
MetricStatisticsProvider(null, executor);
+wb.register(StatisticsProvider.class, metricsStatisticsProvider,
emptyMap());
+Registration c1Registration = wb.register(MongoDatabase.class,
mongoConnection.getDatabase(), emptyMap());
+
+configureIndex(store);
+
+NodeBuilder builder = store.getRoot().builder();
+NodeBuilder appNB = newNode("app:Asset");
+createChild(appNB,
+"jcr:content",
+"jcr:content/comments",
+"jcr:content/metadata",
+"jcr:content/metadata/xmp",
+"jcr:content/renditions",
+"jcr:content/renditions/original",
+"jcr:content/renditions/original/jcr:content"
+);
+builder.child("test").setChildNode("book.jpg", appNB.getNodeState());
+store.merge(builder, EmptyHook.INSTANCE, CommitInfo.EMPTY);
+
+String checkpoint = store.checkpoint(10);
+
+//Shut down this store and restart in readOnly mode
+store.dispose();
Review Comment:
This variable is used to reference two different objects. And I agree that
the test could be improved, but I just copy'n'pasted the existing test and do
not want to spend much more time in refactoring it.
##
oak-run/src/test/java/org/apache/jackrabbit/oak/index/DocumentStoreIndexerIT.java:
##
@@ -380,6 +387,95 @@ protected CompositeIndexer prepareIndexers(NodeStore
nodeStore, NodeBuilder buil
}
+@Test
+public void metrics() throws Exception {
Review Comment:
It's not so simple to share the common code because it is creating several
objects that we need during the test. In this case, I think it is not worth the
effort to improve the code, some duplication is not a big issue here as this is
testing code and the duplication is contained in the same file.
##
oak-run-commons/src/test/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/pipelined/PipelinedMergeSortTaskTest.java:
##
@@ -89,27 +94,41 @@ private PipelinedMergeSortTask.Result runTest(Compression
algorithm, Path... fil
Path sortRoot = sortFolder.getRoot().toPath();
Review Comment:
I moved the executor to the top level, to be created before all tests and
shutdown after the tests. There was a potential resource leak because I was not
calling the shutdown method. For the `MetricStatisticsProvider`. I prefer to
create it close to the code that does the testing. Almost all tests call this
`runTest` method and the tests that don't do not need needs this object. So in
this case, allocation of this helper object is closer to where they are used,
which makes the logic simpler.
##
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/importer/IndexImporter.java:
##
@@ -465,19 +475,31 @@ interface IndexImporterStepExecutor {
}
void runWithRetry(int maxRetries, IndexImportState indexImportState,
IndexImporterStepExecutor step) throws CommitFailedException, IOException {
+String indexImportPhaseName = indexImportState == null ? "null" :
indexImportState.toString();
int count = 1;
Stopwatch start = Stopwatch.createStarted();
while (count <= maxRetries) {
-LOG.info("IndexImporterStepExecutor:{}, count:{}",
indexImportState, count);
-LOG.info("[TASK:{}:START]", indexImportState);
+LOG.info("IndexImporterStepExecutor:{}, count:{}",
indexImportPhaseName, count);
+LOG.info("[TASK:{}:START]", indexImportPhaseName);
try {
step.execute();
-LOG.info("[TASK:{}:END] Metrics: {}", indexImportState,
+long durationSeconds = start.elapsed(TimeUnit.SECONDS);
+LOG.info("[TASK:{}:END] Metrics: {}", indexImportPhaseName,
MetricsFormatter.newBuilder()
Re: [PR] OAK-10519 - Export metrics related to reindexing [jackrabbit-oak]
steffenvan commented on code in PR #1182:
URL: https://github.com/apache/jackrabbit-oak/pull/1182#discussion_r1386131023
##
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/importer/IndexImporter.java:
##
@@ -465,19 +475,31 @@ interface IndexImporterStepExecutor {
}
void runWithRetry(int maxRetries, IndexImportState indexImportState,
IndexImporterStepExecutor step) throws CommitFailedException, IOException {
+String indexImportPhaseName = indexImportState == null ? "null" :
indexImportState.toString();
int count = 1;
Stopwatch start = Stopwatch.createStarted();
while (count <= maxRetries) {
-LOG.info("IndexImporterStepExecutor:{}, count:{}",
indexImportState, count);
-LOG.info("[TASK:{}:START]", indexImportState);
+LOG.info("IndexImporterStepExecutor:{}, count:{}",
indexImportPhaseName, count);
+LOG.info("[TASK:{}:START]", indexImportPhaseName);
try {
step.execute();
-LOG.info("[TASK:{}:END] Metrics: {}", indexImportState,
+long durationSeconds = start.elapsed(TimeUnit.SECONDS);
+LOG.info("[TASK:{}:END] Metrics: {}", indexImportPhaseName,
MetricsFormatter.newBuilder()
.add("duration",
FormattingUtils.formatToSeconds(start))
-.add("durationSeconds",
start.elapsed(TimeUnit.SECONDS))
+.add("durationSeconds", durationSeconds)
.build()
);
+
+String name = "oak_indexer_import_" +
indexImportPhaseName.toLowerCase() + "_duration_seconds";
+CounterStats metric = statisticsProvider.getCounterStats(name,
StatsOptions.METRICS_ONLY);
+LOG.debug("Adding metric: {} {}", name, durationSeconds);
Review Comment:
In `MetricUtils.java` I see the same logic of resetting the counter if it's
not 0. Could be refactored to one function.
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10519 - Export metrics related to reindexing [jackrabbit-oak]
steffenvan commented on code in PR #1182:
URL: https://github.com/apache/jackrabbit-oak/pull/1182#discussion_r1386131023
##
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/importer/IndexImporter.java:
##
@@ -465,19 +475,31 @@ interface IndexImporterStepExecutor {
}
void runWithRetry(int maxRetries, IndexImportState indexImportState,
IndexImporterStepExecutor step) throws CommitFailedException, IOException {
+String indexImportPhaseName = indexImportState == null ? "null" :
indexImportState.toString();
int count = 1;
Stopwatch start = Stopwatch.createStarted();
while (count <= maxRetries) {
-LOG.info("IndexImporterStepExecutor:{}, count:{}",
indexImportState, count);
-LOG.info("[TASK:{}:START]", indexImportState);
+LOG.info("IndexImporterStepExecutor:{}, count:{}",
indexImportPhaseName, count);
+LOG.info("[TASK:{}:START]", indexImportPhaseName);
try {
step.execute();
-LOG.info("[TASK:{}:END] Metrics: {}", indexImportState,
+long durationSeconds = start.elapsed(TimeUnit.SECONDS);
+LOG.info("[TASK:{}:END] Metrics: {}", indexImportPhaseName,
MetricsFormatter.newBuilder()
.add("duration",
FormattingUtils.formatToSeconds(start))
-.add("durationSeconds",
start.elapsed(TimeUnit.SECONDS))
+.add("durationSeconds", durationSeconds)
.build()
);
+
+String name = "oak_indexer_import_" +
indexImportPhaseName.toLowerCase() + "_duration_seconds";
+CounterStats metric = statisticsProvider.getCounterStats(name,
StatsOptions.METRICS_ONLY);
+LOG.debug("Adding metric: {} {}", name, durationSeconds);
Review Comment:
In `MetricUtils.java` I see the same logic of resetting the counter if it's
not 0 from the start. Could be refactored to one function.
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10519 - Export metrics related to reindexing [jackrabbit-oak]
steffenvan commented on code in PR #1182: URL: https://github.com/apache/jackrabbit-oak/pull/1182#discussion_r1385331690 ## oak-run-commons/src/test/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/pipelined/PipelinedMergeSortTaskTest.java: ## @@ -89,27 +94,41 @@ private PipelinedMergeSortTask.Result runTest(Compression algorithm, Path... fil Path sortRoot = sortFolder.getRoot().toPath(); Review Comment: Would it make sense to have a setup/teardown function for instantiating these objects? Like the root, `ScheduledExecutorService` and `MetricStatisticsProvider`? Then it would clearly signal that it is being instantiated for each test case. And we can ensure to deregister the statistics provider etc after each test case. -- 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: dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] OAK-10519 - Export metrics related to reindexing [jackrabbit-oak]
steffenvan commented on code in PR #1182:
URL: https://github.com/apache/jackrabbit-oak/pull/1182#discussion_r1385322575
##
oak-run-commons/src/test/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/pipelined/PipelinedMergeSortTaskTest.java:
##
@@ -89,27 +94,41 @@ private PipelinedMergeSortTask.Result runTest(Compression
algorithm, Path... fil
Path sortRoot = sortFolder.getRoot().toPath();
// +1 for the Sentinel.
ArrayBlockingQueue sortedFilesQueue = new
ArrayBlockingQueue<>(files.length + 1);
-PipelinedMergeSortTask mergeSortTask = new
PipelinedMergeSortTask(sortRoot, pathComparator, algorithm, sortedFilesQueue);
-// Enqueue all the files that are to be merged
-for (Path file : files) {
-// The intermediate files are deleted after being merged, so we
should copy them to the temporary sort root folder
-Path workDirCopy = Files.copy(file,
sortRoot.resolve(file.getFileName()));
-sortedFilesQueue.put(workDirCopy);
+ScheduledExecutorService executor =
Executors.newSingleThreadScheduledExecutor();
+try (MetricStatisticsProvider metricStatisticsProvider = new
MetricStatisticsProvider(null, executor)) {
Review Comment:
Since `null` is passed into the `MetricStatisticsProvider`, I think it would
be nice to know why that is, instead of
`ManagementFactory.getPlatformMBeanServer()`?
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10519 - Export metrics related to reindexing [jackrabbit-oak]
steffenvan commented on code in PR #1182:
URL: https://github.com/apache/jackrabbit-oak/pull/1182#discussion_r1385314065
##
oak-run/src/test/java/org/apache/jackrabbit/oak/index/DocumentStoreIndexerIT.java:
##
@@ -380,6 +387,95 @@ protected CompositeIndexer prepareIndexers(NodeStore
nodeStore, NodeBuilder buil
}
+@Test
+public void metrics() throws Exception {
+MongoConnection mongoConnection = getConnection();
+DocumentNodeStoreBuilder docBuilder = builderProvider.newBuilder()
+.setMongoDB(mongoConnection.getMongoClient(),
mongoConnection.getDBName());
+DocumentNodeStore store = docBuilder.build();
+
+Whiteboard wb = new DefaultWhiteboard();
+MongoDocumentStore ds = (MongoDocumentStore)
docBuilder.getDocumentStore();
+Registration r1 = wb.register(MongoDocumentStore.class, ds,
emptyMap());
+
+ScheduledExecutorService executor =
Executors.newSingleThreadScheduledExecutor();
+MetricStatisticsProvider metricsStatisticsProvider = new
MetricStatisticsProvider(null, executor);
+wb.register(StatisticsProvider.class, metricsStatisticsProvider,
emptyMap());
+Registration c1Registration = wb.register(MongoDatabase.class,
mongoConnection.getDatabase(), emptyMap());
+
+configureIndex(store);
+
+NodeBuilder builder = store.getRoot().builder();
+NodeBuilder appNB = newNode("app:Asset");
+createChild(appNB,
+"jcr:content",
+"jcr:content/comments",
+"jcr:content/metadata",
+"jcr:content/metadata/xmp",
+"jcr:content/renditions",
+"jcr:content/renditions/original",
+"jcr:content/renditions/original/jcr:content"
+);
+builder.child("test").setChildNode("book.jpg", appNB.getNodeState());
+store.merge(builder, EmptyHook.INSTANCE, CommitInfo.EMPTY);
+
+String checkpoint = store.checkpoint(10);
+
+//Shut down this store and restart in readOnly mode
+store.dispose();
Review Comment:
Why is it necessary to call `dispose()` twice? Perhaps the test could be
divided into 2?
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10519 - Export metrics related to reindexing [jackrabbit-oak]
steffenvan commented on code in PR #1182:
URL: https://github.com/apache/jackrabbit-oak/pull/1182#discussion_r1385310232
##
oak-run/src/test/java/org/apache/jackrabbit/oak/index/DocumentStoreIndexerIT.java:
##
@@ -380,6 +387,95 @@ protected CompositeIndexer prepareIndexers(NodeStore
nodeStore, NodeBuilder buil
}
+@Test
+public void metrics() throws Exception {
Review Comment:
From what I can see, this test has a very similar setup as the bundling()
test. Could you refactor that and create a setup function? Then it would be
easier to know what the setup is and what is being tested.
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
