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.

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

Report some failures at INFO level only #359

Merged
merged 3 commits into from
Nov 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,10 +1,30 @@
package io.quarkus.search.app.indexing.reporting;

import org.jboss.logging.Logger;

public interface FailureCollector {

enum Level {
CRITICAL,
WARNING;
/**
* Level for failures that prevent reindexing completely.
*/
CRITICAL(Logger.Level.ERROR),
/**
* 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),
/**
* 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;

Level(Logger.Level logLevel) {
this.logLevel = logLevel;
}
}

enum Stage {
Expand All @@ -13,12 +33,34 @@ enum Stage {
INDEXING;
}

void warning(Stage stage, String details);
default void collect(Level level, Stage stage, String details) {
collect(level, stage, details, null);
}

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);
}

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

void critical(Stage stage, String details);
default void warning(Stage stage, String details, Exception exception) {
collect(Level.WARNING, stage, details, exception);
}

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

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

}
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 @@ -89,25 +89,9 @@ public void close() {
}

@Override
public void warning(Stage stage, String details) {
warning(stage, details, null);
}

@Override
public void warning(Stage stage, String details, Exception exception) {
Log.warn(details, exception);
failures.get(Level.WARNING).add(new Failure(Level.WARNING, stage, details, exception));
}

@Override
public void critical(Stage stage, String details) {
critical(stage, details, null);
}

@Override
public void critical(Stage stage, String details, Exception exception) {
Log.error(details, exception);
failures.get(Level.CRITICAL).add(new Failure(Level.CRITICAL, stage, details, exception));
public void collect(Level level, Stage stage, String details, Exception exception) {
Log.log(level.logLevel, details, exception);
failures.get(level).add(new Failure(level, stage, details, exception));
}

private boolean scheduleRetry() {
Expand All @@ -118,7 +102,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 @@ -133,6 +117,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
28 changes: 17 additions & 11 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,11 +561,11 @@ 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.");
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
Loading