Extend system test to validate absence of _ignored

[flash1293]

Closes #1276

This PR adds a check whether any indexed docs have an _ignored field - this would indicate a mapping problem:

Example output:

Error: error running package system tests: could not complete test run: found _ignored fields in data stream logs-cel.cel-ep

How to test

Cause a mapping error by configuring a field in a wrong way - I used the cel package:

--- a/packages/cel/fields/input.yml +++ b/packages/cel/fields/input.yml @@ -3,7 +3,8 @@ - name: message - external: ecs + type: keyword + ignore_above: 5 - name: input.name type: constant_keyword - name: input.type

As message is longer than 5 characters, this will cause message to not be mapping correctly. Running the system tests for cel will fail with the error shown above.

Notes

The check is done via aggregations instead of checking through each documents to be able to validate all docs, not just the ones that are fetched (which are capped at 500). Once elastic/elasticsearch#101373 is merged, we can use a terms query here for better debug information (but it's also possible to check the index manually to find the root cause).

This cannot be meaningfully applied to the pipeline tests currently, as they don't take mappings into account (elastic/elasticsearch#99920).

[flash1293]

It actually looks like this check finds an error right away:

Error: error running package system tests: could not complete test run: found _ignored fields in data stream logs-auditd_manager.auditd-ep

 
I ran this test locally and the issue is the event.original field, which is mapped as a keyword with ignore_above of 1024 - I think it will be resolved by #1733 , can you confirm @jsoriano ?

[ruflin]

++, nice addition. The more we add of these checks, the higher the quality of integrations will be.

[jsoriano]

test integrations

[jsoriano]

I ran this test locally and the issue is the event.original field, which is mapped as a keyword with ignore_above of 1024 - I think it will be resolved by #1733 , can you confirm @jsoriano ?

It is possible, but we would need to test it when ready.

[jsoriano]

LGTM, thanks!

But let's wait to see if it would affect many existing integrations. If it does, we could introduce this only for packages using the next version of the spec.

[elasticmachine]

Created or updated PR in integrations repository to test this version. Check elastic/integrations#9439

[flash1293]

test integrations

[elasticmachine]

Created or updated PR in integrations repository to test this version. Check elastic/integrations#9439

[flash1293]

test integrations

[flash1293]

Still the same error in the test package, I will look into this

[elasticmachine]

Created or updated PR in integrations repository to test this version. Check elastic/integrations#9439

[andrewkroh]

I think the returned error should at a minimum include the names of ignored fields. You could aggregate those field names. (It needs a runtime field to access the names as per elastic/elasticsearch#59946 (comment)).

POST logs-*/_search { "size": 10, "runtime_mappings": { "my_ignored": { "type": "keyword", "script": { "source": "for (def v : params['_fields']._ignored.values) { emit(v); }" } } }, "aggs": { "all_ignored": { "filter": { "exists": { "field": "_ignored" } }, "aggs": { "ignore_fields": { "terms": { "size": 100, "field": "my_ignored" } } } } } } 

[flash1293]

As mentioned I wanted to wait with this until the change to make _ignored aggregatable is merged, but you are right that we don't need to block on it - I changed the code to use the runtime field workaround for now.

[flash1293]

test integrations

[flash1293]

The test auditd_manager package still fails as it relies on the ecs@mappings component template which won't be fixed until elastic/elasticsearch#106714 is released - we can silence the test by explicitly mapping event.original correctly, but let's check how many of the real integrations are affected in a similar way - if there are a lot, we either should:

• wait for 8.14.0 until we merge this
• or make an exception for event.original for now to at least detect other mapping errors

[elasticmachine]

Created or updated PR in integrations repository to test this version. Check elastic/integrations#9439

[flash1293]

test integrations

[elasticmachine]

Created or updated PR in integrations repository to test this version. Check elastic/integrations#9439

[flash1293]

@jsoriano As the need came up in the existing failures, I opened a PR on the package-spec to add a flag to add exceptions to this test: elastic/package-spec#743

Once this is merged, I'm going to update this PR to check this definition and skip the check if it makes sense. Once this is in place, it should be possible to mute existing errors in the integrations repo and merge this PR.

[ShourieG]

@flash1293, I've often seen encoded payload field values exceed 6-7k characters easily. This can easily go up even more in real world scenarios and the issue is these are always clumped into 'keyword' types. We really need a way to mark 'keyword' types to have an attribute like 'ignore_length: true' or some sort or introduce a new type for storing encoded payloads, otherwise in a real scenario these fields will not get indexed after the recent changes and just addressing the test scenarios won't work.

[flash1293]

@ShourieG Two questions:

• You mean this becoming an Elasticsearch feature so it's always considered, not just during package development?
• Why are these fields indexed as keyword? Wouldn't it make sense to not index them in the first place if they are usually ignored anyway? This would mean that short values also wouldn't get indexed anymore, but the use of such a field seems questionable anyway - if it's only searchable in 50% of cases, will that actually help during exploration?

[flash1293]

otherwise in a real scenario these fields will not get indexed after the recent changes and just addressing the test scenarios won't work

The changes around _ignored didn't change when values got indexed or not, it's just about exposing the problem.

[ShourieG]

otherwise in a real scenario these fields will not get indexed after the recent changes and just addressing the test scenarios won't work

The changes around _ignored didn't change when values got indexed or not, it's just about exposing the problem.

@flash1293, Ah alright, I guess I misunderstood initially the issue around indexing. Then maybe adding the 'ignore_length: true' to the package spec so we can specify that during the tests would be pretty awesome? Something that we can specify in the validation.yml ?

[flash1293]

@ShourieG Currently working on it here: elastic/package-spec#743

[flash1293]

Muting existing failures here: elastic/integrations#9823

[jsoriano]

LGTM, thanks for the work here! Feel free to merge the change in the spec too.

[flash1293]

/ci

[flash1293]

@jsoriano it seems like the build failed, how can I trigger it again? It doesn't look related to the changes of the PR

[jsoriano]

@jsoriano it seems like the build failed, how can I trigger it again? It doesn't look related to the changes of the PR

You can write a /test comment to re-launch everything, or use the buildkite UI to retry single stages:

The failure happening now is in a package whose tests use to be flaky, so probably the failure is not related.

[flash1293]

/test

[flash1293]

/test

[flash1293]

/test

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: b45109e

History

• 💔 Build #3073 failed b45109e
• 💔 Build #3056 failed b45109e
• 💔 Build #3055 failed b45109e
• 💚 Build #3051 succeeded b4b2acf
• 💚 Build #3025 succeeded 52eee94
• 💚 Build #2913 succeeded 934e899