Skip to content

Commit

Permalink
Bound the size of cache in deprecation logger (opensearch-project#16724)
Browse files Browse the repository at this point in the history
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 <[email protected]>
(cherry picked from commit b1bf72f)
  • Loading branch information
andrross committed Dec 3, 2024
1 parent b5392d7 commit 6693fb7
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 7 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ 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))
- [Tiered Caching] Fix bug in cache stats API ([#16560](https://github.com/opensearch-project/OpenSearch/pull/16560))
- Bound the size of cache in deprecation logger ([16702](https://github.com/opensearch-project/OpenSearch/issues/16702))

### Security

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> 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<String> 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;
}

/**
Expand All @@ -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<String, Object> fieldMap(String key, String xOpaqueId) {
Expand All @@ -77,6 +82,15 @@ private static Map<String, Object> 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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}

0 comments on commit 6693fb7

Please sign in to comment.