[Rule Registry] Switch to _source for updating documents instead of Fields API

[simianhacker]

Summary

This PR closes #113003 by removing the Fields API query and including _source in the results, then it used hit._source inplace of hit.fields for parsing the technical fields. I choose this options because it solved the main issue, Fields API returns derived fields which shouldn't be indexed, with the smallest surface area of change.

It appears that the alerts are only being created and updated via the ruleDataClient.getWriter().bulk(...) operation here:

kibana/x-pack/plugins/rule_registry/server/utils/create_lifecycle_executor.ts

Lines 287 to 294 in 5c73c0c

	await ruleDataClient.getWriter().bulk({
	body: allEventsToIndex.flatMap(({ event, indexName }) => [
	indexName
	? { index: { _id: event[ALERT_UUID]!, _index: indexName, require_alias: false } }
	: { index: { _id: event[ALERT_UUID]! } },
	event,
	]),
	});

Checklist

• Unit or functional tests were updated or added to match the most common scenarios

[ersin-erdal]

TBH i didn't fully understand what an executor does, but it looks like switching form fields to _source when reading from ruleDataClient is enough. Then the result of that request is sent back with await ruleDataClient.getWriter().bulk action...
LGTM :)

Note: i know not related to this issue but, functions with side-effects like:

const lifecycleAlertServices: LifecycleAlertServices<
      InstanceState,
      InstanceContext,
      ActionGroupIds
    > = {
      alertWithLifecycle: ({ id, fields }) => {
        currentAlerts[id] = fields;
        return alertInstanceFactory(id);
      },
    };

🥺

[mgiota]

@simianhacker I started looking into this and assuming we agree to use _source instead of fields, shouldn't we change other parts in the code as well to make it more clear we read from _source? I am not saying we should do following changes, I am just thinking of the scenario we might want to use both fields and _source later on as separate fields, then naming would be confusing. Unless we merged fields and _source in one key called fields, then it should be fine. What do you think? Shall we keep just your current changes or do we want to consider below changes? This would require even more changes to each observability rule type executor (apm, uptime and metric)

To be completely honest I don't feel comfortable with this change, since we are changing to something we don't even use (we don't make use of any object type at the moment, so why do we change it now).

However I tested your changes with existing rule types (created a log threshold rule type, an alert was triggered, I relaxed rule conditions, status was updated from active to recovered) and everything worked as expected. I didn't create an object type though so that I could test that the retrieved ES alert document would not change and be saved back to its original form). We could create a few tests.

I'll meet with @dgieselaar to further discuss on this topic, so I'll summarize the conclusions here.

cc @jasonrhodes

[mgiota]

@simianhacker We talked with @dgieselaar regarding parseTechnicalFields() and he confirmed that it was written with fields and not _source in mind.

runtimeTypeFromFieldMap assumes that:

• field names are flattened
• every value is an array

At the moment everything works because we don't use any nested field type, once we do so, the mismatch might cause surprises.

So if we want to use _source instead of fields, we would want to look into this function. I suggest we create separate functions parseTechnicalSource (or something like this) and runtimeTypeFromSourceMap

I suggest we focus on writing some tests with fields of an object type and see how things work.

[mgiota]

@afgomez What's your thoughts on this?

[afgomez]

I did some testing and everything seems to work. I introduced a { foo: { bar: 'baz' }} field in the event to see what happened with nested fields and it kept working right.

LGTM!

[simianhacker]

@afgomez Thanks for testing it. I'm in the process of creating a functional/integration test that does exactly this.

[simianhacker]

@elasticmachine merge upstream

[mgiota]

LGTM! Let's wait for CI and we can merge!

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: d59e21d
• Storybooks Preview

Test Failures

• [job] [logs] Docker CI Group / Fleet Endpoints EPM Endpoints setup api setup performs upgrades upgrades the endpoint package from 0.13.0 to the latest version available

Metrics [docs]

✅ unchanged

History

• 💚 Build #11437 succeeded 9eb83f8
• 💚 Build #9760 succeeded 6641049
• 💚 Build #6749 succeeded 2250c15
• 💚 Build #6402 succeeded c0e602e
• 💔 Build #6001 failed c566976
• 💔 Build #5967 failed 24aaaba

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

[kibanamachine]

💔 Backport failed

Status	Branch	Result
❌	8.0	Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 118245

[elasticmachine]

Pinging @elastic/uptime (Team:uptime)

[elasticmachine]

Pinging @elastic/apm-ui (Team:apm)