-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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 a setting controlling the activation of the logs
index mode in logs@settings
#109025
Introduce a setting controlling the activation of the logs
index mode in logs@settings
#109025
Conversation
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
TEMPLATE_VERSION_VARIABLE, | ||
ADDITIONAL_TEMPLATE_VARIABLES | ||
); | ||
final Map<String, ComponentTemplate> updatedComponentTemplates = new HashMap<>(COMPONENT_TEMPLATE_CONFIGS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to do this because the original map is immutable and I need access to node settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe create two static immutable maps and return the right one based on the setting?
public static ElasticsearchCluster cluster = ElasticsearchCluster.local() | ||
.module("x-pack-stack") | ||
.module("data-streams") | ||
.setting("stack.templates.enabled", "true") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not really be required since the default is "true"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but maybe this is disabled somewhere else in this test cluster setup? It defaults to true, so something must be setting it to false? What do you see when you debug StackPlugin
? Is this plugin loaded btw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I debug the test I don't see it going into StackTemplateRegistry
constructor where the setting is evaluated...so I am not sure that is the issue. I think something is missing to actually use the registry.
Once I have the existing test working I will add a test that enables and disables the setting too. |
Hi @salvatore-campagna, I've created a changelog YAML for you. |
logs
index modelogs
index mode in logs@settings
"@timestamp": { | ||
"type": "date" | ||
}, | ||
"host.name": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice that I added added this specific logs@mapping-logsdb.json to make sure a host.name
field is created and no error is thrown if default sorting on host.name
and @timestamp
is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? We can't assume all data streams have it, logsdb must also work when it is not set.
private final ClusterService clusterService; | ||
private final FeatureService featureService; | ||
private volatile boolean stackTemplateEnabled; | ||
|
||
private volatile boolean logsIndexModeTemplateEnabled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be a final setting, since this setting isn't a dynamic setting
@@ -249,10 +262,97 @@ protected List<LifecyclePolicy> getLifecyclePolicies() { | |||
} | |||
} | |||
COMPONENT_TEMPLATE_CONFIGS = Map.copyOf(componentTemplates); | |||
|
|||
final Map<String, ComponentTemplate> logsdbComponentTemplates = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only think we need to load different ComponentTemplate
instances for logs mappings and settings? These will replace the non logsdb mappings and settings. No need to to load all component templates twice, the logsdb setting is non dynamic, so at node startup we can determine what needs to be loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.logsIndexModeTemplateEnabled = CLUSTER_LOGSDB_ENABLED.get(nodeSettings);
nodeSettings
are only available when the construction of StackTemplateRegistry
happens...is there a way to read settings from a static context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also replacing is not possible...the map in an immutable as I said in one of my previous comments...so no way to replace entries into the map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, I see. The issue here is that the component templates map is loaded when the StackTemplateRegistry
class is loaded. At that time we don't have access to node settings. If the map for component template was initialized in the constructor we can have access to it (adding a Settings
parameter is supported).
Then let's leave this as is.
{ | ||
"template": { | ||
"settings": { | ||
"index": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need an additional template? Why not use a template variable like we do for version for the index mode?
Having more templates means more templates to keep in sync / more confusion.
Here we introduce a
cluster.logsdb.enabled
setting that controls activation of the newlogs
index mode inlogs@settings
. The setting default value isfalse
and prevents usage of the new index mode by default inlogs@settings
. We also changehostname
tohost.name
as the default field used for sorting (other than@timestamp
) and include it inlogs@mappings
.After addressing relevant issues in synthetic source including:
ignore_malformed
for all non-string fields using synthetic source #106483doc_values: false
#109546we will also add a feature flag controlling the default value for the
cluster.logsdb.enabled
setting to betrue
for snapshot builds. This way thelogs
index mode will be the default for logs in deploymentsusing an Elasticsearch snapshot build.
This mechanism will allow us to enable the index mode using the feature flag or directly
via the setting, giving us options when it comes to overriding activation behaviour depending
on the Elasticsearch deployment environment and version.
Resolves #108762