From 4234a49aec6f527ae976b9d7ddad4df540151ec5 Mon Sep 17 00:00:00 2001 From: minwoox Date: Mon, 28 Aug 2023 17:51:39 +0900 Subject: [PATCH] Record metrics when INIT revision or an old revision is used Motivation: Before we apply https://github.com/line/centraldogma/pull/681 that removes old commits, it would be nice if we can find any usages that access data with the INIT Revision or revisions that are more than 5000 (default minimum number of commit retentions) commits ahead from the head. Modifications: - Add a counter that is incresed when INIT Revision or revisions that are more than 5000 ahead from the head are used. Result: - Track the number of usages. - This commit will be reverted after we find the usage and fix it. --- .../server/internal/api/ContentServiceV1.java | 34 +++++++++---- .../internal/api/RepositoryServiceV1.java | 40 +++++++++++++++- .../thrift/CentralDogmaServiceImpl.java | 48 ++++++++++++++----- .../centraldogma/server/MetricsTest.java | 28 +++++++---- 4 files changed, 120 insertions(+), 30 deletions(-) diff --git a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java index 757194d008..3e8c04fac6 100644 --- a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java +++ b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java @@ -24,6 +24,7 @@ import static com.linecorp.centraldogma.internal.Util.isValidFilePath; import static com.linecorp.centraldogma.server.internal.api.DtoConverter.convert; import static com.linecorp.centraldogma.server.internal.api.HttpApiUtil.returnOrThrow; +import static com.linecorp.centraldogma.server.internal.api.RepositoryServiceV1.increaseCounterIfOldRevisionUsed; import static com.linecorp.centraldogma.server.internal.storage.repository.DefaultMetaRepository.metaRepoFiles; import static java.util.Objects.requireNonNull; @@ -114,11 +115,13 @@ public ContentServiceV1(ProjectManager projectManager, CommandExecutor executor, *

