Skip to content

Add logging to native health check #231

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add logging to native health check #231

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions ...core/src/main/java/com/palantir/docker/compose/connection/waiting/ClusterHealthCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,13 @@
import java.util.List;
import java.util.Set;
import java.util.function.Function;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

@FunctionalInterface
public interface ClusterHealthCheck {
Logger log = LoggerFactory.getLogger(ClusterHealthCheck.class);
Copy link
Author

Choose a reason for hiding this comment

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

Having this here is sad. Should I make ClusterHealthCheck an abstract class and have this as a private variable?

Copy link
Contributor

@j-baker j-baker Mar 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Nope - instead, note that this is only used in NativeHealthCheck and do

static ClusterHealthCheck nativeHealthChecks() {
    return new ClusterHealthCheck() {
        static final Logger log = LoggerFactory.getLogger(ClusterHealthCheck.class);
        @Override
        SuccessOrFailure isClusterHealthy(Cluster cluster) throws InterruptedException {
            Set<String> unhealthyContainers = new LinkedHashSet<>();
            .........
        }
    }
}


static ClusterHealthCheck serviceHealthCheck(List<String> containerNames, HealthCheck<List<Container>> delegate) {
return transformingHealthCheck(cluster -> cluster.containers(containerNames), delegate);
}
Expand All @@ -52,13 +56,18 @@ static ClusterHealthCheck nativeHealthChecks() {
return cluster -> {
Set<String> unhealthyContainers = new LinkedHashSet<>();
try {
log.info("Checking health of containers {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

these may be a little spammy, FYI. Think this is done in a fairly tight loop, so you may end up with many log lines.

Instead, I'd recommend doing this like:

Set<String> lastUnhealthyContainers = new LinkedHashSet<>();
return new ClusterHealthCheck() {
    @Override
    public SuccessOrFailure isClusterHealthy(Cluster cluster) throws InterruptedException {
        Set<String> currentUnhealthyContainers = new LinkedHashSet<>();
        boolean healthyContainerSetChanged = false;
        for (Container container : cluster.allContainers()) {
            State state = container.state();
            if (state == State.UNHEALTHY) {
                currentUnhealthyContainers.add(container.getContainerName());
            }
        }

        if (currentUnhealthyContainers.equals(lastUnhealthyContainers)) {
              log(currentUnhealthyContainers);
              lastUnhealthyContainers.clear();
              lastUnhealthyContainers.addAll(currentUnhealthyContainers);
        }

        if (!currentUnhealthyContainers.isEmpty()) {
            return SuccessOrFailure.failure("The following containers are not healthy: " ...);
        }

        return SuccessOrFailure.success();
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

where you only log if the state has actually changed.

cluster.allContainers().stream().map(Container::toString).collect(joining(", ")));
for (Container container : cluster.allContainers()) {
log.info("Checking health of container {}", container);
State state = container.state();
if (state == State.UNHEALTHY) {
log.info("Container unhealthy {}", container);
unhealthyContainers.add(container.getContainerName());
}
}
if (!unhealthyContainers.isEmpty()) {
log.warn("Unhealthy containers {}", unhealthyContainers.stream().collect(joining(", ")));
return SuccessOrFailure.failure(
"The following containers are not healthy: " + unhealthyContainers.stream().collect(joining(", ")));
}
Expand Down