Add missing properties to fields.yml spec #314

andrewkroh · 2022-04-05T21:37:41Z

What does this PR do?

I looked over the code in Fleet EPM to see what properties are used when generating templates and then updated the specification to match.

~~The current version is a draft because it contains commentary as "TODO"s.~~

Why is it important?

A detailed and accurate specification is required to ensure the developers of integration packages and the implementors of the specification agree on the behavior.

Checklist

I have added test packages to test/packages that prove my change is effective.
I have added an entry in versions/N/changelog.yml.

Related issues

Relates [Change Proposal] Stricter validation on fields properties #282

Author Notes

Based on feedback in this PR we need to follow up on these tasks.

Answer why Fleet has include_in_root, include_in_parent. These are for nested type fields and are now in the spec.
Answer why Fleet has "group-nested" and "nested". It's internal to Fleet. No integrations will use it.
null_value should be supported by types other than wildcard.
Add support from dynamic: runtime.
Update schema to support dynamic mappings (e.g. path_match, path_unmatch, match, unmatch, etc). This blocks development of [Fleet] dynamic_template mappings for wildcard field names are not installed kibana#129344. We'll want to address this before incorporating this into elastic-package so that there is a migration path for fields that "think" they are creating dynamic mappings today and are using unsupported properties.
Fleet should support multi_field on match_only_text field types.
Fleet should add support for norms on text fields.
Need to determine how deal with what format does in Beats (e.g. applying a formatter to the data view).

versions/1/data_stream/fields/fields.spec.yml

andrewkroh · 2022-04-05T21:41:30Z

versions/1/data_stream/fields/fields.spec.yml

        - geo_point
-        - object
+        - group


Fleet EPM has a group-nested type. Does that need added to the list? Why would we have both group-nested and nested?

https://github.com/elastic/kibana/blob/0695df649705c91ed28dbc73037aea878ce419f7/x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts#L120

I don't see this type used in integrations nor in beats, I would say to don't add it here, we can add it in a follow up if needed.

@joshdover do you know why this type is in Fleet?

it was added 2 years ago in this pr, but doesn't say a lot of context about why: elastic/kibana#64829

this logic here sets group-nested type: https://github.com/jonathan-buttner/kibana/blob/22b4a693eaaba401b5385d6341a0e9fcf9ff518f/x-pack/plugins/ingest_manager/server/services/epm/fields/field.ts#L147-L151

I didn't try to follow the logic, but my take is that is seems like an internal-only value. So none of the integrations should use group-nested and it does not need to be in the spec.

elasticmachine · 2022-04-05T21:43:04Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-04-29T20:37:33.987+0000
Duration: 2 min 28 sec

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

jsoriano

Thanks @andrewkroh! Awesome revamp of the fields spec.

versions/1/data_stream/fields/fields.spec.yml

jsoriano · 2022-04-06T09:04:48Z

versions/1/data_stream/fields/fields.spec.yml

        - geo_point
-        - object
+        - group


I don't see this type used in integrations nor in beats, I would say to don't add it here, we can add it in a follow up if needed.

@joshdover do you know why this type is in Fleet?

versions/1/data_stream/fields/fields.spec.yml

jsoriano · 2022-04-06T09:15:11Z

versions/1/data_stream/fields/fields.spec.yml

+          Only honored for 'type: text/keyword/wildcard'.
+        $ref: "#" # JSON-schema syntax for pointing to the root of the schema
+
+      # TODO: Fleet only honors this for 'type: wildcard'. That seems wrong.


Ping @joshdover

versions/1/data_stream/fields/fields.spec.yml

jsoriano · 2022-04-06T09:18:50Z

versions/1/data_stream/fields/fields.spec.yml

+    # TODO: We should disable additionalProperties. There are properties used in
+    # elastic/integrations that have no purpose or are typos.
    additionalProperties: true


+1, specially after adding these missing properties. @mtojek wdyt?

Suggested change

# TODO: We should disable additionalProperties. There are properties used in

# elastic/integrations that have no purpose or are typos.

additionalProperties: true

# TODO: We should disable additionalProperties. There are properties used in

# elastic/integrations that have no purpose or are typos.

