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.

Sign up for GitHub

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

Jump to bottom

[Fleet] Add support for additional types for dynamic mappings #168842

Merged
merged 11 commits into from
Oct 17, 2023
Merged

[Fleet] Add support for additional types for dynamic mappings #168842

merged 11 commits into from
Oct 17, 2023

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Oct 13, 2023 •
edited
Loading

Summary

Add support for fields that generate dynamic mappings of the following types:

  • match_only_text
  • scaled_float
  • aggregate_metric_double
  • half_float
  • ip
  • flattened
  • integer (mapped as long as in other cases)
  • group (child fields are installed as dynamic mappings)

Default match_matching_type has been corrected to provide always a valid value. For
example for floats it was using float, what is invalid, for this case it is changed to double.

When the type is not known, an error is raised now instead of silently ignoring the field definition,
this helps package developers discover earlier if their mappings are not well supported.
Tested with the install_all_packages and no current package fails because of this.

Fix #168823

Checklist

Delete any items that are not applicable to this PR.

  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list

Risk Matrix

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Installation of packages with dynamic mappings with unknown or unsupported types fail. Low Low Installation of all packages tested with the install_all_packages script, and no current package fails because of this. Most cases catched earlier on Package Spec validation.

@jsoriano jsoriano self-assigned this Oct 13, 2023
@jsoriano jsoriano requested a review from a team as a code owner October 13, 2023 13:22
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Oct 13, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@jsoriano
[Fleet] Add support for additional types for dynamic mappings
0855613 
Add support for fields that generate and dynamic mapping of the
following types:
- scaled_float
- half_float
- match_only_text
- aggregate_metric_double
- ip
- integer (mapped as long as in other cases)
- group (child fields are installed as dynamic mappings)

When the type is not known, an error is raised now
instead of silently ignoring the field definition.
kpollich
kpollich approved these changes Oct 13, 2023
View reviewed changes
Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

Code + tests look good. Thanks for contributing this!

mrodm
mrodm reviewed Oct 16, 2023
View reviewed changes
x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts
dynProperties.time_series_metric = field.metric_type;
matchingType = field.object_type_mapping_type ?? 'long';
break;
case 'integer':
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be added unsigned_long too ?
It is also an allowed object_type according to package-spec:
https://github.com/elastic/package-spec/blob/a4b0a7ff139c9b85e44602caeb83d93716c1f4d8/spec/integration/data_stream/fields/fields.spec.yml#L488

x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts
@@ -258,27 +256,73 @@ function _generateMappings(
dynProperties = histogram(field);
matchingType = field.object_type_mapping_type ?? '*';
break;
case 'ip':
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some types that are not specified in the object_type field definition in package-spec: ip, wildcard, group or match_only_text

Should these be added too here?

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jsoriano

@jsoriano jsoriano merged commit d2165f3 into elastic:main Oct 17, 2023
37 checks passed
@jsoriano jsoriano deleted the scaled-float-dynamic-mapping branch October 17, 2023 08:24
@kibanamachine kibanamachine added v8.12.0 backport:skip This commit does not require backporting labels Oct 17, 2023
dej611 pushed a commit to dej611/kibana that referenced this pull request Oct 17, 2023
@jsoriano @dej611
[Fleet] Add support for additional types for dynamic mappings (elasti…
ca8500d 
…c#168842)

Add support for fields that generate dynamic mappings of the following
types:
- match_only_text
- scaled_float
- aggregate_metric_double
- half_float
- ip
- flattened
- integer (mapped as long as in other cases)
- group (child fields are installed as dynamic mappings)

Default `match_matching_type` has been corrected to provide always a
valid value. For example for floats it was using `float`, what is invalid, for
this case it is changed to `double`.

When the type is not known, an error is raised now instead of silently
ignoring the field definition, this helps package developers discover earlier
if their mappings are not well supported.

Tested with the `install_all_packages` and no current package fails
because of this.

Fix elastic#168823
benakansara pushed a commit to benakansara/kibana that referenced this pull request Oct 22, 2023
@jsoriano @benakansara
[Fleet] Add support for additional types for dynamic mappings (elasti…
2388044 
…c#168842)

Add support for fields that generate dynamic mappings of the following
types:
- match_only_text
- scaled_float
- aggregate_metric_double
- half_float
- ip
- flattened
- integer (mapped as long as in other cases)
- group (child fields are installed as dynamic mappings)

Default `match_matching_type` has been corrected to provide always a
valid value. For example for floats it was using `float`, what is invalid, for
this case it is changed to `double`.

When the type is not known, an error is raised now instead of silently
ignoring the field definition, this helps package developers discover earlier
if their mappings are not well supported.

Tested with the `install_all_packages` and no current package fails
because of this.

Fix elastic#168823
benakansara pushed a commit to benakansara/kibana that referenced this pull request Oct 22, 2023
@jsoriano @benakansara
[Fleet] Add support for additional types for dynamic mappings (elasti…
ff7cae7 
…c#168842)

Add support for fields that generate dynamic mappings of the following
types:
- match_only_text
- scaled_float
- aggregate_metric_double
- half_float
- ip
- flattened
- integer (mapped as long as in other cases)
- group (child fields are installed as dynamic mappings)

Default `match_matching_type` has been corrected to provide always a
valid value. For example for floats it was using `float`, what is invalid, for
this case it is changed to `double`.

When the type is not known, an error is raised now instead of silently
ignoring the field definition, this helps package developers discover earlier
if their mappings are not well supported.

Tested with the `install_all_packages` and no current package fails
because of this.

Fix elastic#168823
@kyungeunni kyungeunni mentioned this pull request Oct 24, 2023
Merged
8 tasks
@jsoriano jsoriano mentioned this pull request Oct 26, 2023
Merged
1 task
jsoriano added a commit that referenced this pull request Nov 6, 2023
@jsoriano
[Fleet] Create intermediate objects when using dynamic mappings (#169981
d398606 
)

For `dynamic: "runtime"`, and possibly other configurations different to
`dynamic: true`, intermediate objects need to exist for dynamic
mappings, otherwise ingestion can fail with `Missing intermediate object`
errors.

This pull request includes these intermediate objects, and avoids the
creation of static properties with wildcards, that is probably not what
is expected for these mappings.

Intermediate objects are included as static properties for parts of the
name before the wildcard, and as dynamic templates when the full path
has wildcards.

In the modified test files are examples of all of these.

This is more an issue since the following recent fixes, that create the
actual dynamic mappings for some cases where only empty objects were
created before.
* elastic/elastic-package#1492
* #168842
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrodm mrodm mrodm approved these changes

@kpollich kpollich kpollich approved these changes

Assignees

@jsoriano jsoriano

Labels
backport:skip This commit does not require backporting release_note:enhancement Team:Fleet Team label for Observability Data Collection Fleet team v8.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Fleet] Fields with dynamic types are silently ignored for some types
7 participants
@jsoriano @elasticmachine @apmmachine @kibana-ci @mrodm @kpollich @kibanamachine