Remove duplicated code from collectors Introduce `AbstractMetricsCollector` that reduces the duplicated code of submitting metrics calculations to executor but more importantly to ensure consistency in the `*metricsCollectionTime` metric calculation. Change-Id: I8f7216d461b8b48429a54b48e34f8481d1838342
diff --git a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/AbstractMetricsCollector.java b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/AbstractMetricsCollector.java new file mode 100644 index 0000000..756cc5f --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/AbstractMetricsCollector.java
@@ -0,0 +1,79 @@ +// Copyright (C) 2025 The Android Open Source Project +// +// Licensed 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 com.googlesource.gerrit.plugins.gitrepometrics.collectors; + +import com.google.common.collect.ImmutableList; +import java.util.HashMap; +import java.util.concurrent.ExecutorService; +import java.util.function.Consumer; +import org.eclipse.jgit.internal.storage.file.FileRepository; + +/** + * Common class that should be implemented by all collectors. It performs metrics collection using + * the provided executor service but more importantly it collects the <code>*metricsCollectionTime + * </code> meta metric uniformly. + */ +abstract class AbstractMetricsCollector implements MetricsCollector { + private final ExecutorService executorService; + private final GitRepoMetric collectionTime; + private final ImmutableList<GitRepoMetric> availableMetrics; + + protected AbstractMetricsCollector( + ExecutorService executorService, + String collectorPrefix, + ImmutableList<GitRepoMetric> collectorMetrics) { + this.executorService = executorService; + this.collectionTime = + new GitRepoMetric( + String.format("%sMetricsCollectionTime", collectorPrefix), + "Timestamp at which metrics were collected", + "Milliseconds"); + this.availableMetrics = + ImmutableList.<GitRepoMetric>builder().addAll(collectorMetrics).add(collectionTime).build(); + } + + @Override + public void collect( + FileRepository repository, + String projectName, + Consumer<HashMap<GitRepoMetric, Long>> populateMetrics) { + executorService.submit( + () -> { + long collectionStartTime = System.currentTimeMillis(); + HashMap<GitRepoMetric, Long> computed = computeMetrics(repository, projectName); + if (computed != null && !computed.isEmpty()) { + computed.put(collectionTime, collectionStartTime); + populateMetrics.accept(computed); + } + }); + } + + @Override + public final ImmutableList<GitRepoMetric> availableMetrics() { + return availableMetrics; + } + + /** + * Metrics computation should be performed in this function implementation as it is called from + * the {@link AbstractMetricsCollector#collect(FileRepository, String, Consumer)} function. + * + * @param repository + * @param projectName + * @return computed metrics values - <code>null</code> or empty value means that collection failed + * and in such case <code>*metricsCollectionTime</code> will not be calculated + */ + protected abstract HashMap<GitRepoMetric, Long> computeMetrics( + FileRepository repository, String projectName); +}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/FSMetricsCollector.java b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/FSMetricsCollector.java index cf71d5c..77baf70 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/FSMetricsCollector.java +++ b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/FSMetricsCollector.java
@@ -24,13 +24,11 @@ import java.nio.file.Path; import java.util.HashMap; import java.util.Objects; -import java.util.concurrent.ExecutorService; import java.util.concurrent.ScheduledExecutorService; -import java.util.function.Consumer; import java.util.stream.Stream; import org.eclipse.jgit.internal.storage.file.FileRepository; -public class FSMetricsCollector implements MetricsCollector { +public class FSMetricsCollector extends AbstractMetricsCollector { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); static class MetricsRecord { @@ -81,43 +79,19 @@ new GitRepoMetric("numberOfDirectories", "Number of directories on filesystem", "Count"); protected static final GitRepoMetric numberOfFiles = new GitRepoMetric("numberOfFiles", "Number of files on filesystem", "Count"); - private static final GitRepoMetric collectionTime = - new GitRepoMetric( - "fsMetricsCollectionTime", "Timestamp at which metrics were collected", "Milliseconds"); private static final ImmutableList<GitRepoMetric> availableMetrics = ImmutableList.of( - numberOfKeepFiles, - numberOfEmptyDirectories, - numberOfFiles, - numberOfDirectories, - collectionTime); - - private final ExecutorService executorService; + numberOfKeepFiles, numberOfEmptyDirectories, numberOfFiles, numberOfDirectories); @Inject public FSMetricsCollector(@UpdateGitMetricsExecutor ScheduledExecutorService executorService) { - this.executorService = executorService; + super(executorService, "fs", availableMetrics); } @Override - public void collect( - FileRepository repository, - String projectName, - Consumer<HashMap<GitRepoMetric, Long>> populateMetrics) { - - executorService.submit( - () -> { - long collectionStartTime = System.currentTimeMillis(); - HashMap<GitRepoMetric, Long> metrics = filesAndDirectoriesCount(repository, projectName); - metrics.put(collectionTime, collectionStartTime); - populateMetrics.accept(metrics); - }); - } - - private HashMap<GitRepoMetric, Long> filesAndDirectoriesCount( + protected HashMap<GitRepoMetric, Long> computeMetrics( FileRepository repository, String projectName) { - try (Stream<Path> objDir = Files.walk(repository.getObjectsDirectory().toPath())) { MetricsRecord metricsRecord = objDir @@ -152,9 +126,4 @@ return new MetricsRecord().toMap(); } - - @Override - public ImmutableList<GitRepoMetric> availableMetrics() { - return availableMetrics; - } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/GitRefsMetricsCollector.java b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/GitRefsMetricsCollector.java index 1882dc2..ff74995 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/GitRefsMetricsCollector.java +++ b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/GitRefsMetricsCollector.java
@@ -26,67 +26,46 @@ import java.security.NoSuchAlgorithmException; import java.util.Comparator; import java.util.HashMap; -import java.util.concurrent.ExecutorService; import java.util.concurrent.ScheduledExecutorService; -import java.util.function.Consumer; import org.eclipse.jgit.internal.storage.file.FileRepository; import org.eclipse.jgit.lib.Ref; -public class GitRefsMetricsCollector implements MetricsCollector { +public class GitRefsMetricsCollector extends AbstractMetricsCollector { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); @VisibleForTesting protected static final GitRepoMetric combinedRefsSha1 = new GitRepoMetric("combinedRefsSha1", "Numeric value of combined refs SHA-1's", "Number"); - private static final GitRepoMetric collectionTime = - new GitRepoMetric( - "refsMetricsCollectionTime", "Timestamp at which metrics were collected", "Milliseconds"); - private static final ImmutableList<GitRepoMetric> availableMetrics = - ImmutableList.of(combinedRefsSha1, collectionTime); - - private final ExecutorService executorService; + ImmutableList.of(combinedRefsSha1); @Inject GitRefsMetricsCollector(@UpdateGitMetricsExecutor ScheduledExecutorService executorService) { - this.executorService = executorService; + super(executorService, "refs", availableMetrics); } @Override - public void collect( - FileRepository repository, - String projectName, - Consumer<HashMap<GitRepoMetric, Long>> populateMetrics) { - executorService.submit( - () -> { - try { - long collectionStartTime = System.currentTimeMillis(); - MessageDigest md = MessageDigest.getInstance("SHA-1"); - repository.getRefDatabase().getRefs().stream() - .filter(ref -> !ref.isSymbolic()) - .sorted(Comparator.comparing(Ref::getName)) - .forEach(ref -> md.update(ref.getObjectId().toString().getBytes(UTF_8))); - int sha1Int = truncateHashToInt(md.digest()); + protected HashMap<GitRepoMetric, Long> computeMetrics( + FileRepository repository, String projectName) { + HashMap<GitRepoMetric, Long> metrics = new HashMap<>(); + try { + MessageDigest md = MessageDigest.getInstance("SHA-1"); + repository.getRefDatabase().getRefs().stream() + .filter(ref -> !ref.isSymbolic()) + .sorted(Comparator.comparing(Ref::getName)) + .forEach(ref -> md.update(ref.getObjectId().toString().getBytes(UTF_8))); + int sha1Int = truncateHashToInt(md.digest()); - HashMap<GitRepoMetric, Long> metrics = new HashMap<>(); - metrics.put(combinedRefsSha1, (long) sha1Int); - metrics.put(collectionTime, collectionStartTime); - populateMetrics.accept(metrics); - } catch (NoSuchAlgorithmException e) { - logger.atSevere().withCause(e).log( - "Could not obtain SHA-1 implementation will not compute the combinedRefsSha1" - + " metric"); - } catch (IOException e) { - logger.atSevere().withCause(e).log( - "Computing combinedRefsSha1 failed. Will retry next time"); - } - }); - } - - @Override - public ImmutableList<GitRepoMetric> availableMetrics() { - return availableMetrics; + metrics.put(combinedRefsSha1, (long) sha1Int); + } catch (NoSuchAlgorithmException e) { + logger.atSevere().withCause(e).log( + "Could not obtain SHA-1 implementation will not compute the combinedRefsSha1" + + " metric"); + } catch (IOException e) { + logger.atSevere().withCause(e).log("Computing combinedRefsSha1 failed. Will retry next time"); + } + return metrics; } // Source
diff --git a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/GitStatsMetricsCollector.java b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/GitStatsMetricsCollector.java index 5f61c26..edb063a 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/GitStatsMetricsCollector.java +++ b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/GitStatsMetricsCollector.java
@@ -20,13 +20,11 @@ import com.googlesource.gerrit.plugins.gitrepometrics.UpdateGitMetricsExecutor; import java.io.IOException; import java.util.HashMap; -import java.util.concurrent.ExecutorService; import java.util.concurrent.ScheduledExecutorService; -import java.util.function.Consumer; import org.eclipse.jgit.internal.storage.file.FileRepository; import org.eclipse.jgit.internal.storage.file.GC; -public class GitStatsMetricsCollector implements MetricsCollector { +public class GitStatsMetricsCollector extends AbstractMetricsCollector { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); public static final GitRepoMetric numberOfPackedObjects = @@ -56,9 +54,6 @@ "numberOfPackFilesSinceBitmap", "The number of pack files that were created after the last bitmap generation", "Count"); - private static final GitRepoMetric collectionTime = - new GitRepoMetric( - "gitMetricsCollectionTime", "Timestamp at which metrics were collected", "Milliseconds"); private static final ImmutableList<GitRepoMetric> availableMetrics = ImmutableList.of( @@ -71,51 +66,35 @@ sizeOfPackedObjects, numberOfBitmaps, numberOfObjectsSinceBitmap, - numberOfPackFilesSinceBitmap, - collectionTime); - - private final ExecutorService executorService; + numberOfPackFilesSinceBitmap); @Inject public GitStatsMetricsCollector( @UpdateGitMetricsExecutor ScheduledExecutorService executorService) { - this.executorService = executorService; + super(executorService, "git", availableMetrics); } @Override - public void collect( - FileRepository repository, - String projectName, - Consumer<HashMap<GitRepoMetric, Long>> populateMetrics) { + protected HashMap<GitRepoMetric, Long> computeMetrics( + FileRepository repository, String projectName) { + HashMap<GitRepoMetric, Long> metrics = new HashMap<>(); + try { + GC.RepoStatistics statistics = new GC(repository).getStatistics(); + metrics.put(numberOfPackedObjects, statistics.numberOfPackedObjects); + metrics.put(numberOfPackFiles, statistics.numberOfPackFiles); + metrics.put(numberOfLooseObjects, statistics.numberOfLooseObjects); + metrics.put(numberOfLooseRefs, statistics.numberOfLooseRefs); + metrics.put(numberOfPackedRefs, statistics.numberOfPackedRefs); + metrics.put(sizeOfLooseObjects, statistics.sizeOfLooseObjects); + metrics.put(sizeOfPackedObjects, statistics.sizeOfPackedObjects); + metrics.put(numberOfBitmaps, statistics.numberOfBitmaps); + metrics.put(numberOfObjectsSinceBitmap, statistics.numberOfObjectsSinceBitmap); + metrics.put(numberOfPackFilesSinceBitmap, statistics.numberOfPackFilesSinceBitmap); + logger.atFine().log("New Git Statistics metrics collected: %s", statistics.toString()); + } catch (IOException e) { + logger.atSevere().log("Something went wrong: %s", e.getMessage()); + } - executorService.submit( - () -> { - HashMap<GitRepoMetric, Long> metrics = new HashMap<>(); - try { - long collectionStartTime = System.currentTimeMillis(); - GC.RepoStatistics statistics = new GC(repository).getStatistics(); - metrics.put(numberOfPackedObjects, statistics.numberOfPackedObjects); - metrics.put(numberOfPackFiles, statistics.numberOfPackFiles); - metrics.put(numberOfLooseObjects, statistics.numberOfLooseObjects); - metrics.put(numberOfLooseRefs, statistics.numberOfLooseRefs); - metrics.put(numberOfPackedRefs, statistics.numberOfPackedRefs); - metrics.put(sizeOfLooseObjects, statistics.sizeOfLooseObjects); - metrics.put(sizeOfPackedObjects, statistics.sizeOfPackedObjects); - metrics.put(numberOfBitmaps, statistics.numberOfBitmaps); - metrics.put(numberOfObjectsSinceBitmap, statistics.numberOfObjectsSinceBitmap); - metrics.put(numberOfPackFilesSinceBitmap, statistics.numberOfPackFilesSinceBitmap); - metrics.put(collectionTime, collectionStartTime); - logger.atFine().log("New Git Statistics metrics collected: %s", statistics.toString()); - } catch (IOException e) { - logger.atSevere().log("Something went wrong: %s", e.getMessage()); - } - - populateMetrics.accept(metrics); - }); - } - - @Override - public ImmutableList<GitRepoMetric> availableMetrics() { - return availableMetrics; + return metrics; } }