additionalProperties: false

+1 on my side too.

I think that we left it open to allow for extra properties like examples, patterns, etc. It seems that we aren't using the actively, so let's close this option.

I set additionalProperties: false and adjusted some of the test packages to remove level, group, and title.

There are many fields using level, group and/or title in the integrations repo, wdyt about adding these fields here by now? We could add them directly as deprecated, and eventually phase them out.

I have created this PR to check what would fail in the integrations repo after this change: elastic/integrations#3024

We'll need to investigate why these are in Fleet. Then either add them here or remove them from Fleet.

joshdover · 2022-04-07T11:23:58Z

@juliaElastic Do you have time to review this? I believe you're more up to date on our mapping field logic than I am.

juliaElastic · 2022-04-07T11:27:39Z

@juliaElastic Do you have time to review this? I believe you're more up to date on our mapping field logic than I am.

I can have a look later today.

versions/1/data_stream/fields/fields.spec.yml

# Conflicts: # versions/1/changelog.yml

juliaElastic

LGTM

jsoriano · 2022-04-29T13:08:19Z

I have updated elastic/integrations#3024, and there are still some missing fields (level, group, title, example...). It may be hard to support all fields on a first pass.

@andrewkroh @mtojek wdyt about setting additionalProperties back to true so we can merge all the fields and docs included here, and continue adding fields in future PRs?

andrewkroh · 2022-04-29T13:11:29Z

additionalProperties back to true

👍 That's what I'm thinking to do. Even if we remove level, group, title, example there will still be some keys related to dynamic fields that we need to account for. So after we specify the dynamic field behavior we can make this strict.

test/packages/missing_required_fields/data_stream/foo/fields/some_fields.yml

This reverts commit b9b5fc1.

jsoriano

LGTM, I have updated elastic/integrations#3024 as a last check in case there is something unexpected.

jsoriano · 2022-04-29T13:24:44Z

test/packages/good/data_stream/foo/fields/some_fields.yml

@@ -35,3 +35,15 @@
 - name: error.message
  description: Error message.
  type: match_only_text
+- name: metric.*_bytes


Nit. Is this an intentional test case? We don't use to have wildcards mixed with suffixes, not sure if everything works as expected with this.

Suggested change

- name: metric.*_bytes

- name: metric.*.bytes

This was intentional to test the pattern specified for name. It really is an area of unspecified behavior at the moment. I was operating under the assumption that wildcards will be used to implement behavior similar to path_match. But for now this is a test case just to check that the pattern matches fields like this:

https://github.com/elastic/integrations/blob/3c88555054fd24a461babebe624d8645b5feea6c/packages/aws/data_stream/cloudwatch_metrics/fields/package-fields.yml#L16

We should refine this area of the spec for dynamic fields.

I could remove the pattern for now. Then when we get to dynamic fields add it back to match the definition there.

Oh ok, lets leave it. Thanks.

jsoriano

Pending failures in elastic/integrations#3024 don't seem to be related to this, but to #309.

jsoriano · 2022-04-29T15:00:29Z

test/packages/good/data_stream/foo/fields/some_fields.yml

@@ -35,3 +35,15 @@
 - name: error.message
  description: Error message.
  type: match_only_text
+- name: metric.*_bytes


Oh ok, lets leave it. Thanks.

andrewkroh · 2022-04-29T20:18:11Z

I've been looking over the issues in the integrations repo. After fixing many unused fields and typos (see https://github.com/elastic/integrations/pull/3239/commits) this is what's left:

763 instances of format - used to specify things like byte, percent
1162 instances of norms - used to specify https://www.elastic.co/guide/en/elasticsearch/reference/current/norms.html
164 instances of object_type_mapping_type - part of dynamic fields behavior that is TBD
61 issues with field names that don't match pattern. We need to include - (dash) in the list of accepted characters.

andrewkroh · 2022-04-29T20:45:22Z

After allowing a dash there are three issues with names, and they all seems like problems with the integration (ignoring issues from the norms, format, object_type_mapping_type, and duplicate fields):

DBClusterIdentifier,Role - https://github.com/elastic/integrations/blob/4247dfe2b3fe14d5e3dd02eb2e9891814e9f92e0/packages/aws/data_stream/rds/fields/fields.yml#L13

