Accept strings that parse as numbers as numeric types#269
Accept strings that parse as numbers as numeric types#269jsoriano wants to merge 2 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/integrations (Team:Integrations) |
|
We already have this feature, but the strict mode is enabled by default. If you prefer to enable conversion, please use test config ( cc @ycombinator |
|
@mtojek oh ok, thanks! |
💔 Build Failed
Expand to view the summary
Build stats
Trends 🧪Steps errors
Expand to view the steps failures
|
|
@mtojek Maybe we should add documentation for the test config options somewhere under https://github.com/elastic/elastic-package/tree/master/docs/howto? |
|
It's a good idea to do. Frankly speaking I though we have already documented it somewhere, but it looks like it's missing. |
|
I'll make a PR for the docs. |
|
Umm, after doing some tests with
I need something for fields that are defined as numbers, but are present as strings in the documents, for example: For Should we add another option for this? Or should I reopen this PR? Or should I be doing something different? 🙂 For context, I am needing this to migrate the test files for PostgreSQL (elastic/integrations#747). |
|
I would be in favor of reopening this PR. From what I can tell the code in this PR is only converting string values to numbers if the field is defined as a number. So there is enough strictness already IMO. In fact, now I'm wondering if we could apply the same logic for numeric values that are intended to be keywords and not need the The other option would be to introduce another setting, similar to @mtojek WDYT? |
|
Ok, I see the conversion direction now, missed that earlier (sorry about that). Actually I would be in favor of preserving strict checks, but I understand this is a behavior that is enabled by default (dynamic field mapping)? We can merge this PR, but then it won't be consistent with Let's review this PR to unblock @jsoriano and think what would be the solution for our cases. /cc @andrewkroh as the Security Team uses /cc @ruflin do we have any guidance in terms of fields strictness for agent/packages? |
ycombinator
left a comment
There was a problem hiding this comment.
Left a minor suggestion for adding a comment. Otherwise LGTM.
|
|
/test |
|
I think the processing pipeline should be using a If we do choose to go away from the strict type checking for strings that become numbers then it makes no sense to keep the |
I would prefer that option as well, but maybe I'm not familiar with historical reasons/customs/etc and it's natural to depend on dynamic mappings in Elasticsearch. @jsoriano did you consider just using As I said before I prefer strictness in this area, but I'm ok to merge this PR and then deprecate the |
|
Sounds good to me. I didn't try to modify the pipeline, I just imported it from beats, but I agree that it would be better to make it more strict, I'll give a try to add explicit conversions. |
|
elastic/integrations#747 updated to don't need this. Thanks for the suggestion! |
|
@mtojek @andrewkroh I am finding this issue on a metricset, I have tried to add an ingest pipeline and a processor in the stream, but none of them seemed to work. Should any of these work for a metrics data stream? or the only way is to fix this in Metricbeat? |
|
I'm not aware of any reasons that a data stream for metrics would not allow an ingest pipeline to be used. So that's interesting and curious. But I think Metricbeat should be sending a number for this value if that is its intent. |
|
Open PR for Metricbeat: elastic/beats#24359 |
elastic-package testfails if a document contains a string value with a number in a field with numeric type, such as:But these values are accepted by Elasticsearch.
Modify the check to accept also strings that can be parsed as numbers.