From a0a4a543dcb6e1cb050c31d354bc7af07b8b0ab4 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 27 Dec 2024 10:14:07 -0800 Subject: [PATCH] indirect slowlog provider is not stateful --- .../elasticsearch/index/IndexingSlowLog.java | 12 ++-- .../elasticsearch/index/SearchSlowLog.java | 21 +++---- .../index/SlowLogFieldProvider.java | 18 +----- .../elasticsearch/index/SlowLogFields.java | 30 +++++++++ .../indices/IndicesServiceBuilder.java | 21 ++++--- .../elasticsearch/node/NodeConstruction.java | 35 +++++------ .../indices/IndicesServiceTests.java | 62 ++++++++++--------- .../slowlog/SecuritySlowLogFieldProvider.java | 59 ++++++++++-------- 8 files changed, 145 insertions(+), 113 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/index/SlowLogFields.java diff --git a/server/src/main/java/org/elasticsearch/index/IndexingSlowLog.java b/server/src/main/java/org/elasticsearch/index/IndexingSlowLog.java index 3ae4c0eb82ad0..2d7185eeb7c25 100644 --- a/server/src/main/java/org/elasticsearch/index/IndexingSlowLog.java +++ b/server/src/main/java/org/elasticsearch/index/IndexingSlowLog.java @@ -103,7 +103,7 @@ public final class IndexingSlowLog implements IndexingOperationListener { * characters of the source. */ private int maxSourceCharsToLog; - private final SlowLogFieldProvider slowLogFieldProvider; + private final SlowLogFields slowLogFields; /** * Reads how much of the source to log. The user can specify any value they @@ -126,7 +126,7 @@ public final class IndexingSlowLog implements IndexingOperationListener { ); IndexingSlowLog(IndexSettings indexSettings, SlowLogFieldProvider slowLogFieldProvider) { - this.slowLogFieldProvider = slowLogFieldProvider; + this.slowLogFields = slowLogFieldProvider.create(indexSettings); this.index = indexSettings.getIndex(); indexSettings.getScopedSettings().addSettingsUpdateConsumer(INDEX_INDEXING_SLOWLOG_REFORMAT_SETTING, this::setReformat); @@ -180,7 +180,7 @@ public void postIndex(ShardId shardId, Engine.Index indexOperation, Engine.Index if (indexWarnThreshold >= 0 && tookInNanos > indexWarnThreshold) { indexLogger.warn( IndexingSlowLogMessage.of( - this.slowLogFieldProvider.indexSlowLogFields(), + this.slowLogFields.indexFields(), index, doc, tookInNanos, @@ -191,7 +191,7 @@ public void postIndex(ShardId shardId, Engine.Index indexOperation, Engine.Index } else if (indexInfoThreshold >= 0 && tookInNanos > indexInfoThreshold) { indexLogger.info( IndexingSlowLogMessage.of( - this.slowLogFieldProvider.indexSlowLogFields(), + this.slowLogFields.indexFields(), index, doc, tookInNanos, @@ -202,7 +202,7 @@ public void postIndex(ShardId shardId, Engine.Index indexOperation, Engine.Index } else if (indexDebugThreshold >= 0 && tookInNanos > indexDebugThreshold) { indexLogger.debug( IndexingSlowLogMessage.of( - this.slowLogFieldProvider.indexSlowLogFields(), + this.slowLogFields.indexFields(), index, doc, tookInNanos, @@ -213,7 +213,7 @@ public void postIndex(ShardId shardId, Engine.Index indexOperation, Engine.Index } else if (indexTraceThreshold >= 0 && tookInNanos > indexTraceThreshold) { indexLogger.trace( IndexingSlowLogMessage.of( - this.slowLogFieldProvider.indexSlowLogFields(), + this.slowLogFields.indexFields(), index, doc, tookInNanos, diff --git a/server/src/main/java/org/elasticsearch/index/SearchSlowLog.java b/server/src/main/java/org/elasticsearch/index/SearchSlowLog.java index e4836a391bfec..480c4f3b0c70a 100644 --- a/server/src/main/java/org/elasticsearch/index/SearchSlowLog.java +++ b/server/src/main/java/org/elasticsearch/index/SearchSlowLog.java @@ -45,7 +45,7 @@ public final class SearchSlowLog implements SearchOperationListener { private static final Logger queryLogger = LogManager.getLogger(INDEX_SEARCH_SLOWLOG_PREFIX + ".query"); private static final Logger fetchLogger = LogManager.getLogger(INDEX_SEARCH_SLOWLOG_PREFIX + ".fetch"); - private final SlowLogFieldProvider slowLogFieldProvider; + private final SlowLogFields slowLogFields; public static final Setting INDEX_SEARCH_SLOWLOG_INCLUDE_USER_SETTING = Setting.boolSetting( INDEX_SEARCH_SLOWLOG_PREFIX + ".include.user", @@ -127,8 +127,7 @@ public final class SearchSlowLog implements SearchOperationListener { private static final ToXContent.Params FORMAT_PARAMS = new ToXContent.MapParams(Collections.singletonMap("pretty", "false")); public SearchSlowLog(IndexSettings indexSettings, SlowLogFieldProvider slowLogFieldProvider) { - slowLogFieldProvider.init(indexSettings); - this.slowLogFieldProvider = slowLogFieldProvider; + this.slowLogFields = slowLogFieldProvider.create(indexSettings); indexSettings.getScopedSettings() .addSettingsUpdateConsumer(INDEX_SEARCH_SLOWLOG_THRESHOLD_QUERY_WARN_SETTING, this::setQueryWarnThreshold); this.queryWarnThreshold = indexSettings.getValue(INDEX_SEARCH_SLOWLOG_THRESHOLD_QUERY_WARN_SETTING).nanos(); @@ -159,26 +158,26 @@ public SearchSlowLog(IndexSettings indexSettings, SlowLogFieldProvider slowLogFi @Override public void onQueryPhase(SearchContext context, long tookInNanos) { if (queryWarnThreshold >= 0 && tookInNanos > queryWarnThreshold) { - queryLogger.warn(SearchSlowLogMessage.of(this.slowLogFieldProvider.searchSlowLogFields(), context, tookInNanos)); + queryLogger.warn(SearchSlowLogMessage.of(this.slowLogFields.searchFields(), context, tookInNanos)); } else if (queryInfoThreshold >= 0 && tookInNanos > queryInfoThreshold) { - queryLogger.info(SearchSlowLogMessage.of(this.slowLogFieldProvider.searchSlowLogFields(), context, tookInNanos)); + queryLogger.info(SearchSlowLogMessage.of(this.slowLogFields.searchFields(), context, tookInNanos)); } else if (queryDebugThreshold >= 0 && tookInNanos > queryDebugThreshold) { - queryLogger.debug(SearchSlowLogMessage.of(this.slowLogFieldProvider.searchSlowLogFields(), context, tookInNanos)); + queryLogger.debug(SearchSlowLogMessage.of(this.slowLogFields.searchFields(), context, tookInNanos)); } else if (queryTraceThreshold >= 0 && tookInNanos > queryTraceThreshold) { - queryLogger.trace(SearchSlowLogMessage.of(this.slowLogFieldProvider.searchSlowLogFields(), context, tookInNanos)); + queryLogger.trace(SearchSlowLogMessage.of(this.slowLogFields.searchFields(), context, tookInNanos)); } } @Override public void onFetchPhase(SearchContext context, long tookInNanos) { if (fetchWarnThreshold >= 0 && tookInNanos > fetchWarnThreshold) { - fetchLogger.warn(SearchSlowLogMessage.of(this.slowLogFieldProvider.searchSlowLogFields(), context, tookInNanos)); + fetchLogger.warn(SearchSlowLogMessage.of(this.slowLogFields.searchFields(), context, tookInNanos)); } else if (fetchInfoThreshold >= 0 && tookInNanos > fetchInfoThreshold) { - fetchLogger.info(SearchSlowLogMessage.of(this.slowLogFieldProvider.searchSlowLogFields(), context, tookInNanos)); + fetchLogger.info(SearchSlowLogMessage.of(this.slowLogFields.searchFields(), context, tookInNanos)); } else if (fetchDebugThreshold >= 0 && tookInNanos > fetchDebugThreshold) { - fetchLogger.debug(SearchSlowLogMessage.of(this.slowLogFieldProvider.searchSlowLogFields(), context, tookInNanos)); + fetchLogger.debug(SearchSlowLogMessage.of(this.slowLogFields.searchFields(), context, tookInNanos)); } else if (fetchTraceThreshold >= 0 && tookInNanos > fetchTraceThreshold) { - fetchLogger.trace(SearchSlowLogMessage.of(this.slowLogFieldProvider.searchSlowLogFields(), context, tookInNanos)); + fetchLogger.trace(SearchSlowLogMessage.of(this.slowLogFields.searchFields(), context, tookInNanos)); } } diff --git a/server/src/main/java/org/elasticsearch/index/SlowLogFieldProvider.java b/server/src/main/java/org/elasticsearch/index/SlowLogFieldProvider.java index c61d4d4c85a86..e93edccc83b15 100644 --- a/server/src/main/java/org/elasticsearch/index/SlowLogFieldProvider.java +++ b/server/src/main/java/org/elasticsearch/index/SlowLogFieldProvider.java @@ -9,28 +9,14 @@ package org.elasticsearch.index; -import java.util.Map; - /** * Interface for providing additional fields to the slow log from a plugin. * Intended to be loaded through SPI. */ public interface SlowLogFieldProvider { /** - * Initialize field provider with index level settings to be able to listen for updates and set initial values + * Create a field provider with index level settings to be able to listen for updates and set initial values * @param indexSettings settings for the index */ - void init(IndexSettings indexSettings); - - /** - * Slow log fields for indexing events - * @return map of field name to value - */ - Map indexSlowLogFields(); - - /** - * Slow log fields for search events - * @return map of field name to value - */ - Map searchSlowLogFields(); + SlowLogFields create(IndexSettings indexSettings); } diff --git a/server/src/main/java/org/elasticsearch/index/SlowLogFields.java b/server/src/main/java/org/elasticsearch/index/SlowLogFields.java new file mode 100644 index 0000000000000..e018e3a4d6bb7 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/index/SlowLogFields.java @@ -0,0 +1,30 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +package org.elasticsearch.index; + +import java.util.Map; + +/** + * Fields for the slow log. These may be different each call depending on the state of the system. + */ +public interface SlowLogFields { + + /** + * Slow log fields for indexing events + * @return map of field name to value + */ + Map indexFields(); + + /** + * Slow log fields for search events + * @return map of field name to value + */ + Map searchFields(); +} diff --git a/server/src/main/java/org/elasticsearch/indices/IndicesServiceBuilder.java b/server/src/main/java/org/elasticsearch/indices/IndicesServiceBuilder.java index 950aaa191337e..d88bbfa3eba17 100644 --- a/server/src/main/java/org/elasticsearch/indices/IndicesServiceBuilder.java +++ b/server/src/main/java/org/elasticsearch/indices/IndicesServiceBuilder.java @@ -24,6 +24,7 @@ import org.elasticsearch.gateway.MetaStateService; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.SlowLogFieldProvider; +import org.elasticsearch.index.SlowLogFields; import org.elasticsearch.index.analysis.AnalysisRegistry; import org.elasticsearch.index.engine.EngineFactory; import org.elasticsearch.index.mapper.MapperMetrics; @@ -82,16 +83,18 @@ public class IndicesServiceBuilder { QueryRewriteInterceptor queryRewriteInterceptor = null; SlowLogFieldProvider slowLogFieldProvider = new SlowLogFieldProvider() { @Override - public void init(IndexSettings indexSettings) {} - - @Override - public Map indexSlowLogFields() { - return Map.of(); - } + public SlowLogFields create(IndexSettings indexSettings) { + return new SlowLogFields() { + @Override + public Map indexFields() { + return Map.of(); + } - @Override - public Map searchSlowLogFields() { - return Map.of(); + @Override + public Map searchFields() { + return Map.of(); + } + }; } }; diff --git a/server/src/main/java/org/elasticsearch/node/NodeConstruction.java b/server/src/main/java/org/elasticsearch/node/NodeConstruction.java index 6fc5c26974803..0ebc33e0a5371 100644 --- a/server/src/main/java/org/elasticsearch/node/NodeConstruction.java +++ b/server/src/main/java/org/elasticsearch/node/NodeConstruction.java @@ -111,9 +111,9 @@ import org.elasticsearch.index.IndexMode; import org.elasticsearch.index.IndexSettingProvider; import org.elasticsearch.index.IndexSettingProviders; -import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.IndexingPressure; import org.elasticsearch.index.SlowLogFieldProvider; +import org.elasticsearch.index.SlowLogFields; import org.elasticsearch.index.analysis.AnalysisRegistry; import org.elasticsearch.index.mapper.MapperMetrics; import org.elasticsearch.index.mapper.SourceFieldMetrics; @@ -810,25 +810,24 @@ private void construct( List slowLogFieldProviders = pluginsService.loadServiceProviders(SlowLogFieldProvider.class); // NOTE: the response of index/search slow log fields below must be calculated dynamically on every call // because the responses may change dynamically at runtime - SlowLogFieldProvider slowLogFieldProvider = new SlowLogFieldProvider() { - @Override - public void init(IndexSettings indexSettings) { - slowLogFieldProviders.forEach(provider -> provider.init(indexSettings)); - } - - @Override - public Map indexSlowLogFields() { - return slowLogFieldProviders.stream() - .flatMap(provider -> provider.indexSlowLogFields().entrySet().stream()) - .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); + SlowLogFieldProvider slowLogFieldProvider = indexSettings -> { + final List fields = new ArrayList<>(); + for (var provider : slowLogFieldProviders) { + fields.add(provider.create(indexSettings)); } + return new SlowLogFields() { + @Override + public Map indexFields() { + return fields.stream().flatMap(f -> f.indexFields().entrySet().stream()) + .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); + } - @Override - public Map searchSlowLogFields() { - return slowLogFieldProviders.stream() - .flatMap(provider -> provider.searchSlowLogFields().entrySet().stream()) - .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); - } + @Override + public Map searchFields() { + return fields.stream().flatMap(f -> f.searchFields().entrySet().stream()) + .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); + } + }; }; IndicesService indicesService = new IndicesServiceBuilder().settings(settings) diff --git a/server/src/test/java/org/elasticsearch/indices/IndicesServiceTests.java b/server/src/test/java/org/elasticsearch/indices/IndicesServiceTests.java index 1571777449e91..b56afadb14924 100644 --- a/server/src/test/java/org/elasticsearch/indices/IndicesServiceTests.java +++ b/server/src/test/java/org/elasticsearch/indices/IndicesServiceTests.java @@ -44,6 +44,7 @@ import org.elasticsearch.index.IndexVersion; import org.elasticsearch.index.IndexVersions; import org.elasticsearch.index.SlowLogFieldProvider; +import org.elasticsearch.index.SlowLogFields; import org.elasticsearch.index.engine.Engine; import org.elasticsearch.index.engine.EngineConfig; import org.elasticsearch.index.engine.EngineFactory; @@ -209,16 +210,18 @@ static void setFields(Map fields) { } @Override - public void init(IndexSettings indexSettings) {} - - @Override - public Map indexSlowLogFields() { - return fields; - } - - @Override - public Map searchSlowLogFields() { - return fields; + public SlowLogFields create(IndexSettings indexSettings) { + return new SlowLogFields() { + @Override + public Map indexFields() { + return fields; + } + + @Override + public Map searchFields() { + return fields; + } + }; } } @@ -231,16 +234,18 @@ static void setFields(Map fields) { } @Override - public void init(IndexSettings indexSettings) {} - - @Override - public Map indexSlowLogFields() { - return fields; - } - - @Override - public Map searchSlowLogFields() { - return fields; + public SlowLogFields create(IndexSettings indexSettings) { + return new SlowLogFields() { + @Override + public Map indexFields() { + return fields; + } + + @Override + public Map searchFields() { + return fields; + } + }; } } @@ -806,32 +811,33 @@ public void testLoadSlowLogFieldProvider() { var indicesService = getIndicesService(); SlowLogFieldProvider fieldProvider = indicesService.slowLogFieldProvider; + SlowLogFields fields = fieldProvider.create(null); // The map of fields from the two providers are merged to a single map of fields - assertEquals(Map.of("key1", "value1", "key2", "value2"), fieldProvider.searchSlowLogFields()); - assertEquals(Map.of("key1", "value1", "key2", "value2"), fieldProvider.indexSlowLogFields()); + assertEquals(Map.of("key1", "value1", "key2", "value2"), fields.searchFields()); + assertEquals(Map.of("key1", "value1", "key2", "value2"), fields.indexFields()); TestSlowLogFieldProvider.setFields(Map.of("key1", "value1")); TestAnotherSlowLogFieldProvider.setFields(Map.of("key1", "value2")); // There is an overlap of field names, since this isn't deterministic and probably a // programming error (two providers provide the same field) throw an exception - assertThrows(IllegalStateException.class, fieldProvider::searchSlowLogFields); - assertThrows(IllegalStateException.class, fieldProvider::indexSlowLogFields); + assertThrows(IllegalStateException.class, fields::searchFields); + assertThrows(IllegalStateException.class, fields::indexFields); TestSlowLogFieldProvider.setFields(Map.of("key1", "value1")); TestAnotherSlowLogFieldProvider.setFields(Map.of()); // One provider has no fields - assertEquals(Map.of("key1", "value1"), fieldProvider.searchSlowLogFields()); - assertEquals(Map.of("key1", "value1"), fieldProvider.indexSlowLogFields()); + assertEquals(Map.of("key1", "value1"), fields.searchFields()); + assertEquals(Map.of("key1", "value1"), fields.indexFields()); TestSlowLogFieldProvider.setFields(Map.of()); TestAnotherSlowLogFieldProvider.setFields(Map.of()); // Both providers have no fields - assertEquals(Map.of(), fieldProvider.searchSlowLogFields()); - assertEquals(Map.of(), fieldProvider.indexSlowLogFields()); + assertEquals(Map.of(), fields.searchFields()); + assertEquals(Map.of(), fields.indexFields()); } public void testWithTempIndexServiceHandlesExistingIndex() throws Exception { diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/slowlog/SecuritySlowLogFieldProvider.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/slowlog/SecuritySlowLogFieldProvider.java index 1610aedd1d363..9a7a8fd7e86de 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/slowlog/SecuritySlowLogFieldProvider.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/slowlog/SecuritySlowLogFieldProvider.java @@ -9,6 +9,7 @@ import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.SlowLogFieldProvider; +import org.elasticsearch.index.SlowLogFields; import org.elasticsearch.xpack.security.Security; import java.util.Map; @@ -18,8 +19,37 @@ public class SecuritySlowLogFieldProvider implements SlowLogFieldProvider { private final Security plugin; - private boolean includeUserInIndexing = false; - private boolean includeUserInSearch = false; + + + private class SecuritySlowLogFields implements SlowLogFields{ + private boolean includeUserInIndexing = false; + private boolean includeUserInSearch = false; + + SecuritySlowLogFields(IndexSettings indexSettings) { + indexSettings.getScopedSettings() + .addSettingsUpdateConsumer(INDEX_SEARCH_SLOWLOG_INCLUDE_USER_SETTING, newValue -> this.includeUserInSearch = newValue); + this.includeUserInSearch = indexSettings.getValue(INDEX_SEARCH_SLOWLOG_INCLUDE_USER_SETTING); + indexSettings.getScopedSettings() + .addSettingsUpdateConsumer(INDEX_INDEXING_SLOWLOG_INCLUDE_USER_SETTING, newValue -> this.includeUserInIndexing = newValue); + this.includeUserInIndexing = indexSettings.getValue(INDEX_INDEXING_SLOWLOG_INCLUDE_USER_SETTING); + } + + @Override + public Map indexFields() { + if (includeUserInIndexing) { + return plugin.getAuthContextForSlowLog(); + } + return Map.of(); + } + + @Override + public Map searchFields() { + if (includeUserInSearch) { + return plugin.getAuthContextForSlowLog(); + } + return Map.of(); + } + } public SecuritySlowLogFieldProvider() { throw new IllegalStateException("Provider must be constructed using PluginsService"); @@ -30,28 +60,7 @@ public SecuritySlowLogFieldProvider(Security plugin) { } @Override - public void init(IndexSettings indexSettings) { - indexSettings.getScopedSettings() - .addSettingsUpdateConsumer(INDEX_SEARCH_SLOWLOG_INCLUDE_USER_SETTING, newValue -> this.includeUserInSearch = newValue); - this.includeUserInSearch = indexSettings.getValue(INDEX_SEARCH_SLOWLOG_INCLUDE_USER_SETTING); - indexSettings.getScopedSettings() - .addSettingsUpdateConsumer(INDEX_INDEXING_SLOWLOG_INCLUDE_USER_SETTING, newValue -> this.includeUserInIndexing = newValue); - this.includeUserInIndexing = indexSettings.getValue(INDEX_INDEXING_SLOWLOG_INCLUDE_USER_SETTING); - } - - @Override - public Map indexSlowLogFields() { - if (includeUserInIndexing) { - return plugin.getAuthContextForSlowLog(); - } - return Map.of(); - } - - @Override - public Map searchSlowLogFields() { - if (includeUserInSearch) { - return plugin.getAuthContextForSlowLog(); - } - return Map.of(); + public SlowLogFields create(IndexSettings indexSettings) { + return new SecuritySlowLogFields(indexSettings); } }