DbClusterIdentifier, EngineName - https://github.com/elastic/integrations/blob/4247dfe2b3fe14d5e3dd02eb2e9891814e9f92e0/packages/aws/data_stream/rds/fields/fields.yml#L16

Application,Platform - https://github.com/elastic/integrations/blob/8770467808f03a38de9563d7052a22ea1e472eea/packages/aws/data_stream/sns/fields/fields.yml#L10 (edited: to fix line reference)

So I think this is good now.

jsoriano · 2022-05-01T09:52:17Z

DBClusterIdentifier,Role - https://github.com/elastic/integrations/blob/4247dfe2b3fe14d5e3dd02eb2e9891814e9f92e0/packages/aws/data_stream/rds/fields/fields.yml#L13

DbClusterIdentifier, EngineName - https://github.com/elastic/integrations/blob/4247dfe2b3fe14d5e3dd02eb2e9891814e9f92e0/packages/aws/data_stream/rds/fields/fields.yml#L16

Application,Platform - https://github.com/elastic/integrations/blob/8770467808f03a38de9563d7052a22ea1e472eea/packages/aws/data_stream/sns/fields/fields.yml#L10

@kaiyan-sheng do you expect any problem if we fix these fields to don't contain unexpected characters?

kaiyan-sheng · 2022-05-02T16:37:21Z

DBClusterIdentifier,Role

@jsoriano Hmmm good catch, I don't think we are actually storing dimensions like this. For example, this dimension is stored as aws.dimensions.DBClusterIdentifier and aws.dimensions.Role as two separate fields. These fields are documented wrong, I will fix them.

andrewkroh · 2022-05-03T00:23:35Z

@kaiyan-sheng Thanks for addressing RDS in elastic/integrations#3253. Is the SNS dimension a bug or are we expecting to have JSON key names containing commas?

kaiyan-sheng · 2022-05-03T15:02:51Z

Is the SNS dimension a bug or are we expecting to have JSON key names containing commas?

@andrewkroh Good catch! SNS dimension is also a bug and I just added the fix into the same PR. Thank you!

andrewkroh · 2022-05-04T12:47:22Z

I think this is ready now. I will start creating follow-up issues for the things listed in the PR description.

jsoriano

👍 thanks Andrew! @mtojek @juliaElastic any objection to merge this?

jsoriano · 2022-05-09T09:31:05Z

Oh, one thing would be missing, @andrewkroh could you please add a changelog entry?

jsoriano · 2022-05-09T15:53:49Z

Ah, changelog is already here 🤦 Merging.

Add fields honored by Fleet

84f656f

andrewkroh commented Apr 5, 2022

View reviewed changes

versions/1/data_stream/fields/fields.spec.yml Outdated Show resolved Hide resolved

andrewkroh commented Apr 5, 2022

View reviewed changes

andrewkroh mentioned this pull request Apr 5, 2022

[Fleet] dynamic_template mappings for wildcard field names are not installed elastic/kibana#129344

Closed

andrewkroh requested review from joshdover and jsoriano April 5, 2022 21:50

jsoriano reviewed Apr 6, 2022

View reviewed changes

andrewkroh added 7 commits April 6, 2022 17:00

Revert sorting of types

e56cc7f

Add test to check that asterisk is accepted in field name

00c5fa1

Disallow additional properties in fields files

afccc99

Remove level, group, title properties from test files

b9b5fc1

Remove include_in_parent, include_in_root

9295999

We'll need to investigate why these are in Fleet. Then either add them here or remove them from Fleet.

Update multi_fields to mention match_only_text

f8fa2ba

Improve ignore_above description

23bf678

andrewkroh marked this pull request as ready for review April 6, 2022 21:50

andrewkroh requested a review from a team as a code owner April 6, 2022 21:50

andrewkroh added 2 commits April 6, 2022 17:53

Merge remote-tracking branch 'elastic/main' into fields-yml-spec

fc49926

Add changelog entry

ec1318b

jsoriano mentioned this pull request Apr 7, 2022

Test changes to mandatory field attributes elastic/integrations#3024

