Skip to content
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.

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

Introduce an IndexSettingsProvider to inject logsdb index mode #113505

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
57896c2
feature: introduce an IndexSettingsProvider to inject logsdb inde mode
salvatore-campagna Sep 25, 2024
083fa10
fix: use existing Regex.simpleMatch
salvatore-campagna Sep 25, 2024
ac41457
fix: move cluster.logsdb.enabled setting
salvatore-campagna Sep 25, 2024
7cc9a03
fix: remove dependency from ClusterService
salvatore-campagna Sep 25, 2024
899a6ed
fix: some improvements
salvatore-campagna Sep 25, 2024
d39dea0
fix: extract LOGS_PATTERN constant
salvatore-campagna Sep 25, 2024
1223d7f
fix: inject only if index.mode is not set
salvatore-campagna Sep 25, 2024
d9fadc0
fix: call set and early exit on index.mode not set
salvatore-campagna Sep 25, 2024
0062e2b
fix: remove unused method
salvatore-campagna Sep 25, 2024
44e0411
fix: xtract checking for logs@settings in a method
salvatore-campagna Sep 25, 2024
8b7e899
fix: do not default to standard
salvatore-campagna Sep 25, 2024
12e1aaf
fix: some more improvements
salvatore-campagna Sep 25, 2024
0a93ac0
fix: let's do some less work
salvatore-campagna Sep 25, 2024
01e7de8
nit: code style
salvatore-campagna Sep 25, 2024
306ca7d
fix: final
salvatore-campagna Sep 25, 2024
5c952ad
fix: check only data streams
salvatore-campagna Sep 25, 2024
165ebe4
fix: less word to do
salvatore-campagna Sep 25, 2024
fab29f1
fix: let's do regex match only if required
salvatore-campagna Sep 25, 2024
4068659
fix: multiple '-' are accepted
salvatore-campagna Sep 25, 2024
fbfb9da
nit: clusterService
salvatore-campagna Sep 25, 2024
b9e0d39
fix: simple match does not work
salvatore-campagna Sep 25, 2024
c67984c
test: test the IndexSettingsProvider
salvatore-campagna Sep 25, 2024
8711426
fix: export package
salvatore-campagna Sep 25, 2024
214046e
fix: use Regex.simpleMatch
salvatore-campagna Sep 25, 2024
afe4380
test: logs pattern matching case sensitivity
salvatore-campagna Sep 25, 2024
0df32fb
Merge branch 'main' into feature/logsdb-index-settings-provider
salvatore-campagna Sep 25, 2024
5b12503
fix: move to logsdb xpack plugin
salvatore-campagna Sep 25, 2024
f15652e
fix: remove non-necessary export
salvatore-campagna Sep 25, 2024
52cff88
fix: put CLUSTER_LOGSDB_ENABLED setting in a common place
salvatore-campagna Sep 25, 2024
caecbb3
fix: rename class
salvatore-campagna Sep 25, 2024
feb79f9
fix: import the right setting
salvatore-campagna Sep 25, 2024
ccc0ed2
Merge branch 'main' into feature/logsdb-index-settings-provider
salvatore-campagna Sep 25, 2024
eff012b
fix: simplify some code
salvatore-campagna Sep 25, 2024
0fed418
fix: return the IndexSettingProvider
salvatore-campagna Sep 25, 2024
5f230cc
fix: return CLUSTER_LOGSDB_ENABLED setting
salvatore-campagna Sep 25, 2024
5969e12
final package private
salvatore-campagna Sep 25, 2024
6315c6e
Revert "fix: return CLUSTER_LOGSDB_ENABLED setting"
salvatore-campagna Sep 25, 2024
a620b66
fix: test: switching logsdb enabled
salvatore-campagna Sep 25, 2024
58d0be8
fix: deduplicate settings
salvatore-campagna Sep 25, 2024
6560fc9
test: logs@custom override
salvatore-campagna Sep 25, 2024
16c8cbd
fix: register cluster.logsdb.enabled once
salvatore-campagna Sep 26, 2024
2cd6a50
fix: move tests from modules:data-streams to x-pack:logsdb
salvatore-campagna Sep 26, 2024
56358e2
Revert "fix: move tests from modules:data-streams to x-pack:logsdb"
salvatore-campagna Sep 26, 2024
bcd3d08
fix: replace stream with loop
salvatore-campagna Sep 26, 2024
71900f4
fix: replace stream with loop
salvatore-campagna Sep 26, 2024
3b09f28
fix: remove logger
salvatore-campagna Sep 26, 2024
242a166
fix: use IndexSettings.MODE.getKey()
salvatore-campagna Sep 26, 2024
255e7d5
fix: register CLUSTER_LOGSDB_ENABLED in LogsDBPlugin
salvatore-campagna Sep 26, 2024
dee37a4
fix: some review comments
salvatore-campagna Sep 26, 2024
8627254
fix: use IndexSetting.MODE and IndexMode
salvatore-campagna Sep 26, 2024
2015481
test: time_series with non-allowed sorting template composition
salvatore-campagna Sep 26, 2024
b28d01a
nit: explain test failure
salvatore-campagna Sep 26, 2024
358fda2
Merge branch 'main' into feature/logsdb-index-settings-provider
salvatore-campagna Sep 26, 2024
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/*
* 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; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

package org.elasticsearch.datastreams.logsdb;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.elasticsearch.cluster.metadata.Metadata;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.compress.CompressedXContent;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.IndexMode;
import org.elasticsearch.index.IndexSettingProvider;

import java.time.Instant;
import java.util.Arrays;
import java.util.List;

public class LogsdbIndexModeSettingsProvider implements IndexSettingProvider {
private static final Logger logger = LogManager.getLogger(LogsdbIndexModeSettingsProvider.class);
private static final PatternMatcher logsNameMatcher = PatternMatcher.forLogs();

public static final Setting<Boolean> CLUSTER_INDEX_MODE_LOGSDB_ENABLED = Setting.boolSetting(
"cluster.index.mode.logsdb.enabled",
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 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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@salvatore-campagna salvatore-campagna Sep 25, 2024

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

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.

false,
Setting.Property.Dynamic,
Setting.Property.NodeScope
);
private boolean isLogsdbEnabled;

public LogsdbIndexModeSettingsProvider(final ClusterService clusterService) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

this.isLogsdbEnabled = clusterService.getSettings().getAsBoolean(CLUSTER_INDEX_MODE_LOGSDB_ENABLED.getKey(), false);
clusterService.getClusterSettings()
.addSettingsUpdateConsumer(CLUSTER_INDEX_MODE_LOGSDB_ENABLED, this::updateClusterIndexModeLogsdbEnabled);
}

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

@Override
public Settings getAdditionalIndexSettings(
final String indexName,
final String dataStreamName,
boolean isTimeSeries,
final Metadata metadata,
final Instant resolvedAt,
final Settings settings,
final List<CompressedXContent> combinedTemplateMappings
) {
if (isLogsdbEnabled == false) {
return Settings.EMPTY;
}

final IndexMode indexMode = resolveIndexMode(settings.get("index.mode"));
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@salvatore-campagna salvatore-campagna Sep 25, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

if (IndexMode.STANDARD.equals(indexMode) && isLogsDataStreamOrIndexName(indexName, dataStreamName)) {
return Settings.builder().put("index.mode", IndexMode.LOGSDB.getName()).build();
}

return Settings.EMPTY;
}

private static boolean isLogsDataStreamOrIndexName(final String indexName, final String dataStreamName) {
return logsNameMatcher.matches(indexName) || logsNameMatcher.matches(dataStreamName);
}

private IndexMode resolveIndexMode(final String mode) {
if (mode == null) {
return IndexMode.STANDARD;
}
return Arrays.stream(IndexMode.values())
.filter(indexMode -> indexMode.getName().equals(mode))
.findFirst()
.orElse(IndexMode.STANDARD);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* 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.datastreams.logsdb;

import java.util.regex.Pattern;

public class PatternMatcher {
Copy link
Member

Choose a reason for hiding this comment

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

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)`

private final Pattern pattern;

private PatternMatcher(final Pattern pattern) {
this.pattern = pattern;
}

public boolean matches(final String value) {
return value != null && pattern.matcher(value).matches();
}

public static PatternMatcher forLogs() {
return new PatternMatcher(Pattern.compile("logs-[^-]+-[^-]+"));
felixbarny marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.elasticsearch.common.ssl.SslConfiguration;
import org.elasticsearch.common.util.BigArrays;
import org.elasticsearch.core.Booleans;
import org.elasticsearch.datastreams.logsdb.LogsdbIndexModeSettingsProvider;
import org.elasticsearch.env.Environment;
import org.elasticsearch.features.NodeFeature;
import org.elasticsearch.index.IndexSettingProvider;
Expand Down Expand Up @@ -184,6 +185,8 @@ public Void run() {
private static SetOnce<XPackLicenseState> licenseState = new SetOnce<>();
private static SetOnce<LicenseService> licenseService = new SetOnce<>();

private static SetOnce<ClusterService> clusterService = new SetOnce<>();

public XPackPlugin(final Settings settings) {
super();
// FIXME: The settings might be changed after this (e.g. from "additionalSettings" method in other plugins)
Expand Down Expand Up @@ -231,6 +234,10 @@ protected void setEpochMillisSupplier(LongSupplier epochMillisSupplier) {
XPackPlugin.epochMillisSupplier.set(epochMillisSupplier);
}

protected void setClusterService(ClusterService clusterService) {
XPackPlugin.clusterService.set(clusterService);
}

public static SSLService getSharedSslService() {
final SSLService ssl = XPackPlugin.sslService.get();
if (ssl == null) {
Expand Down Expand Up @@ -338,6 +345,7 @@ public Collection<?> createComponents(PluginServices services) {
}

setEpochMillisSupplier(services.threadPool()::absoluteTimeInMillis);
setClusterService(services.clusterService());

// It is useful to override these as they are what guice is injecting into actions
components.add(sslService);
Expand Down Expand Up @@ -482,7 +490,7 @@ public Collection<IndexSettingProvider> getAdditionalIndexSettingProviders(Index
if (DiscoveryNode.isStateless(settings)) {
return List.of();
}
return Collections.singleton(new DataTier.DefaultHotAllocationSettingProvider());
return List.of(new DataTier.DefaultHotAllocationSettingProvider(), new LogsdbIndexModeSettingsProvider(clusterService.get()));
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* 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.datastreams.logsdb;

import junit.framework.TestCase;

public class PatternMatcherTests extends TestCase {

private static final PatternMatcher matcher = PatternMatcher.forLogs();

public void testMatching() {
assertTrue(matcher.matches("logs-apache-production"));
assertTrue(matcher.matches("logs-apache-123"));
assertTrue(matcher.matches("logs-123-production"));
assertTrue(matcher.matches("logs-apache.kafka-production.eu.west"));
}

public void testNonMatching() {
assertFalse(matcher.matches(null));
assertFalse(matcher.matches(""));
assertFalse(matcher.matches("logs"));
assertFalse(matcher.matches("logs-apache"));
assertFalse(matcher.matches("standard-apache-production"));
assertFalse(matcher.matches("logs-apache-production-eu"));
}

}