From 7449adb8993272dd9081db3569bebc592be59ac3 Mon Sep 17 00:00:00 2001 From: Andrew Ross Date: Tue, 3 Dec 2024 11:59:46 -0800 Subject: [PATCH] Bound the size of cache in deprecation logger (#16724) The current implementation of the map used to de-duplicate deprecation log messages can grow without bound. This adds a simple fixed limit to the data structure tracking existing loggers. Once the limit is breached new loggers will no longer log deprecation warnings. I also added a check to skip the tracking if the deprecation logger is disabled. Signed-off-by: Andrew Ross (cherry picked from commit b1bf72f26e2681e4dbe726bc9605209675f6ab38) Signed-off-by: Andrew Ross --- CHANGELOG.md | 1 + .../common/logging/DeprecatedMessage.java | 22 +++++++++++++++---- .../common/logging/DeprecationLogger.java | 9 +++++--- .../logging/DeprecationLoggerTests.java | 18 +++++++++++++++ 4 files changed, 43 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 54c9988f14450..13836419559f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix permissions error on scripted query against remote snapshot ([#16544](https://github.com/opensearch-project/OpenSearch/pull/16544)) - Fix `doc_values` only (`index:false`) IP field searching for masks ([#16628](https://github.com/opensearch-project/OpenSearch/pull/16628)) - Fix stale cluster state custom file deletion ([#16670](https://github.com/opensearch-project/OpenSearch/pull/16670)) +- Bound the size of cache in deprecation logger ([16702](https://github.com/opensearch-project/OpenSearch/issues/16702)) ### Security diff --git a/server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java b/server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java index 25c1ba9675600..b906752e74b31 100644 --- a/server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java +++ b/server/src/main/java/org/opensearch/common/logging/DeprecatedMessage.java @@ -47,12 +47,17 @@ */ public class DeprecatedMessage extends OpenSearchLogMessage { public static final String X_OPAQUE_ID_FIELD_NAME = "x-opaque-id"; - private static final Set keys = ConcurrentHashMap.newKeySet(); + + // Arbitrary maximum size, should be much larger than unique number of + // loggers, but small relative to heap size. + static final int MAX_DEDUPE_CACHE_ENTRIES = 16_384; + + private static final Set keyDedupeCache = ConcurrentHashMap.newKeySet(); private final String keyWithXOpaqueId; public DeprecatedMessage(String key, String xOpaqueId, String messagePattern, Object... args) { super(fieldMap(key, xOpaqueId), messagePattern, args); - this.keyWithXOpaqueId = new StringBuilder().append(key).append(xOpaqueId).toString(); + this.keyWithXOpaqueId = key + xOpaqueId; } /** @@ -62,7 +67,7 @@ public DeprecatedMessage(String key, String xOpaqueId, String messagePattern, Ob * Otherwise, a warning can be logged by some test and the upcoming test can be impacted by it. */ public static void resetDeprecatedMessageForTests() { - keys.clear(); + keyDedupeCache.clear(); } private static Map fieldMap(String key, String xOpaqueId) { @@ -77,6 +82,15 @@ private static Map fieldMap(String key, String xOpaqueId) { } public boolean isAlreadyLogged() { - return !keys.add(keyWithXOpaqueId); + if (keyDedupeCache.contains(keyWithXOpaqueId)) { + return true; + } + if (keyDedupeCache.size() >= MAX_DEDUPE_CACHE_ENTRIES) { + // Stop logging if max size is breached to avoid performance problems from + // excessive logging. The historical logs will be full of deprecation warnings + // at this point anyway. + return true; + } + return !keyDedupeCache.add(keyWithXOpaqueId); } } diff --git a/server/src/main/java/org/opensearch/common/logging/DeprecationLogger.java b/server/src/main/java/org/opensearch/common/logging/DeprecationLogger.java index d4dbb953ffe12..7a1911f6a83c8 100644 --- a/server/src/main/java/org/opensearch/common/logging/DeprecationLogger.java +++ b/server/src/main/java/org/opensearch/common/logging/DeprecationLogger.java @@ -116,9 +116,12 @@ public DeprecationLoggerBuilder deprecate(final String key, final String msg, fi public class DeprecationLoggerBuilder { public DeprecationLoggerBuilder withDeprecation(String key, String msg, Object[] params) { - DeprecatedMessage deprecationMessage = new DeprecatedMessage(key, HeaderWarning.getXOpaqueId(), msg, params); - if (!deprecationMessage.isAlreadyLogged()) { - logger.log(DEPRECATION, deprecationMessage); + // Check if the logger is enabled to skip the overhead of deduplicating messages if the logger is disabled + if (logger.isEnabled(DEPRECATION)) { + DeprecatedMessage deprecationMessage = new DeprecatedMessage(key, HeaderWarning.getXOpaqueId(), msg, params); + if (!deprecationMessage.isAlreadyLogged()) { + logger.log(DEPRECATION, deprecationMessage); + } } return this; } diff --git a/server/src/test/java/org/opensearch/common/logging/DeprecationLoggerTests.java b/server/src/test/java/org/opensearch/common/logging/DeprecationLoggerTests.java index 96ee7831c20ed..98fa1fc4022fe 100644 --- a/server/src/test/java/org/opensearch/common/logging/DeprecationLoggerTests.java +++ b/server/src/test/java/org/opensearch/common/logging/DeprecationLoggerTests.java @@ -69,4 +69,22 @@ public void testDuplicateLogMessages() { // assert that only unique warnings are logged assertWarnings("Deprecated message 1", "Deprecated message 2", "Deprecated message 3"); } + + public void testMaximumSizeOfCache() { + final int maxEntries = DeprecatedMessage.MAX_DEDUPE_CACHE_ENTRIES; + // Fill up the cache, asserting every message is new + for (int i = 0; i < maxEntries; i++) { + DeprecatedMessage message = new DeprecatedMessage("key-" + i, "message-" + i, ""); + assertFalse(message.toString(), message.isAlreadyLogged()); + } + // Do the same thing except assert every message has been seen + for (int i = 0; i < maxEntries; i++) { + DeprecatedMessage message = new DeprecatedMessage("key-" + i, "message-" + i, ""); + assertTrue(message.toString(), message.isAlreadyLogged()); + } + // Add one more new entry, asserting it will forever been seen as already logged (cache is full) + DeprecatedMessage message = new DeprecatedMessage("key-new", "message-new", ""); + assertTrue(message.toString(), message.isAlreadyLogged()); + assertTrue(message.toString(), message.isAlreadyLogged()); + } }