Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Record metrics when INIT revision or an old revision is used #874

Merged
merged 10 commits into from
Sep 4, 2023
Merged

Record metrics when INIT revision or an old revision is used #874

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4234a49
Record metrics when INIT revision or an old revision is used
minwoox Aug 28, 2023
a302f38
Address comments from @jrhee17
minwoox Aug 28, 2023
37e6a85
Update tag
minwoox Aug 28, 2023
82fa47c
Add projectName to mirroring metrics
minwoox Aug 29, 2023
2b5fd93
lint
minwoox Aug 29, 2023
a89d781
Fix tests
minwoox Aug 29, 2023
24ef8be
Fix again
minwoox Aug 29, 2023
f295224
Add dontnote
minwoox Aug 29, 2023
d1d36a2
Address comments from @trustin
minwoox Aug 29, 2023
dce6327
Address comments from @ikhoon
minwoox Sep 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -114,11 +115,13 @@ public ContentServiceV1(ProjectManager projectManager, CommandExecutor executor,
* <p>Returns the list of files in the path.
*/
@Get("regex:/projects/(?<projectName>[^/]+)/repos/(?<repoName>[^/]+)/list(?<path>(|/.*))$")
public CompletableFuture<List<EntryDto<?>>> listFiles(@Param String path,
public CompletableFuture<List<EntryDto<?>>> 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<List<EntryDto<?>>> future = new CompletableFuture<>();
listFiles(repository, normalizedPath, normalizedRev, false, future);
return future;
Expand Down Expand Up @@ -214,12 +217,14 @@ private CompletableFuture<Revision> push(long commitTimeMills, Author author, Re
*/
@Post("/projects/{projectName}/repos/{repoName}/preview")
public CompletableFuture<Iterable<ChangeDto<?>>> preview(
ServiceRequestContext ctx,
@Param @Default("-1") String revision,
Repository repository,
@RequestConverter(ChangesRequestConverter.class) Iterable<Change<?>> changes) {

final Revision baseRevision = new Revision(revision);
increaseCounterIfOldRevisionUsed(ctx, repository, baseRevision);
final CompletableFuture<Map<String, Change<?>>> changesFuture =
repository.previewDiff(new Revision(revision), changes);
repository.previewDiff(baseRevision, changes);

return changesFuture.thenApply(previewDiffs -> previewDiffs.values().stream()
.map(DtoConverter::convert)
Expand All @@ -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
Expand Down Expand Up @@ -325,7 +331,8 @@ private static Object handleWatchFailure(Throwable thrown) {
* specify {@code to}, this will return the list of commits.
*/
@Get("regex:/projects/(?<projectName>[^/]+)/repos/(?<repoName>[^/]+)/commits(?<revision>(|/.*))$")
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,
Expand All @@ -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)
Expand All @@ -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()));
}
Expand All @@ -402,9 +417,12 @@ private static <T> Object objectOrList(Collection<T> collection, boolean toList,
*/
@Get("/projects/{projectName}/repos/{repoName}/merge")
public <T> CompletableFuture<MergedEntryDto<T>> mergeFiles(
ServiceRequestContext ctx,
@Param @Default("-1") String revision, Repository repository,
@RequestConverter(MergeQueryRequestConverter.class) MergeQuery<T> 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);
}

/**
Expand Down
40 changes: 39 additions & 1 deletion server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -186,8 +191,41 @@ public CompletableFuture<RepositoryDto> patchRepository(@Param String repoName,
*/
@Get("/projects/{projectName}/repos/{repoName}/revision/{revision}")
@RequiresReadPermission
public Map<String, Integer> normalizeRevision(Repository repository, @Param String revision) {
public Map<String, Integer> 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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It may be simpler.

Suggested change
ServiceRequestContext ctx, String projectName, String repoName,
ServiceRequestContext ctx, Repository repository,

Revision normalized, Revision head) {
if (normalized.major() == 1 || head.major() - normalized.major() >= 5000) {
ctx.log().whenComplete().thenAccept(log -> {
Copy link
Contributor

@jrhee17 jrhee17 Aug 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question) Do we want to differentiate between cases using 1 vs. >5000?
Imagining that the counter goes up, I wonder if we want to know which case needs handling.

Nevermind, I realized you added the revision as a tag

ctx.meterRegistry().counter("dogma.old.revision",
generateTags(projectName, repoName, log, normalized, head))
.increment();
});
}
}

private static List<Tag> 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())),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding a tag which can increase cardinality, what do you think of just adding a distribution?

i.e. If there is a repo with head version 1M, this is the possible cardinality worse case.

ctx.meterRegistry().summary("").record(head.major() - normalized.major());
ctx.meterRegistry().counter("", "init.major", String.valueOf(normalized.major() == 1)).increment();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the metric with the assumption that this will be short-lived and there will not be many requests that are caught by the condition.
However, there's no reason not to decrease the cardinality so let me use the distribution. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the metric with the assumption that this will be short-lived and there will not be many requests that are caught by the condition.

I agree, but I also thought this code will also need to go to production env. to confirm our hypothesis 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct. 🤣

Tag.of("head", Integer.toString(head.major())));
}
}
48 changes: 37 additions & 11 deletions ...c/main/java/com/linecorp/centraldogma/server/internal/thrift/CentralDogmaServiceImpl.java
View file
Open in desktop
minwoox marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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 -> {
Expand All @@ -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 -> {
Expand All @@ -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()
Expand All @@ -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)),
Expand All @@ -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<Change> 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)),
Expand Down Expand Up @@ -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())),
Expand All @@ -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))
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
Loading