Closed

juliaElastic reviewed Apr 7, 2022

View reviewed changes

versions/1/data_stream/fields/fields.spec.yml Outdated Show resolved Hide resolved

juliaElastic reviewed Apr 7, 2022

View reviewed changes

versions/1/data_stream/fields/fields.spec.yml Outdated Show resolved Hide resolved

andrewkroh mentioned this pull request Apr 7, 2022

[hashicorp_vault] undefined metric fields detected by tests elastic/integrations#2898

Closed

joshdover removed their request for review April 11, 2022 17:40

andrewkroh added 4 commits April 28, 2022 16:48

Merge remote-tracking branch 'elastic/main' into fields-yml-spec

cb32bb1

# Conflicts: # versions/1/changelog.yml

Clarify metric_type and dimension docs

116168d

Remove match_only_text reference from multi_fields

f77a923

Add include_in_parent and include_in_root for nested fields

2223ec9

juliaElastic previously approved these changes Apr 29, 2022

View reviewed changes

Set additionalProperties: true

81b7637

andrewkroh dismissed juliaElastic’s stale review via 81b7637 April 29, 2022 13:13

jsoriano reviewed Apr 29, 2022

View reviewed changes

test/packages/missing_required_fields/data_stream/foo/fields/some_fields.yml Outdated Show resolved Hide resolved

andrewkroh added 2 commits April 29, 2022 09:18

Revert "Remove level, group, title properties from test files"

f86db77

This reverts commit b9b5fc1.

Amend changelog

2baa12c

jsoriano reviewed Apr 29, 2022

View reviewed changes

jsoriano previously approved these changes Apr 29, 2022

View reviewed changes

andrewkroh dismissed jsoriano’s stale review via 99e3c40 April 29, 2022 20:20

andrewkroh force-pushed the fields-yml-spec branch from 99e3c40 to 5953a0e Compare April 29, 2022 20:30

Allow dash in field names

86254fe

andrewkroh force-pushed the fields-yml-spec branch from 5953a0e to 86254fe Compare April 29, 2022 20:37

andrewkroh requested a review from jsoriano May 4, 2022 12:46

jsoriano approved these changes May 9, 2022

View reviewed changes

mtojek approved these changes May 9, 2022

View reviewed changes

jsoriano merged commit 6d1d0e7 into elastic:main May 9, 2022

jsoriano mentioned this pull request Jun 1, 2022

Disallow unknown properties on fields definitions #281

Closed

2 tasks

endorama mentioned this pull request Sep 13, 2022

object_type_mapping_type is not defined nor documented #416

Closed

jsoriano mentioned this pull request Sep 15, 2022

Stricter validation of field definitions #420

Merged

3 tasks

Add missing properties to fields.yml spec #314

Add missing properties to fields.yml spec #314

Conversation

andrewkroh commented Apr 5, 2022 • edited Loading

What does this PR do?

Why is it important?

Checklist

Related issues

Author Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliaElastic Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Apr 5, 2022 • edited Loading

💚 Build Succeeded

Build stats

🤖 GitHub comments

jsoriano left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdover commented Apr 7, 2022

juliaElastic commented Apr 7, 2022

juliaElastic left a comment

Choose a reason for hiding this comment

jsoriano commented Apr 29, 2022

andrewkroh commented Apr 29, 2022

jsoriano left a 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

jsoriano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh commented Apr 29, 2022

andrewkroh commented Apr 29, 2022 • edited Loading

jsoriano commented May 1, 2022 • edited by andrewkroh Loading

kaiyan-sheng commented May 2, 2022

andrewkroh commented May 3, 2022

kaiyan-sheng commented May 3, 2022

andrewkroh commented May 4, 2022

jsoriano left a comment

Choose a reason for hiding this comment

jsoriano commented May 9, 2022

jsoriano commented May 9, 2022

andrewkroh commented Apr 5, 2022 •

edited

Loading

juliaElastic Apr 7, 2022 •

edited

Loading

elasticmachine commented Apr 5, 2022 •

edited

Loading

andrewkroh commented Apr 29, 2022 •

edited

Loading

jsoriano commented May 1, 2022 •

edited by andrewkroh

Loading