Check fields type and format #13188
Check fields type and format #13188
Conversation
jsoriano
added
review
libbeat
and removed
in progress
jsoriano
force-pushed
the
check-fields-format
branch
from
2a06ee1
to
64483f5
Compare
jsoriano
added
Team:Integrations
libbeat/mapping/field.go
Outdated
| } | ||
| groupFormats, ok := validFormats[group] | ||
| if !ok { | ||
| panic("unexpected type group " + group) |
There was a problem hiding this comment.
We can return an error, is a panic necessary?
There was a problem hiding this comment.
This is a function only used internally, where group can only be set in validateType, so an incorrect value here is probably a bug.
In any case, I have modified it so a type group has its own type and so it can be checked by the compiller, that is its job 🙂
Let me know what you think.
|
Yay! This is long-needed. The logic LGTM. |
libbeat/mapping/field.go
Outdated
| @@ -110,6 +110,9 @@ func (d *DynamicType) Unpack(s string) error { | |||
|
|
|||
| // Validate ensures objectTypeParams are not mixed with top level objectType configuration | |||
| func (f *Field) Validate() error { | |||
| if err := f.validateType(); err != nil { | |||
| return err | |||
There was a problem hiding this comment.
What about returning here error trying to check 'type' field in 'fields.yml' fike '%s'. Check the 'fields.yml' in the module you are modifying for an incorrect or unknonwn 'type' value
There was a problem hiding this comment.
Ok, I have reordered error wrapping a little bit, this is what is printed now:
Generated global fields.yml file for metricbeat is invalid: incorrect type configuration for field 'max.bytes': unexpected formatter for number type, expected one of: string, url, bytes, duration, number, percent, color accessing '52.fields.0.fields.0.fields.1.fields.2.fields.0' (source:'fields.yml')
exit status 3
This is not ideal, but I haven't found a way to get more info from the underlying parsing library. In any case it is much better than what we have now.
There was a problem hiding this comment.
Totally agree. It's much better. Sorry for asking you a bit more 😅 maybe it's worth to just add (source: 'beats/metricbeat/module/{your-module}/_meta/fields.yml') or something similar. A new contributor won't know where the fields.yml is gonna be located.
There was a problem hiding this comment.
This part of the message is generated by the library, we are a bit limitated by that.
Also take into account that code in this PR only checks the global fields.yml, fields.yml files in modules and metricsets are not valid YAML by design, so they are not so easy to parse and check (and we don't). They are concatenated to create the global fields.yml and only this should be a valid YAML, but we are actually not even checking that, and this is not detected at all unless something mysteriously breaks in some integration test.
In summary, yes, at the moment it is not possible to easily know the module of the fields causing the problems, but hopefully a PR will only touch a limited number of fields, so it shouldn't be so hard to spot.
jsoriano
force-pushed
the
check-fields-format
branch
from
c723572
to
e114175
Compare
|
jenkins, test this again please |
|
jenkins, test this again please |
|
jenkins, test this again please |
On elastic#13188 a check is added on global fields generator to check that the generated fields file is valid. This check was done by reading the result from disk, but this file is not available when output is stdout. Modify the check so it is done over the buffered result before writing to file or stdout.
On #13188 a check is added on global fields generator to check that the generated fields file is valid. This check was done by reading the result from disk, but this file is not available when output is stdout. Modify the check so it is done over the buffered result before writing to file or stdout.
field.ymlfiles contain fields information that is used to build index templates,Kibana index patterns and documentation.
typeandformatsettings for thefields in these files have proven to be confusing to contributors, specially by the
reason that some unexpected values are ignored, or not used as expected.
This change adds a check so we can earlier detect unexpected values, and
provide feedback on the valid values.