Introduce an `IndexSettingsProvider` to inject logsdb index mode #113505

salvatore-campagna · 2024-09-25T07:08:05Z

Here we introduce a new implementation of IndexSettingProvider whose goal is to "inject" the
index.mode setting with value logsdb when a cluster setting cluster.logsdb.enabled is true.
We also make sure that:

the existing index.mode is not set
the datastream name matches the logs-*-* pattern
logs@settings component template is used

Resolves #113507

salvatore-campagna · 2024-09-25T07:09:23Z

...core/src/main/java/org/elasticsearch/datastreams/logsdb/LogsdbIndexModeSettingsProvider.java

+    private static final PatternMatcher logsNameMatcher = PatternMatcher.forLogs();
+
+    public static final Setting<Boolean> CLUSTER_INDEX_MODE_LOGSDB_ENABLED = Setting.boolSetting(
+        "cluster.index.mode.logsdb.enabled",


I wanted to use a different setting to avoid reusing an existing one. I think it would be better to remove the cluster.logsdb.enabled setting.

What is wrong with the existing setting? Maybe just move it to here?

So I see the following issues:

if we use the existing one and keep the old behaviour we would have a single setting controlling two different things, the index setting provider injection logic and the logs@setting template injection

if we use the existing one and remove the old behaviour (from logs@settings) than the behaviour of the setting will change with elasticsearch version...

The old mechanism doesn't work and I think we should remove it. Integrations can either hardcode index.mode=logsdb in index templates or they can rely on this index settings provider to enable logsdb.

martijnvg

I left a few comments. Like we discussed this index settings provider should move to do soon to be added logsdb xpack plugin.

martijnvg · 2024-09-25T07:19:19Z

x-pack/plugin/core/src/main/java/org/elasticsearch/datastreams/logsdb/PatternMatcher.java

+
+import java.util.regex.Pattern;
+
+public class PatternMatcher {


I don't think this is needed. We already have logic that deals with pattern matches for index templates. I think just using is good enough: Regex.simpleMatch(logs-*-*, dataStreamName)`

martijnvg · 2024-09-25T07:21:55Z

...core/src/main/java/org/elasticsearch/datastreams/logsdb/LogsdbIndexModeSettingsProvider.java

+            return Settings.EMPTY;
+        }
+
+        final IndexMode indexMode = resolveIndexMode(settings.get("index.mode"));


I don't think we want to overwrite index mode, even if index.mode=standard is set. I think we should only resolve it when it isn't defined. So maybe just check if index.mode is set and only then based on data stream name decide whether logsdb index mode should be used?

I understand that the IndexSettingsProvier mechanism kicks in after template composition happens...if we do what you suggest and the template composition completes with index.mode: standard than we would not overwrite it with logsdb. Is this what we want?

Yes, I think we should only configure index.mode to logsdb when it hasn't been set already.

Let's only set index.mode to logsdb when no index mode has been provided?

final IndexMode indexMode = resolveIndexMode(settings.get("index.mode")); if (indexMode != null) { return Settings.EMPTY; }

This is what I do...I check it for null

martijnvg · 2024-09-25T07:23:16Z

...core/src/main/java/org/elasticsearch/datastreams/logsdb/LogsdbIndexModeSettingsProvider.java

+    private static final PatternMatcher logsNameMatcher = PatternMatcher.forLogs();
+
+    public static final Setting<Boolean> CLUSTER_INDEX_MODE_LOGSDB_ENABLED = Setting.boolSetting(
+        "cluster.index.mode.logsdb.enabled",


What is wrong with the existing setting? Maybe just move it to here?

martijnvg · 2024-09-25T07:24:50Z

...core/src/main/java/org/elasticsearch/datastreams/logsdb/LogsdbIndexModeSettingsProvider.java

+    );
+    private boolean isLogsdbEnabled;
+
+    public LogsdbIndexModeSettingsProvider(final ClusterService clusterService) {


Let's remove the ClusterService dependency from this class? This would make unit testing this class easier.
We can do clusterService.getClusterSettings().addSettingsUpdateConsumer(...) from the plugin class. The setter is already defined here.

x-pack/plugin/core/src/main/java/org/elasticsearch/datastreams/logsdb/PatternMatcher.java

rjernst

Is the PR title based on a previous implementation? I do not see anything called an IndexSettingsProvider here?

rjernst · 2024-09-26T00:01:13Z

server/src/main/java/org/elasticsearch/node/NodeConstruction.java

@@ -514,7 +514,7 @@ private SettingsModule validateSettings(Settings envSettings, Settings settings,
        // so we might be late here already
        SettingsModule settingsModule = new SettingsModule(
            settings,
-            additionalSettings,
+            additionalSettings.stream().distinct().collect(Collectors.toList()),


Why is this needed? We should not have duplicate settings

Yes, I don't think a change to this file is needed.

There is a setting cluster.logsdb.enabedl which is used by StackPlugin and LogsDBPlugin, and which is returned by method getSettings by both. SO I have two options here:

I deduplicate the additional settings as done here.

I return the setting only from one of the plugins.

I saw the code in NodeConstruction passing these settings to the SettingsModule and I see the logic to register settings which does not allow duplicates. Some of my tests were failing before introducing this change because of the exception thrown while registering duplicate settings. I see the logic uses a list of settings rather than a set which tells me that for some reason duplicates are not expected.

For my understanding would be useful to know why duplicates are not expected. Of course it is clear to me that registering duplicate settings would be incorrect...but I don't see what is wrong in deduplicating them before registration happens. Probably because we would not know which one is registered and which one is removed...?

Also I wonder if it is possible to have multiple instances of the same plugin...which would result in duplicate settings too. I imagine not. (My knowledge here is a bit limited TBH).

@rjernst what is the right way to proceed? Is there any reason why we can't return the same settings from two different plugins and deduplicate. SHould I go for option 2 above and just return the setting from one of them?

I return the setting only from one of the plugins.

This is the only option. Registering the same setting twice isn't allowed because this would allow for leniency.

.../javaRestTest/java/org/elasticsearch/datastreams/logsdb/LogsIndexModeDisabledRestTestIT.java

martijnvg · 2024-09-26T05:18:13Z

server/src/main/java/org/elasticsearch/node/NodeConstruction.java

@@ -514,7 +514,7 @@ private SettingsModule validateSettings(Settings envSettings, Settings settings,
        // so we might be late here already
        SettingsModule settingsModule = new SettingsModule(
            settings,
-            additionalSettings,
+            additionalSettings.stream().distinct().collect(Collectors.toList()),


Yes, I don't think a change to this file is needed.

martijnvg · 2024-09-26T05:19:33Z

x-pack/plugin/core/template-resources/src/main/resources/logs@settings-logsdb.json

@@ -0,0 +1,26 @@
+{


why is this template added?

The new logs@settings-logsdb template is required because we can have two cases:

"mode": "logsdb" when logsdb is enabled

no index.mode at all. In LogsdbIndexModeSettingsProvider the logic expects to have empty/null index.mode to inject the "logsdb" index mode.

The way it is the template now always assigns index.mode a value, "logsdb" or "standard". As a result injection would never work in LogsdbIndexModeSettingsProvider.

I believe if this setting does not work in StackTemplateRegistry is it better to remove it...despite requiring more work...especially because some tests might rely on it. If you agree I would do the removal in a separate PR to keep the scope of this one a bit more limited.

Ok, so we said we temporarily add this template file so the xpack.stack.template.logsdb.index.mode template variable can be removed. Otherwise standard would always be set as index mode and there is no way to know whether this was set specifically or a default.

After this PR is merged we will remove the cluster.logsdb.enable setting logic and this templayte from the stack module. Which makes reviewing that change easier to review.

x-pack/plugin/logsdb/src/main/java/org/elasticsearch/xpack/logsdb/LogsDBPlugin.java

martijnvg · 2024-09-26T05:29:38Z

...gin/logsdb/src/main/java/org/elasticsearch/xpack/logsdb/LogsdbIndexModeSettingsProvider.java

+    }
+
+    private IndexMode resolveIndexMode(final String mode) {
+        return mode == null


I don't think there is a need here to use streams api. Maybe just do:

IndexMode indexMode = mode != null ? Enum.valueOf(IndexMode.class, mode(Locale.ROOT)) : null;

Since we read mode variable from settings, it can only have valid values.

martijnvg · 2024-09-26T05:30:56Z

...gin/logsdb/src/main/java/org/elasticsearch/xpack/logsdb/LogsdbIndexModeSettingsProvider.java

+        if (composableIndexTemplate == null) {
+            return false;
+        }
+        return composableIndexTemplate.composedOf()


Maybe replace this with a for loop?

martijnvg · 2024-09-26T05:32:08Z

...gin/logsdb/src/main/java/org/elasticsearch/xpack/logsdb/LogsdbIndexModeSettingsProvider.java

+    }
+
+    public void updateClusterIndexModeLogsdbEnabled(boolean isLogsdbEnabled) {
+        logger.debug("LogsDB " + (isLogsdbEnabled ? "enabled" : "disabled"));


Maybe remove this debug log statement? The setting infrastructure already emits an info log when cluster settings are updated.

This reverts commit 2cd6a50.

martijnvg · 2024-09-26T08:08:18Z

x-pack/plugin/stack/src/main/java/org/elasticsearch/xpack/stack/StackPlugin.java

@@ -14,6 +14,8 @@
 import java.util.Collection;
 import java.util.List;

+import static org.elasticsearch.xpack.cluster.settings.ClusterSettings.CLUSTER_LOGSDB_ENABLED;


maybe undo this change? It isn't needed and reduce that files this PR touches.

If I undo this I need to register the setting in LogsDBPlugin...is that what you are asking? I guess so if the plan is to remove it from StackTemplateRegistry.

x-pack/plugin/logsdb/src/main/java/org/elasticsearch/xpack/logsdb/LogsDBPlugin.java

martijnvg · 2024-09-26T08:11:20Z

...gin/logsdb/src/main/java/org/elasticsearch/xpack/logsdb/LogsdbIndexModeSettingsProvider.java

+            return Settings.EMPTY;
+        }
+
+        final IndexMode indexMode = resolveIndexMode(settings.get("index.mode"));


maybe use IndexSetting.MODE.getKey() instead of index.mode directly?

martijnvg

I think there are three outstanding comments, otherwise LGTM.

martijnvg · 2024-09-26T08:23:42Z

...ogsdb/src/test/java/org/elasticsearch/xpack/logsdb/LogsdbIndexModeSettingsProviderTests.java

+            List.of(new CompressedXContent(DEFAULT_MAPPING))
+        );
+
+        assertIndexMode(additionalIndexSettings, null);


In this case the returned settings should be empty. Maybe assert whether additionalIndexSettings.isEmpty() is true instead?

Maybe all assertIndexMode(additionalIndexSettings, null); imstances can be replaced with assertTrue(additionalIndexSettings.isEmpty())

martijnvg · 2024-09-26T08:28:25Z

...gin/logsdb/src/main/java/org/elasticsearch/xpack/logsdb/LogsdbIndexModeSettingsProvider.java

+        this.isLogsdbEnabled = CLUSTER_LOGSDB_ENABLED.get(settings);
+    }
+
+    public void updateClusterIndexModeLogsdbEnabled(boolean isLogsdbEnabled) {


This methods visibility can also be package protected?

martijnvg · 2024-09-26T08:30:43Z

...gin/logsdb/src/main/java/org/elasticsearch/xpack/logsdb/LogsdbIndexModeSettingsProvider.java

+        }
+
+        if (usesLogsAtSettingsComponentTemplate(metadata, dataStreamName) && matchesLogsPattern(dataStreamName)) {
+            return Settings.builder().put("index.mode", IndexMode.LOGSDB.getName()).build();


Replace "index.mode" with IndexSettings.MODE.getKey()`?

salvatore-campagna · 2024-09-26T08:30:56Z

Is the PR title based on a previous implementation? I do not see anything called an IndexSettingsProvider here?

LogsdbIndexModeSettingsProvider implements IndexSettingProvider...there is no previous implementation.

elasticsearchmachine · 2024-09-26T12:45:15Z

💚 Backport successful

Status	Branch	Result
✅	8.x

…stic#113505) Here we introduce a new implementation of `IndexSettingProvider` whose goal is to "inject" the `index.mode` setting with value `logsdb` when a cluster setting `cluster.logsdb.enabled` is `true`. We also make sure that: * the existing `index.mode` is not set * the datastream name matches the `logs-*-*` pattern * `logs@settings` component template is used

…3505) (#113594) Here we introduce a new implementation of `IndexSettingProvider` whose goal is to "inject" the `index.mode` setting with value `logsdb` when a cluster setting `cluster.logsdb.enabled` is `true`. We also make sure that: * the existing `index.mode` is not set * the datastream name matches the `logs-*-*` pattern * `logs@settings` component template is used

After introducing #113505 we need to remove `cluster.logsdb.enabled` from `StackPlugin` and `StackTemplateRegistry`.

feature: introduce an IndexSettingsProvider to inject logsdb inde mode

57896c2

salvatore-campagna added >non-issue auto-backport-and-merge Automatically create backport pull requests and merge when ready :StorageEngine/Logs You know, for Logs v8.16.0 v9.0.0 labels Sep 25, 2024

salvatore-campagna requested a review from martijnvg September 25, 2024 07:08

salvatore-campagna self-assigned this Sep 25, 2024

salvatore-campagna commented Sep 25, 2024

View reviewed changes

martijnvg reviewed Sep 25, 2024

View reviewed changes

salvatore-campagna added 2 commits September 25, 2024 10:06

fix: use existing Regex.simpleMatch

083fa10

fix: move cluster.logsdb.enabled setting

ac41457

felixbarny reviewed Sep 25, 2024

View reviewed changes

x-pack/plugin/core/src/main/java/org/elasticsearch/datastreams/logsdb/PatternMatcher.java Outdated Show resolved Hide resolved

salvatore-campagna added 17 commits September 25, 2024 10:31

fix: remove dependency from ClusterService

7cc9a03

fix: some improvements

899a6ed

fix: extract LOGS_PATTERN constant

d39dea0

fix: inject only if index.mode is not set

1223d7f

fix: call set and early exit on index.mode not set

d9fadc0

fix: remove unused method

0062e2b

fix: xtract checking for logs@settings in a method

44e0411

fix: do not default to standard

8b7e899

fix: some more improvements

12e1aaf

fix: let's do some less work

0a93ac0

nit: code style

01e7de8

fix: final

306ca7d

fix: check only data streams

5c952ad

fix: less word to do

165ebe4

fix: let's do regex match only if required

fab29f1

fix: multiple '-' are accepted

4068659

nit: clusterService

fbfb9da

test: logs@custom override

6560fc9

salvatore-campagna requested a review from a team as a code owner September 25, 2024 22:43

rjernst reviewed Sep 26, 2024

View reviewed changes

martijnvg reviewed Sep 26, 2024

View reviewed changes

salvatore-campagna added 4 commits September 26, 2024 09:13

fix: register cluster.logsdb.enabled once

16c8cbd

fix: move tests from modules:data-streams to x-pack:logsdb

2cd6a50

Revert "fix: move tests from modules:data-streams to x-pack:logsdb"

56358e2

This reverts commit 2cd6a50.

fix: replace stream with loop

bcd3d08

martijnvg reviewed Sep 26, 2024

View reviewed changes

salvatore-campagna added 4 commits September 26, 2024 10:11

fix: replace stream with loop

71900f4

fix: remove logger

3b09f28

fix: use IndexSettings.MODE.getKey()

242a166

fix: register CLUSTER_LOGSDB_ENABLED in LogsDBPlugin

255e7d5

martijnvg approved these changes Sep 26, 2024

View reviewed changes

martijnvg reviewed Sep 26, 2024

View reviewed changes

salvatore-campagna added 5 commits September 26, 2024 10:48

fix: some review comments

dee37a4

fix: use IndexSetting.MODE and IndexMode

8627254

test: time_series with non-allowed sorting template composition

2015481

nit: explain test failure

b28d01a

Merge branch 'main' into feature/logsdb-index-settings-provider

358fda2

salvatore-campagna merged commit e2281a1 into elastic:main Sep 26, 2024
15 checks passed

salvatore-campagna mentioned this pull request Sep 26, 2024

[8.x] Introduce an IndexSettingsProvider to inject logsdb index mode (#113505) #113594

Merged

salvatore-campagna mentioned this pull request Sep 26, 2024

Remove cluster.logsdb.enabled setting from StackPlugin #113602

Closed

salvatore-campagna mentioned this pull request Sep 26, 2024

Remove cluster.logsdb.enabled from StackPlugin #113616

Merged

salvatore-campagna added a commit that referenced this pull request Sep 30, 2024

Remove cluster.logsdb.enabled from StackPlugin (#113616)

487c556

After introducing #113505 we need to remove `cluster.logsdb.enabled` from `StackPlugin` and `StackTemplateRegistry`.

salvatore-campagna mentioned this pull request Oct 3, 2024

[APM] Internal Server Error returns when requesting service errors elastic/kibana#194841

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce an `IndexSettingsProvider` to inject logsdb index mode #113505

Introduce an `IndexSettingsProvider` to inject logsdb index mode #113505

salvatore-campagna commented Sep 25, 2024 •

edited

Loading

salvatore-campagna Sep 25, 2024

martijnvg Sep 25, 2024

salvatore-campagna Sep 25, 2024 •

edited

Loading

martijnvg Sep 25, 2024

martijnvg left a comment

martijnvg Sep 25, 2024

martijnvg Sep 25, 2024

salvatore-campagna Sep 25, 2024 •

edited

Loading

martijnvg Sep 25, 2024

martijnvg Sep 25, 2024

salvatore-campagna Sep 25, 2024

martijnvg Sep 25, 2024

martijnvg Sep 25, 2024

rjernst left a comment

rjernst Sep 26, 2024

martijnvg Sep 26, 2024

salvatore-campagna Sep 26, 2024 •

edited

Loading

martijnvg Sep 26, 2024

martijnvg Sep 26, 2024

martijnvg Sep 26, 2024 •

edited

Loading

salvatore-campagna Sep 26, 2024 •

edited

Loading

martijnvg Sep 26, 2024

martijnvg Sep 26, 2024

martijnvg Sep 26, 2024

martijnvg Sep 26, 2024

martijnvg Sep 26, 2024

salvatore-campagna Sep 26, 2024 •

edited

Loading

martijnvg Sep 26, 2024

martijnvg left a comment •

edited

Loading

martijnvg Sep 26, 2024

martijnvg Sep 26, 2024

martijnvg Sep 26, 2024

salvatore-campagna commented Sep 26, 2024

elasticsearchmachine commented Sep 26, 2024


		import java.util.regex.Pattern;

		public class PatternMatcher {

Introduce an IndexSettingsProvider to inject logsdb index mode #113505

Introduce an IndexSettingsProvider to inject logsdb index mode #113505

Conversation

salvatore-campagna commented Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvatore-campagna Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvatore-campagna Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvatore-campagna Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

salvatore-campagna Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvatore-campagna Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvatore-campagna commented Sep 26, 2024

elasticsearchmachine commented Sep 26, 2024

💚 Backport successful

Introduce an `IndexSettingsProvider` to inject logsdb index mode #113505

Introduce an `IndexSettingsProvider` to inject logsdb index mode #113505

salvatore-campagna commented Sep 25, 2024 •

edited

Loading

salvatore-campagna Sep 25, 2024 •

edited

Loading

salvatore-campagna Sep 25, 2024 •

edited

Loading

salvatore-campagna Sep 26, 2024 •

edited

Loading

martijnvg Sep 26, 2024 •

edited

Loading

salvatore-campagna Sep 26, 2024 •

edited

Loading

salvatore-campagna Sep 26, 2024 •

edited

Loading

martijnvg left a comment •

edited

Loading