Returns the list of files in the path. */ @Get("regex:/projects/(?[^/]+)/repos/(?[^/]+)/list(?(|/.*))$") - public CompletableFuture>> listFiles(@Param String path, + public CompletableFuture>> listFiles(ServiceRequestContext ctx, + @Param String path, @Param @Default("-1") String revision, Repository repository) { final String normalizedPath = normalizePath(path); final Revision normalizedRev = repository.normalizeNow(new Revision(revision)); + increaseCounterIfOldRevisionUsed(ctx, repository, normalizedRev); final CompletableFuture>> future = new CompletableFuture<>(); listFiles(repository, normalizedPath, normalizedRev, false, future); return future; @@ -214,12 +217,14 @@ private CompletableFuture push(long commitTimeMills, Author author, Re */ @Post("/projects/{projectName}/repos/{repoName}/preview") public CompletableFuture>> preview( + ServiceRequestContext ctx, @Param @Default("-1") String revision, Repository repository, @RequestConverter(ChangesRequestConverter.class) Iterable> changes) { - + final Revision baseRevision = new Revision(revision); + increaseCounterIfOldRevisionUsed(ctx, repository, baseRevision); final CompletableFuture>> changesFuture = - repository.previewDiff(new Revision(revision), changes); + repository.previewDiff(baseRevision, changes); return changesFuture.thenApply(previewDiffs -> previewDiffs.values().stream() .map(DtoConverter::convert) @@ -245,6 +250,7 @@ public CompletableFuture getFiles( Repository repository, @RequestConverter(WatchRequestConverter.class) @Nullable WatchRequest watchRequest, @RequestConverter(QueryRequestConverter.class) @Nullable Query query) { + increaseCounterIfOldRevisionUsed(ctx, repository, new Revision(revision)); final String normalizedPath = normalizePath(path); // watch repository or a file @@ -325,7 +331,8 @@ private static Object handleWatchFailure(Throwable thrown) { * specify {@code to}, this will return the list of commits. */ @Get("regex:/projects/(?[^/]+)/repos/(?[^/]+)/commits(?(|/.*))$") - public CompletableFuture listCommits(@Param String revision, + public CompletableFuture listCommits(ServiceRequestContext ctx, + @Param String revision, @Param @Default("/**") String path, @Param @Nullable String to, @Param @Nullable Integer maxCommits, @@ -346,6 +353,10 @@ public CompletableFuture listCommits(@Param String revision, } final RevisionRange range = repository.normalizeNow(fromRevision, toRevision).toDescending(); + + increaseCounterIfOldRevisionUsed(ctx, repository, range.from()); + increaseCounterIfOldRevisionUsed(ctx, repository, range.to()); + final int maxCommits0 = firstNonNull(maxCommits, Repository.DEFAULT_MAX_COMMITS); return repository .history(range.from(), range.to(), normalizePath(path), maxCommits0) @@ -368,17 +379,21 @@ public CompletableFuture listCommits(@Param String revision, */ @Get("/projects/{projectName}/repos/{repoName}/compare") public CompletableFuture getDiff( + ServiceRequestContext ctx, @Param @Default("/**") String pathPattern, @Param @Default("1") String from, @Param @Default("head") String to, Repository repository, @RequestConverter(QueryRequestConverter.class) @Nullable Query query) { - + final Revision fromRevision = new Revision(from); + final Revision toRevision = new Revision(to); + increaseCounterIfOldRevisionUsed(ctx, repository, fromRevision); + increaseCounterIfOldRevisionUsed(ctx, repository, toRevision); if (query != null) { - return repository.diff(new Revision(from), new Revision(to), query) + return repository.diff(fromRevision, toRevision, query) .thenApply(DtoConverter::convert); } else { return repository - .diff(new Revision(from), new Revision(to), normalizePath(pathPattern)) + .diff(fromRevision, toRevision, normalizePath(pathPattern)) .thenApply(changeMap -> changeMap.values().stream() .map(DtoConverter::convert).collect(toImmutableList())); } @@ -402,9 +417,12 @@ private static Object objectOrList(Collection collection, boolean toList, */ @Get("/projects/{projectName}/repos/{repoName}/merge") public CompletableFuture> mergeFiles( + ServiceRequestContext ctx, @Param @Default("-1") String revision, Repository repository, @RequestConverter(MergeQueryRequestConverter.class) MergeQuery query) { - return repository.mergeFiles(new Revision(revision), query).thenApply(DtoConverter::convert); + final Revision rev = new Revision(revision); + increaseCounterIfOldRevisionUsed(ctx, repository, rev); + return repository.mergeFiles(rev, query).thenApply(DtoConverter::convert); } /** diff --git a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java index 655d7ea992..99533bd2c4 100644 --- a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java +++ b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java @@ -16,6 +16,7 @@ package com.linecorp.centraldogma.server.internal.api; +import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.linecorp.centraldogma.server.internal.api.HttpApiUtil.checkUnremoveArgument; import static com.linecorp.centraldogma.server.internal.api.HttpApiUtil.returnOrThrow; @@ -28,9 +29,11 @@ import javax.annotation.Nullable; import com.fasterxml.jackson.databind.JsonNode; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.common.logging.RequestLog; import com.linecorp.armeria.server.ServiceRequestContext; import com.linecorp.armeria.server.annotation.Consumes; import com.linecorp.armeria.server.annotation.Delete; @@ -58,6 +61,8 @@ import com.linecorp.centraldogma.server.storage.project.ProjectManager; import com.linecorp.centraldogma.server.storage.repository.Repository; +import io.micrometer.core.instrument.Tag; + /** * Annotated service object for managing repositories. */ @@ -186,8 +191,41 @@ public CompletableFuture patchRepository(@Param String repoName, */ @Get("/projects/{projectName}/repos/{repoName}/revision/{revision}") @RequiresReadPermission - public Map normalizeRevision(Repository repository, @Param String revision) { + public Map normalizeRevision(ServiceRequestContext ctx, + Repository repository, @Param String revision) { final Revision normalizedRevision = repository.normalizeNow(new Revision(revision)); + final Revision head = repository.normalizeNow(Revision.HEAD); + increaseCounterIfOldRevisionUsed(ctx, repository.parent().name(), repository.name(), + normalizedRevision, head); return ImmutableMap.of("revision", normalizedRevision.major()); } + + static void increaseCounterIfOldRevisionUsed(ServiceRequestContext ctx, Repository repository, + Revision revision) { + final Revision normalized = repository.normalizeNow(revision); + final Revision head = repository.normalizeNow(Revision.HEAD); + increaseCounterIfOldRevisionUsed(ctx, repository.parent().name(), repository.name(), normalized, head); + } + + public static void increaseCounterIfOldRevisionUsed( + ServiceRequestContext ctx, String projectName, String repoName, + Revision normalized, Revision head) { + if (normalized.major() == 1 || head.major() - normalized.major() >= 5000) { + ctx.log().whenComplete().thenAccept(log -> { + ctx.meterRegistry().counter("dogma.old.revision", + generateTags(projectName, repoName, log, normalized, head)) + .increment(); + }); + } + } + + private static List generateTags(String projectName, String repoName, + RequestLog log, Revision normalized, Revision head) { + return ImmutableList.of(Tag.of("project", projectName), + Tag.of("repo", repoName), + Tag.of("service", firstNonNull(log.serviceName(), "none")), + Tag.of("method", log.name()), + Tag.of("normalized", Integer.toString(normalized.major())), + Tag.of("head", Integer.toString(head.major()))); + } } diff --git a/server/src/main/java/com/linecorp/centraldogma/server/internal/thrift/CentralDogmaServiceImpl.java b/server/src/main/java/com/linecorp/centraldogma/server/internal/thrift/CentralDogmaServiceImpl.java index 8265629c01..8faf4b3d6e 100644 --- a/server/src/main/java/com/linecorp/centraldogma/server/internal/thrift/CentralDogmaServiceImpl.java +++ b/server/src/main/java/com/linecorp/centraldogma/server/internal/thrift/CentralDogmaServiceImpl.java @@ -17,7 +17,9 @@ import static com.google.common.base.MoreObjects.firstNonNull; import static com.linecorp.centraldogma.common.Author.SYSTEM; +import static com.linecorp.centraldogma.common.Revision.HEAD; import static com.linecorp.centraldogma.server.internal.api.ContentServiceV1.checkPush; +import static com.linecorp.centraldogma.server.internal.api.RepositoryServiceV1.increaseCounterIfOldRevisionUsed; import static com.linecorp.centraldogma.server.internal.thrift.Converter.convert; import static com.linecorp.centraldogma.server.storage.project.Project.isReservedRepoName; import static com.linecorp.centraldogma.server.storage.repository.FindOptions.FIND_ALL_WITHOUT_CONTENT; @@ -36,6 +38,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.linecorp.armeria.common.RequestContext; import com.linecorp.armeria.common.util.Exceptions; import com.linecorp.centraldogma.internal.thrift.Author; import com.linecorp.centraldogma.internal.thrift.CentralDogmaConstants; @@ -218,15 +221,27 @@ public void normalizeRevision(String projectName, String repositoryName, Revisio AsyncMethodCallback resultHandler) { final com.linecorp.centraldogma.common.Revision normalized = - projectManager.get(projectName).repos().get(repositoryName) - .normalizeNow(convert(revision)); + normalizeRevision(projectName, repositoryName, revision); + resultHandler.onComplete(convert(normalized)); } + private com.linecorp.centraldogma.common.Revision normalizeRevision( + String projectName, String repositoryName, Revision revision) { + final Repository repository = projectManager.get(projectName).repos().get(repositoryName); + final com.linecorp.centraldogma.common.Revision normalized = + repository.normalizeNow(convert(revision)); + final com.linecorp.centraldogma.common.Revision head = repository.normalizeNow(HEAD); + increaseCounterIfOldRevisionUsed(RequestContext.current(), projectName, repositoryName, + normalized, head); + return normalized; + } + @Override public void listFiles(String projectName, String repositoryName, Revision revision, String pathPattern, AsyncMethodCallback resultHandler) { - + // Call normalizeRevision() first to check if the specified revision needs to be recorded. + normalizeRevision(projectName, repositoryName, revision); handle(projectManager.get(projectName).repos().get(repositoryName) .find(convert(revision), pathPattern, FIND_ALL_WITHOUT_CONTENT) .thenApply(entries -> { @@ -241,7 +256,8 @@ public void listFiles(String projectName, String repositoryName, Revision revisi @Override public void getFiles(String projectName, String repositoryName, Revision revision, String pathPattern, AsyncMethodCallback resultHandler) { - + // Call normalizeRevision() first to check if the specified revision needs to be recorded. + normalizeRevision(projectName, repositoryName, revision); handle(projectManager.get(projectName).repos().get(repositoryName) .find(convert(revision), pathPattern) .thenApply(entries -> { @@ -257,7 +273,9 @@ public void getFiles(String projectName, String repositoryName, Revision revisio @Override public void getHistory(String projectName, String repositoryName, Revision from, Revision to, String pathPattern, AsyncMethodCallback resultHandler) { - + // Call normalizeRevision() first to check if the specified revision needs to be recorded. + normalizeRevision(projectName, repositoryName, from); + normalizeRevision(projectName, repositoryName, to); handle(projectManager.get(projectName).repos().get(repositoryName) .history(convert(from), convert(to), pathPattern) .thenApply(commits -> commits.stream() @@ -269,7 +287,9 @@ public void getHistory(String projectName, String repositoryName, Revision from, @Override public void getDiffs(String projectName, String repositoryName, Revision from, Revision to, String pathPattern, AsyncMethodCallback resultHandler) { - + // Call normalizeRevision() first to check if the specified revision needs to be recorded. + normalizeRevision(projectName, repositoryName, from); + normalizeRevision(projectName, repositoryName, to); handle(projectManager.get(projectName).repos().get(repositoryName) .diff(convert(from), convert(to), pathPattern) .thenApply(diffs -> convert(diffs.values(), Converter::convert)), @@ -279,7 +299,8 @@ public void getDiffs(String projectName, String repositoryName, Revision from, R @Override public void getPreviewDiffs(String projectName, String repositoryName, Revision baseRevision, List changes, AsyncMethodCallback resultHandler) { - + // Call normalizeRevision() first to check if the specified revision needs to be recorded. + normalizeRevision(projectName, repositoryName, baseRevision); handle(projectManager.get(projectName).repos().get(repositoryName) .previewDiff(convert(baseRevision), convert(changes, Converter::convert)) .thenApply(diffs -> convert(diffs.values(), Converter::convert)), @@ -313,7 +334,8 @@ public void push(String projectName, String repositoryName, Revision baseRevisio @Override public void getFile(String projectName, String repositoryName, Revision revision, Query query, AsyncMethodCallback resultHandler) { - + // Call normalizeRevision() first to check if the specified revision needs to be recorded. + normalizeRevision(projectName, repositoryName, revision); handle(projectManager.get(projectName).repos().get(repositoryName) .get(convert(revision), convert(query)) .thenApply(res -> new GetFileResult(convert(res.type()), res.contentAsText())), @@ -323,7 +345,9 @@ public void getFile(String projectName, String repositoryName, Revision revision @Override public void diffFile(String projectName, String repositoryName, Revision from, Revision to, Query query, AsyncMethodCallback resultHandler) { - + // Call normalizeRevision() first to check if the specified revision needs to be recorded. + normalizeRevision(projectName, repositoryName, from); + normalizeRevision(projectName, repositoryName, to); // FIXME(trustin): Remove the firstNonNull() on the change content once we make it optional. handle(projectManager.get(projectName).repos().get(repositoryName) .diff(convert(from), convert(to), convert(query)) @@ -348,7 +372,8 @@ public void mergeFiles(String projectName, String repositoryName, Revision revis public void watchRepository( String projectName, String repositoryName, Revision lastKnownRevision, String pathPattern, long timeoutMillis, AsyncMethodCallback resultHandler) { - + // Call normalizeRevision() first to check if the specified revision needs to be recorded. + normalizeRevision(projectName, repositoryName, lastKnownRevision); if (timeoutMillis <= 0) { rejectInvalidWatchTimeout("watchRepository", resultHandler); return; @@ -384,7 +409,8 @@ private static void handleWatchRepositoryResult( public void watchFile( String projectName, String repositoryName, Revision lastKnownRevision, Query query, long timeoutMillis, AsyncMethodCallback resultHandler) { - + // Call normalizeRevision() first to check if the specified revision needs to be recorded. + normalizeRevision(projectName, repositoryName, lastKnownRevision); if (timeoutMillis <= 0) { rejectInvalidWatchTimeout("watchFile", resultHandler); return; diff --git a/server/src/test/java/com/linecorp/centraldogma/server/MetricsTest.java b/server/src/test/java/com/linecorp/centraldogma/server/MetricsTest.java index 1d98fefd09..aad00c6396 100644 --- a/server/src/test/java/com/linecorp/centraldogma/server/MetricsTest.java +++ b/server/src/test/java/com/linecorp/centraldogma/server/MetricsTest.java @@ -20,12 +20,16 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import com.fasterxml.jackson.databind.JsonNode; + import com.linecorp.armeria.common.AggregatedHttpResponse; import com.linecorp.armeria.common.HttpStatus; import com.linecorp.armeria.common.MediaType; import com.linecorp.centraldogma.client.CentralDogma; +import com.linecorp.centraldogma.client.WatchRequest; import com.linecorp.centraldogma.common.Change; import com.linecorp.centraldogma.common.Query; +import com.linecorp.centraldogma.common.Revision; import com.linecorp.centraldogma.testing.junit.CentralDogmaExtension; import io.micrometer.core.instrument.MeterRegistry; @@ -55,7 +59,7 @@ void metrics() { assertThat(((CompositeMeterRegistry) meterRegistry).getRegistries()) .hasAtLeastOneElementOfType(PrometheusMeterRegistry.class); - AggregatedHttpResponse res = dogma.httpClient().get("/monitor/metrics").aggregate().join(); + final AggregatedHttpResponse res = dogma.httpClient().get("/monitor/metrics").aggregate().join(); String content = res.contentUtf8(); assertThat(res.status()).isEqualTo(HttpStatus.OK); assertThat(res.contentType()).isEqualTo(MediaType.parse(TextFormat.CONTENT_TYPE_004)); @@ -63,15 +67,19 @@ void metrics() { assertThat(content).doesNotContain( "com.linecorp.centraldogma.server.internal.api.WatchContentServiceV1"); - dogma.client() - .forRepo("foo", "bar") - .watch(Query.ofJson("/foo.json")) - .timeoutMillis(100) - .errorOnEntryNotFound(false) - .start() - .join(); - res = dogma.httpClient().get("/monitor/metrics").aggregate().join(); - content = res.contentUtf8(); + final WatchRequest jsonNodeWatchRequest = dogma.client() + .forRepo("foo", "bar") + .watch(Query.ofJson("/foo.json")) + .timeoutMillis(100) + .errorOnEntryNotFound(false); + jsonNodeWatchRequest.start().join(); + content = dogma.httpClient().get("/monitor/metrics").aggregate().join().contentUtf8(); assertThat(content).contains("com.linecorp.centraldogma.server.internal.api.WatchContentServiceV1"); + assertThat(content).doesNotContain("dogma_old_revision"); + + // Trigger old revision recording + jsonNodeWatchRequest.start(Revision.INIT).join(); + content = dogma.httpClient().get("/monitor/metrics").aggregate().join().contentUtf8(); + assertThat(content).contains("dogma_old_revision"); } }