Skip to content

Update write load monitor log-levels #136137

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

Merged
merged 3 commits into from
Oct 8, 2025
+27 −13
Merged

Update write load monitor log-levels #136137

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5db5286
Update write load monitor log-levels
DiannaHohensee Oct 7, 2025
b5498c0
Merge branch 'main' into 2025/10/07/ES-13139-change-monitor-logging-l…
DiannaHohensee Oct 8, 2025
14661de
improve log message
DiannaHohensee Oct 8, 2025
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
22 changes: 17 additions & 5 deletions ...rc/main/java/org/elasticsearch/cluster/routing/allocation/WriteLoadConstraintMonitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.gateway.GatewayService;
import org.elasticsearch.threadpool.ThreadPool;

Expand Down Expand Up @@ -62,12 +63,12 @@ public WriteLoadConstraintMonitor(
public void onNewInfo(ClusterInfo clusterInfo) {
final ClusterState state = clusterStateSupplier.get();
if (state.blocks().hasGlobalBlock(GatewayService.STATE_NOT_RECOVERED_BLOCK)) {
logger.debug("skipping monitor as the cluster state is not recovered yet");
logger.trace("skipping monitor as the cluster state is not recovered yet");
return;
}

if (writeLoadConstraintSettings.getWriteLoadConstraintEnabled().notFullyEnabled()) {
logger.debug("skipping monitor because the write load decider is not fully enabled");
logger.trace("skipping monitor because the write load decider is not fully enabled");
return;
}

Expand All @@ -85,7 +86,7 @@ public void onNewInfo(ClusterInfo clusterInfo) {
});

if (nodeIdsExceedingLatencyThreshold.isEmpty()) {
logger.debug("No hot-spotting nodes detected");
logger.trace("No hot-spotting nodes detected");
return;
}

Expand All @@ -94,12 +95,20 @@ public void onNewInfo(ClusterInfo clusterInfo) {
final boolean haveCalledRerouteRecently = timeSinceLastRerouteMillis < writeLoadConstraintSettings.getMinimumRerouteInterval()
.millis();

// We know that there is at least one hot-spotting node if we've reached this code. Now check whether there are any new hot-spots
// or hot-spots that are persisting and need further balancing work.
if (haveCalledRerouteRecently == false
|| Sets.difference(nodeIdsExceedingLatencyThreshold, lastSetOfHotSpottedNodes).isEmpty() == false) {
if (logger.isDebugEnabled()) {
logger.debug(
"Found {} exceeding the write thread pool queue latency threshold ({} total), triggering reroute",
"""
Nodes [{}] are hot-spotting, of {} total cluster nodes. Reroute for hot-spotting was last called [{}] ago. \
Previously hot-spotting nodes are [{}]. The write thread pool queue latency threshold is [{}]. Triggering reroute.
""",
nodeSummary(nodeIdsExceedingLatencyThreshold),
state.nodes().size(),
lastRerouteTimeMillis == 0 ? "never" : TimeValue.timeValueMillis(timeSinceLastRerouteMillis),
Copy link
Member

Choose a reason for hiding this comment

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

With never, the log message will be was last called [never] ago which reads rather odd. I suggest we either just use TimeValue.timeValueMillis(timeSinceLastRerouteMillis) similar to how that is used in the else branch. Or adjust the logging message further so it reads something like was never called. Also, it would be great to handle it similarly here and in the else branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, it doesn't flow. I figured that it was clear, but I can make it better 👍

The else-statement has the advantage of not encountering never because it's logically impossible (haveCalledRerouteRecently is always true in that branch). Doesn't apply in this case.

nodeSummary(lastSetOfHotSpottedNodes),
state.nodes().size()
);
}
Expand All @@ -115,7 +124,10 @@ public void onNewInfo(ClusterInfo clusterInfo) {
lastRerouteTimeMillis = currentTimeMillisSupplier.getAsLong();
lastSetOfHotSpottedNodes = nodeIdsExceedingLatencyThreshold;
} else {
logger.debug("Not calling reroute because we called reroute recently and there are no new hot spots");
logger.debug(
"Not calling reroute because we called reroute [{}] ago and there are no new hot spots",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Not calling reroute because we called reroute [{}] ago and there are no new hot spots",
"Not calling reroute because we called reroute [{}] ago or there are no new hot spots",

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 believe this is and. If either enough time passed or a new hot spot, then the if-statement triggers. So the else-statement is when time has not passed AND there are no new hot-spots.

I tried to make that a bit clearer with the new comment before the if-else statement, but seems still tricky.

Copy link
Member

Choose a reason for hiding this comment

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

Err you are right.

TimeValue.timeValueMillis(timeSinceLastRerouteMillis)
);
}
}

Expand Down
16 changes: 8 additions & 8 deletions ...st/java/org/elasticsearch/cluster/routing/allocation/WriteLoadConstraintMonitorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public void testRerouteIsCalledWhenAHotSpotIsDetected() {
}

@TestLogging(
value = "org.elasticsearch.cluster.routing.allocation.WriteLoadConstraintMonitor:DEBUG",
value = "org.elasticsearch.cluster.routing.allocation.WriteLoadConstraintMonitor:TRACE",
reason = "ensure we're skipping reroute for the right reason"
)
public void testRerouteIsNotCalledWhenStateIsNotRecovered() {
Expand All @@ -81,7 +81,7 @@ public void testRerouteIsNotCalledWhenStateIsNotRecovered() {
new MockLog.SeenEventExpectation(
"don't reroute due to global block",
WriteLoadConstraintMonitor.class.getCanonicalName(),
Level.DEBUG,
Level.TRACE,
"skipping monitor as the cluster state is not recovered yet"
)
);
Expand All @@ -93,7 +93,7 @@ public void testRerouteIsNotCalledWhenStateIsNotRecovered() {
}

@TestLogging(
value = "org.elasticsearch.cluster.routing.allocation.WriteLoadConstraintMonitor:DEBUG",
value = "org.elasticsearch.cluster.routing.allocation.WriteLoadConstraintMonitor:TRACE",
reason = "ensure we're skipping reroute for the right reason"
)
public void testRerouteIsNotCalledWhenDeciderIsNotEnabled() {
Expand All @@ -117,7 +117,7 @@ public void testRerouteIsNotCalledWhenDeciderIsNotEnabled() {
new MockLog.SeenEventExpectation(
"don't reroute due to decider being disabled",
WriteLoadConstraintMonitor.class.getCanonicalName(),
Level.DEBUG,
Level.TRACE,
"skipping monitor because the write load decider is not fully enabled"
)
);
Expand All @@ -129,7 +129,7 @@ public void testRerouteIsNotCalledWhenDeciderIsNotEnabled() {
}

@TestLogging(
value = "org.elasticsearch.cluster.routing.allocation.WriteLoadConstraintMonitor:DEBUG",
value = "org.elasticsearch.cluster.routing.allocation.WriteLoadConstraintMonitor:TRACE",
reason = "ensure we're skipping reroute for the right reason"
)
public void testRerouteIsNotCalledWhenNoNodesAreHotSpotting() {
Expand All @@ -146,7 +146,7 @@ public void testRerouteIsNotCalledWhenNoNodesAreHotSpotting() {
new MockLog.SeenEventExpectation(
"don't reroute due to no nodes hot-spotting",
WriteLoadConstraintMonitor.class.getCanonicalName(),
Level.DEBUG,
Level.TRACE,
"No hot-spotting nodes detected"
)
);
Expand Down Expand Up @@ -196,7 +196,7 @@ public void testRerouteIsNotCalledAgainBeforeMinimumIntervalHasPassed() {
"don't reroute due to reroute being called recently",
WriteLoadConstraintMonitor.class.getCanonicalName(),
Level.DEBUG,
"Not calling reroute because we called reroute recently and there are no new hot spots"
"Not calling reroute because we called reroute * ago and there are no new hot spots"
)
);
writeLoadConstraintMonitor.onNewInfo(testState.clusterInfo);
Expand All @@ -213,7 +213,7 @@ public void testRerouteIsNotCalledAgainBeforeMinimumIntervalHasPassed() {
}

@TestLogging(
value = "org.elasticsearch.cluster.routing.allocation.WriteLoadConstraintMonitor:DEBUG",
value = "org.elasticsearch.cluster.routing.allocation.WriteLoadConstraintMonitor:TRACE",
reason = "ensure we're skipping reroute for the right reason"
)
public void testRerouteIsCalledBeforeMinimumIntervalHasPassedIfNewNodesBecomeHotSpotted() {
Expand Down