Inject the `host.name` field mapping only if required for `logsdb` index mode #114573

salvatore-campagna · 2024-10-11T08:10:54Z

Here we check for the existence of a host.name field in index sort settings
when the index mode is logsdb and decide to inject the field in the mapping
depending on whether it exists or not. By default host.name is required for
sorting in LogsDB. This reduces the chances for errors at mapping or template
composition time as a result of injecting the host.name field only if strictly
required. A user who wants to override index sort settings without including
a host.name field would be able to do so without finding an additional
host.name field in the mappings (injected automatically). If users override the
sort settings and a host.name field is not included we don't need
to inject such field since sorting does not require it anymore.

As a result of this change we have the following:

the user does not provide any index sorting configuration: we are responsible for injecting the default sort fields and their mapping (for logsdb)
the user explicitly provides non-empty index sorting configuration: the user is also responsible for providing correct mappings and we do not modify index sorting or mappings

Note also that all sort settings index.sort.* are final which means doing this
check once, when mappings are merged at template composition time, is enough.

salvatore-campagna · 2024-10-11T10:51:42Z

@elasticmachine update branch

salvatore-campagna · 2024-10-11T13:17:01Z

@elasticmachine update branch

salvatore-campagna · 2024-10-11T17:49:31Z

@elasticmachine update branch

salvatore-campagna · 2024-10-11T20:40:05Z

@elasticmachine update branch

salvatore-campagna · 2024-10-14T09:02:23Z

@elasticmachine update branch

elasticsearchmachine · 2024-10-14T16:18:35Z

Pinging @elastic/es-storage-engine (Team:StorageEngine)

dnhatn

LGTM. Thanks @salvatore-campagna.

dnhatn · 2024-10-14T16:46:10Z

server/src/main/java/org/elasticsearch/index/IndexMode.java

