Skip to content

Commit

Permalink
Report some failures at INFO level only
Browse files Browse the repository at this point in the history
And don't report them on GitHub if there is nothing more serious,
to create less noise with GitHub notifications.
  • Loading branch information
yrodiere committed Nov 8, 2024
1 parent 43050d7 commit 44eb21e
Show file tree
Hide file tree
Showing 6 changed files with 49 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@ enum Level {
* Level for failures that don't prevent reindexing but cause the index to be incomplete/incorrect,
* and may require maintainer's attention.
*/
WARNING(Logger.Level.WARN);
WARNING(Logger.Level.WARN),
/**
* Level for failures that don't prevent reindexing but cause the index to be incomplete/incorrect,
* though maintainer's attention is not needed (the solution is just waiting).
*/
INFO(Logger.Level.INFO);

public final Logger.Level logLevel;

Expand All @@ -34,6 +39,14 @@ default void collect(Level level, Stage stage, String details) {

void collect(Level level, Stage stage, String details, Exception exception);

default void info(Stage stage, String details) {
collect(Level.INFO, stage, details);
}

default void info(Stage stage, String details, Exception exception) {
collect(Level.INFO, stage, details, exception);
}

default void warning(Stage stage, String details) {
collect(Level.WARNING, stage, details);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ public void report(Status status, Map<FailureCollector.Level, List<Failure>> fai
switch (status) {
case IN_PROGRESS, SUCCESS -> {
// When in progress or successful, never comment: we want to avoid unnecessary noise.
// Note this means INFO level failures will have been logged by the FailureCollector,
// but won't be reported to GitHub.
}
case WARNING -> {
// When warning, only comment if we didn't comment the same thing recently.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Function;

import io.quarkus.logging.Log;
import io.quarkus.search.app.indexing.reporting.Failure;
import io.quarkus.search.app.indexing.reporting.FailureCollector;
import io.quarkus.search.app.indexing.reporting.Status;
import io.quarkus.search.app.indexing.reporting.StatusReporter;

import io.quarkus.logging.Log;

import io.smallrye.mutiny.subscription.Cancellable;

public class IndexingState {
Expand Down Expand Up @@ -105,7 +107,7 @@ private boolean scheduleRetry() {
try {
scheduledRetry = retryScheduler.apply(retryConfig.delay());
// If we get here, a retry was scheduled.
warning(Stage.INDEXING, "Indexing will be tried again later.");
info(Stage.INDEXING, "Indexing will be tried again later.");
return true;
} catch (RuntimeException e) {
// If we get here, we'll abort.
Expand All @@ -120,6 +122,8 @@ private boolean scheduleRetry() {
}

private static Status indexingResultStatus(Map<Level, List<Failure>> failures) {
// INFO level is ignored for the overall status.
// We will only report INFO failures if there are other CRITICAL/WARNING failures.
if (failures.get(Level.CRITICAL).isEmpty()) {
if (failures.get(Level.WARNING).isEmpty()) {
return Status.SUCCESS;
Expand Down
26 changes: 16 additions & 10 deletions src/main/java/io/quarkus/search/app/quarkusio/QuarkusIO.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import java.io.InputStream;
import java.net.URI;
import java.net.URISyntaxException;
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.time.Duration;
import java.time.Instant;
Expand Down Expand Up @@ -137,7 +138,7 @@ private static void validateRepositories(
// more than 7 days; so we give it 2 weeks period before we start considering that there's a sync problem:
Duration duration = Duration.between(lastSync, Instant.now());
if (duration.compareTo(Duration.ofDays(14)) > 0) {
failureCollector.warning(
failureCollector.info(
FailureCollector.Stage.TRANSLATION,
"Localized repository '" + localized.getKey() + "' is out of sync for " + duration);
}
Expand All @@ -153,7 +154,7 @@ private static void validateRepositories(
@Override
public void close() throws IOException {
for (QuarkusIOCloneDirectory directory : allSites.values()) {
directory.unprocessed().ifPresent(m -> failureCollector.warning(FailureCollector.Stage.PARSING, m));
directory.unprocessed().ifPresent(m -> failureCollector.info(FailureCollector.Stage.PARSING, m));
}
try (var closer = new Closer<IOException>()) {
closer.push(CloseableDirectory::close, prefetchedGuides);
Expand Down Expand Up @@ -447,12 +448,17 @@ private Catalog translations(Repository repository, RevTree sources, String path

try (InputStream file = GitUtils.file(repository, sources, path)) {
return new PoParser().parseCatalog(file, false);
} catch (IOException | IllegalStateException e) {
// it may be that not all localized sites are up-to-date, in that case we just assume that the translation is not there
// and the non-translated english text will be used.
} catch (NoSuchFileException e) {
// translation may be missing, but that's just the way it is: it'll get translated when someone has time
failureCollector.info(FailureCollector.Stage.TRANSLATION,
"Unable to open translation file " + path + " : " + e.getMessage(), e);
// in the meantime we'll use non-translated English text
return new Catalog();
} catch (IOException e) {
// opening/parsing may fail, in which case we definitely need to have a look
failureCollector.warning(FailureCollector.Stage.TRANSLATION,
"Unable to parse a translation file " + path + " : " + e.getMessage(), e);

"Unable to parse translation file " + path + " : " + e.getMessage(), e);
// in the meantime we'll use non-translated English text
return new Catalog();
}
}
Expand Down Expand Up @@ -555,9 +561,9 @@ private Guide createCoreGuide(GitCloneDirectory cloneDirectory, String quarkusVe
// if a file is not present we do not want to add such guide. Since if the html is not there
// it means that users won't be able to open it on the site, and returning it in the search results make it pointless.

// Since this file was listed in the yaml of a rev from which the site was built the html should've been present,
// but is missing for some reason that needs investigation:
failureCollector.warning(
// Since this file was listed in the yaml of a rev from which the site was built, the html should've been present.
// So if it's missing, that's important context when investigating other errors.
failureCollector.info(
FailureCollector.Stage.TRANSLATION,
"Guide " + guide + " is ignored since we were not able to find an HTML content file for it.");
return null;
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/io/quarkus/search/app/util/GitUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.nio.file.NoSuchFileException;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -59,7 +60,7 @@ public static InputStream file(Repository repo, RevTree tree, String path) throw
treeWalk.setRecursive(true);
treeWalk.setFilter(PathFilter.create(path));
if (!treeWalk.next()) {
throw new IllegalStateException("Missing file '%s' in '%s'".formatted(path, tree));
throw new NoSuchFileException("Missing file '%s' in '%s'".formatted(path, tree));
}
ObjectId objectId = treeWalk.getObjectId(0);
ObjectLoader loader = repo.open(objectId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ void success() {
assertThat(state.isInProgress()).isTrue();
verify(statusReporterMock).report(eq(Status.IN_PROGRESS), eq(Map.of()));

// info should not affect the status
attempt.info(Stage.INDEXING, "Some info");

// No warning/critical: success
}
verify(statusReporterMock).report(eq(Status.SUCCESS), anyMap());
Expand Down Expand Up @@ -80,6 +83,9 @@ void warning() {
verify(statusReporterMock).report(eq(Status.IN_PROGRESS), eq(Map.of()));

attempt.warning(Stage.INDEXING, "Some warning");

// info should not affect the status
attempt.info(Stage.INDEXING, "Some info");
}
verify(statusReporterMock).report(eq(Status.WARNING), anyMap());
assertThat(state.isInProgress()).isFalse();
Expand All @@ -96,6 +102,9 @@ void failure_max1Attempt() {
verify(statusReporterMock).report(eq(Status.IN_PROGRESS), eq(Map.of()));

attempt.critical(Stage.INDEXING, "Something critical");

// info should not affect the status
attempt.info(Stage.INDEXING, "Some info");
}
verify(statusReporterMock).report(eq(Status.CRITICAL), anyMap());
assertThat(state.isInProgress()).isFalse();
Expand Down

0 comments on commit 44eb21e

Please sign in to comment.