-            return DEFAULT_LOGS_TIMESTAMP_MAPPING;
+        public CompressedXContent getDefaultMapping(final IndexSettings indexSettings) {
+            final IndexSortConfig.FieldSortSpec[] fieldSortSpecs = indexSettings.getIndexSortConfig().sortSpecs;
+            if (fieldSortSpecs == null || fieldSortSpecs.length == 0) {


I think we don't need this check because the index sort for logsdb should already be injected in the IndexSortConfig constructor.

maybe we need this for the case if index.sort is set to null?

I wanted to be extra cautious and make sure we always inject something if there is no sort configuration...but I think this is needed in case there is no sort configuration. EVen if it might not make sense a user can specify empty sort config.

I will add a test where index sort config is an empty array

dnhatn · 2024-10-14T16:48:20Z

server/src/main/java/org/elasticsearch/index/IndexMode.java

    }

-    public static final CompressedXContent DEFAULT_TIME_SERIES_TIMESTAMP_MAPPING;
+    private static CompressedXContent createMapping(boolean includeHostName) throws IOException {


nit: Could we add default to the method name? I think createMapping is quite generic and might cause confusion.

martijnvg

Left a testing comment, LGTM otherwise.

martijnvg · 2024-10-15T06:39:20Z

server/src/main/java/org/elasticsearch/index/IndexMode.java

-            return DEFAULT_LOGS_TIMESTAMP_MAPPING;
+        public CompressedXContent getDefaultMapping(final IndexSettings indexSettings) {
+            final IndexSortConfig.FieldSortSpec[] fieldSortSpecs = indexSettings.getIndexSortConfig().sortSpecs;
+            if (fieldSortSpecs == null || fieldSortSpecs.length == 0) {


maybe we need this for the case if index.sort is set to null?

martijnvg · 2024-10-15T06:59:48Z

...ck/plugin/logsdb/src/yamlRestTest/resources/rest-api-spec/test/30_logsdb_default_mapping.yml

+  - match: { .$backing_index.mappings.properties.agent.properties.id.type: keyword }
+  - match: { .$backing_index.mappings.properties.host.properties.name.type: text }
+  - match: { .$backing_index.mappings.properties.host.properties.id.type: long }
+  - match: { .$backing_index.mappings.properties.host.properties.name.ignore_above: null }


host.* fields are introduced here via dynamic mapping updates, right? Maybe remove host.name field from the data and assert that no host.name exist?

martijnvg · 2024-10-15T07:00:38Z

...ck/plugin/logsdb/src/yamlRestTest/resources/rest-api-spec/test/30_logsdb_default_mapping.yml


  - match: { error.type: "illegal_argument_exception" }
  - match: { error.reason: "composable template [logsdb-index-template] template after composition with component templates [logsdb-mappings] is invalid" }
+


Maybe also introduce a test case where we only sort by @timestamp?

Right...some integration like APM does it.

kkrik-es · 2024-10-15T07:05:50Z

server/src/main/java/org/elasticsearch/index/IndexMode.java

+            }
+
+            for (final IndexSortConfig.FieldSortSpec fieldSortSpec : fieldSortSpecs) {
+                if (HOST_NAME.equals(fieldSortSpec.field)) {


What if the user provides an override with "hostname" or "attributes.host.name"?

I think we should just skip injecting anything other than timestamp when index sorting is explicitly set.

I think I tested this right? Can you be more precise...what do you mean with override?

An override is an explicit setting of index sorting. I'm trying to understand why this loop is needed, is it to pick the default setting from IndexSortConfig, as mentioned above?

The loop is required because the sort field is normally a list of fields...all index.sort.* settings are used as "parallel" arrays. The host.name or any other field could be the first, second,...you can look at IndexSortConfig.

This method is used to merge default mappings with the result of template composition...which happens taking into account priorities and fields defined by our standard templates or custom templates. We need to provide them anyway because then whether they are used or not will depend on the composition logic. Also, injecting the host.name field is done already if no sort configuration is provided by the user. There is a difference between injecting a sort field and injecting its mapping.

I will go for the original plan got the host.name field...we can't really detect in the getDefault method of IndexMode if the field was added to index sorting by us (injecting it in IndexSortConfig or if it was the user explicitly adding it by means of index.sort.field .
In IndexSortConfig we already inject the host.name field if the sort config is empty...later on in getDefault we just make sure the field is mapped because otherwise sorting would not be possible

I'm fine either way. But could we do something like this?

if (indexSettings.getSettings().hasValue(IndexSortConfig.INDEX_SORT_FIELD_SETTING.getKey())) { return DEFAULT_LOGS_TIMESTAMP_MAPPING; } else { return DEFAULT_LOGS_TIMESTAMP_MAPPING_WITH_HOSTNAME; }

Seems like that works. We don't actually inject an index setting in LogsDB, we just apply a default in IndexSortConfig.

I'm fine either way. But could we do something like this?

if (indexSettings.getSettings().hasValue(IndexSortConfig.INDEX_SORT_FIELD_SETTING.getKey())) { return DEFAULT_LOGS_TIMESTAMP_MAPPING; } else { return DEFAULT_LOGS_TIMESTAMP_MAPPING_WITH_HOSTNAME; }

I think that would not work if we have something like

settings: index: sort.field: [ ] sort.order: [ ] mode: logsdb

I imagine that this is kind of a corner case...but I would be careful just in case this is automatically generated in some way and an empty array is used for some reason.

felixbarny · 2024-10-16T06:12:02Z

I hope this can be back ported to 8.16.0 as it is causing issues with being able to pass through resource.attributes.host.name for OTel.

salvatore-campagna · 2024-10-16T09:57:37Z

@elasticmachine update branch

salvatore-campagna · 2024-10-16T12:49:32Z

@elasticmachine update branch

elasticsearchmachine · 2024-10-16T14:13:20Z

💔 Backport failed

The backport operation could not be completed due to the following error:

An unexpected error occurred when attempting to backport this PR.

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 114573

…dex mode (elastic#114573) Here we check for the existence of a `host.name` field in index sort settings when the index mode is `logsdb` and decide to inject the field in the mapping depending on whether it exists or not. By default `host.name` is required for sorting in LogsDB. This reduces the chances for errors at mapping or template composition time as a result of injecting the `host.name` field only if strictly required. A user who wants to override index sort settings without including a `host.name` field would be able to do so without finding an additional `host.name` field in the mappings (injected automatically). If users override the sort settings and a `host.name` field is not included we don't need to inject such field since sorting does not require it anymore. As a result of this change we have the following: * the user does not provide any index sorting configuration: we are responsible for injecting the default sort fields and their mapping (for `logsdb`) * the user explicitly provides non-empty index sorting configuration: the user is also responsible for providing correct mappings and we do not modify index sorting or mappings Note also that all sort settings `index.sort.*` are `final` which means doing this check once, when mappings are merged at template composition time, is enough. (cherry picked from commit 9bf6e3b)

salvatore-campagna · 2024-10-16T14:15:47Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Questions ?

Please refer to the Backport tool documentation

…dex mode (elastic#114573) Here we check for the existence of a `host.name` field in index sort settings when the index mode is `logsdb` and decide to inject the field in the mapping depending on whether it exists or not. By default `host.name` is required for sorting in LogsDB. This reduces the chances for errors at mapping or template composition time as a result of injecting the `host.name` field only if strictly required. A user who wants to override index sort settings without including a `host.name` field would be able to do so without finding an additional `host.name` field in the mappings (injected automatically). If users override the sort settings and a `host.name` field is not included we don't need to inject such field since sorting does not require it anymore. As a result of this change we have the following: * the user does not provide any index sorting configuration: we are responsible for injecting the default sort fields and their mapping (for `logsdb`) * the user explicitly provides non-empty index sorting configuration: the user is also responsible for providing correct mappings and we do not modify index sorting or mappings Note also that all sort settings `index.sort.*` are `final` which means doing this check once, when mappings are merged at template composition time, is enough.

…dex mode (#114573) (#114916) Here we check for the existence of a `host.name` field in index sort settings when the index mode is `logsdb` and decide to inject the field in the mapping depending on whether it exists or not. By default `host.name` is required for sorting in LogsDB. This reduces the chances for errors at mapping or template composition time as a result of injecting the `host.name` field only if strictly required. A user who wants to override index sort settings without including a `host.name` field would be able to do so without finding an additional `host.name` field in the mappings (injected automatically). If users override the sort settings and a `host.name` field is not included we don't need to inject such field since sorting does not require it anymore. As a result of this change we have the following: * the user does not provide any index sorting configuration: we are responsible for injecting the default sort fields and their mapping (for `logsdb`) * the user explicitly provides non-empty index sorting configuration: the user is also responsible for providing correct mappings and we do not modify index sorting or mappings Note also that all sort settings `index.sort.*` are `final` which means doing this check once, when mappings are merged at template composition time, is enough. (cherry picked from commit 9bf6e3b) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

felixbarny · 2024-10-16T16:38:37Z

Unfortunately, the back port was merged into the 8.x branch after the 8.16 branch has been created. Could we create another back port for the 8.16 branch, given that this fixes the bug of not being able to use the host.name passthrough for OTel?

…dex mode (elastic#114573) Here we check for the existence of a `host.name` field in index sort settings when the index mode is `logsdb` and decide to inject the field in the mapping depending on whether it exists or not. By default `host.name` is required for sorting in LogsDB. This reduces the chances for errors at mapping or template composition time as a result of injecting the `host.name` field only if strictly required. A user who wants to override index sort settings without including a `host.name` field would be able to do so without finding an additional `host.name` field in the mappings (injected automatically). If users override the sort settings and a `host.name` field is not included we don't need to inject such field since sorting does not require it anymore. As a result of this change we have the following: * the user does not provide any index sorting configuration: we are responsible for injecting the default sort fields and their mapping (for `logsdb`) * the user explicitly provides non-empty index sorting configuration: the user is also responsible for providing correct mappings and we do not modify index sorting or mappings Note also that all sort settings `index.sort.*` are `final` which means doing this check once, when mappings are merged at template composition time, is enough. (cherry picked from commit 9bf6e3b)

salvatore-campagna · 2024-10-16T17:38:04Z

💚 All backports created successfully

Status	Branch	Result
✅	8.16

Questions ?

Please refer to the Backport tool documentation

…dex mode (#114573) (#114936) Here we check for the existence of a `host.name` field in index sort settings when the index mode is `logsdb` and decide to inject the field in the mapping depending on whether it exists or not. By default `host.name` is required for sorting in LogsDB. This reduces the chances for errors at mapping or template composition time as a result of injecting the `host.name` field only if strictly required. A user who wants to override index sort settings without including a `host.name` field would be able to do so without finding an additional `host.name` field in the mappings (injected automatically). If users override the sort settings and a `host.name` field is not included we don't need to inject such field since sorting does not require it anymore. As a result of this change we have the following: * the user does not provide any index sorting configuration: we are responsible for injecting the default sort fields and their mapping (for `logsdb`) * the user explicitly provides non-empty index sorting configuration: the user is also responsible for providing correct mappings and we do not modify index sorting or mappings Note also that all sort settings `index.sort.*` are `final` which means doing this check once, when mappings are merged at template composition time, is enough. (cherry picked from commit 9bf6e3b)

…dex mode (elastic#114573) Here we check for the existence of a `host.name` field in index sort settings when the index mode is `logsdb` and decide to inject the field in the mapping depending on whether it exists or not. By default `host.name` is required for sorting in LogsDB. This reduces the chances for errors at mapping or template composition time as a result of injecting the `host.name` field only if strictly required. A user who wants to override index sort settings without including a `host.name` field would be able to do so without finding an additional `host.name` field in the mappings (injected automatically). If users override the sort settings and a `host.name` field is not included we don't need to inject such field since sorting does not require it anymore. As a result of this change we have the following: * the user does not provide any index sorting configuration: we are responsible for injecting the default sort fields and their mapping (for `logsdb`) * the user explicitly provides non-empty index sorting configuration: the user is also responsible for providing correct mappings and we do not modify index sorting or mappings Note also that all sort settings `index.sort.*` are `final` which means doing this check once, when mappings are merged at template composition time, is enough.

feature: inject the host.name field only if required for sorting

be2c6b1

salvatore-campagna added >non-issue :StorageEngine/Logs You know, for Logs labels Oct 11, 2024

salvatore-campagna self-assigned this Oct 11, 2024

elasticsearchmachine added the v9.0.0 label Oct 11, 2024

salvatore-campagna changed the title ~~Inject the host.name field only if required for sorting for logsdb index mode.~~ Inject the host.name field only if required for sorting when using logsdb index mode. Oct 11, 2024

salvatore-campagna added 2 commits October 11, 2024 10:13

Merge branch 'main' into feature/inject-host-name-if-required

e3083bf

refactor: static mapping cretion for logsdb and time_series

192da05

Merge branch 'main' into feature/inject-host-name-if-required

42cc978

Merge branch 'main' into feature/inject-host-name-if-required

176cae4

Merge branch 'main' into feature/inject-host-name-if-required

a267dbd

Merge branch 'main' into feature/inject-host-name-if-required

6c76b56

elasticmachine and others added 4 commits October 14, 2024 11:02

Merge branch 'main' into feature/inject-host-name-if-required

14c1736

fix: remove duplicate test

59d0837

test: add a few more tests

5cd2dac

test: text fields do not have doc values

bb585ea

salvatore-campagna marked this pull request as ready for review October 14, 2024 16:18

salvatore-campagna requested review from kkrik-es, lkts and martijnvg October 14, 2024 16:18

elasticsearchmachine added the Team:StorageEngine label Oct 14, 2024

dnhatn approved these changes Oct 14, 2024

View reviewed changes

martijnvg approved these changes Oct 15, 2024

View reviewed changes

kkrik-es reviewed Oct 15, 2024

View reviewed changes

lkts approved these changes Oct 15, 2024

View reviewed changes

martijnvg mentioned this pull request Oct 16, 2024

OTel mappings: allow resource.attributes.host.name to pass through #114849

Closed

refactor: getDefault method and add some more tests

046f208

salvatore-campagna requested a review from a team as a code owner October 16, 2024 09:28

nit: rename createMapping to createDefaultMapping

1ca0351

salvatore-campagna changed the title ~~Inject the host.name field only if required for sorting when using logsdb index mode.~~ Inject the host.name field mapping only if required when using logsdb index mode. Oct 16, 2024

salvatore-campagna changed the title ~~Inject the host.name field mapping only if required when using logsdb index mode.~~ Inject the host.name field mapping only if required for logsdb index mode Oct 16, 2024

Merge branch 'main' into feature/inject-host-name-if-required

60b9b8a

Merge branch 'main' into feature/inject-host-name-if-required

6f897a8

salvatore-campagna added v8.16.0 auto-backport Automatically create backport pull requests when merged labels Oct 16, 2024

salvatore-campagna merged commit 9bf6e3b into elastic:main Oct 16, 2024
16 checks passed

elasticsearchmachine added the backport pending label Oct 16, 2024

salvatore-campagna mentioned this pull request Oct 16, 2024

[8.x] Inject the host.name field mapping only if required for logsdb index mode (#114573) #114916

Merged

salvatore-campagna mentioned this pull request Oct 16, 2024

[8.16] Inject the host.name field mapping only if required for logsdb index mode (#114573) #114936

Merged


		- match: { error.type: "illegal_argument_exception" }
		- match: { error.reason: "composable template [logsdb-index-template] template after composition with component templates [logsdb-mappings] is invalid" }

Inject the host.name field mapping only if required for logsdb index mode #114573

Inject the host.name field mapping only if required for logsdb index mode #114573

Uh oh!

Conversation

salvatore-campagna commented Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

salvatore-campagna commented Oct 11, 2024

Uh oh!

salvatore-campagna commented Oct 11, 2024

Uh oh!

salvatore-campagna commented Oct 11, 2024

Uh oh!

salvatore-campagna commented Oct 11, 2024

Uh oh!

salvatore-campagna commented Oct 14, 2024

Uh oh!

elasticsearchmachine commented Oct 14, 2024

Uh oh!

dnhatn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salvatore-campagna Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salvatore-campagna Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salvatore-campagna Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixbarny commented Oct 16, 2024

Uh oh!

salvatore-campagna commented Oct 16, 2024

Uh oh!

salvatore-campagna commented Oct 16, 2024

Uh oh!

Uh oh!

elasticsearchmachine commented Oct 16, 2024

💔 Backport failed

Uh oh!

salvatore-campagna commented Oct 16, 2024

💚 All backports created successfully

Questions ?

Inject the `host.name` field mapping only if required for `logsdb` index mode #114573

Inject the `host.name` field mapping only if required for `logsdb` index mode #114573

salvatore-campagna commented Oct 11, 2024 •

edited

Loading

salvatore-campagna Oct 15, 2024 •

edited

Loading

salvatore-campagna Oct 15, 2024 •

edited

Loading

salvatore-campagna Oct 15, 2024 •

edited

Loading