Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Reponse Ops][Alerting] Excluding ECS experimental fields from ECS component template #170571

Merged
merged 15 commits into from
Nov 27, 2023
Merged

[Reponse Ops][Alerting] Excluding ECS experimental fields from ECS component template #170571

merged 15 commits into from
Nov 27, 2023

Conversation

ymao1
Copy link
Contributor

@ymao1 ymao1 commented Nov 3, 2023 •
edited
Loading

Resolves #168959

Summary

Hard-coding a list of experimental (RFC stage 2) ECS fields to exclude from the ECS component template. These are only the fields that are not currently defined in ecs_flat.yml. The only existing field that is excluded is faas.trigger which, if included, will cause a mapping conflict exception because of an ECS mapping change from nested to object.

To Verify

Compare the mappings for the .alerts-ecs-mappings component template between main and this branch and notice that the faas.trigger field is excluded from the component template on this branch.

@ymao1 ymao1 self-assigned this Nov 3, 2023
@ymao1 ymao1 added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Alerting/Alerts-as-Data Issues related to Alerts-as-data and RuleRegistry v8.12.0 labels Nov 3, 2023
@ymao1 ymao1 marked this pull request as ready for review November 3, 2023 20:33
@ymao1 ymao1 requested a review from a team as a code owner November 3, 2023 20:33
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@ymao1 ymao1 requested a review from a team as a code owner November 3, 2023 21:42
@ymao1 ymao1 requested a review from vitaliidm November 3, 2023 21:42
@ymao1
Copy link
Contributor Author

ymao1 commented Nov 6, 2023

@elasticmachine merge upstream

vitaliidm
vitaliidm reviewed Nov 7, 2023
View reviewed changes
Copy link
Contributor

@vitaliidm vitaliidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one question regarding impact on existing customers before alerts index rolls out

...solution/server/lib/detection_engine/rule_types/factories/utils/strip_non_ecs_fields.test.ts Outdated Show resolved Hide resolved
@ymao1
Copy link
Contributor Author

ymao1 commented Nov 20, 2023

@elasticmachine merge upstream

@pmuellr
Copy link
Member

pmuellr commented Nov 20, 2023

I tried a "migration" from main to this PR, and got the following:

[2023-11-20T10:35:57.010-05:00][INFO ][plugins.alerting] Creating concrete write index - .internal.alerts-observability.metrics.alerts-default-000001
[2023-11-20T10:35:57.047-05:00][ERROR][plugins.alerting] Failed to PUT mapping for .alerts-observability.metrics.alerts-default: illegal_argument_exception
	Root causes:
		illegal_argument_exception: can't merge a non-nested mapping [faas.trigger] with a nested mapping
[2023-11-20T10:35:57.047-05:00][ERROR][plugins.alerting] ResponseError: illegal_argument_exception
	Root causes:
		illegal_argument_exception: can't merge a non-nested mapping [faas.trigger] with a nested mapping
    at KibanaTransport.request (/Users/pmuellr/Projects/elastic/kibana-170571-exclude-experimental-ecs-fields/node_modules/@elastic/transport/src/Transport.ts:535:17)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

@ymao1 ymao1 requested a review from pmuellr November 20, 2023 16:22
pmuellr
pmuellr approved these changes Nov 20, 2023
View reviewed changes
Copy link
Member

@pmuellr pmuellr left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - survives "migrating" from main to this PR; adding the "children" of faas to be excluded (added in subsequent commits) resolved the migration issue I reported ^^^

@ymao1
Copy link
Contributor Author

ymao1 commented Nov 21, 2023

@elasticmachine merge upstream

@ymao1
Copy link
Contributor Author

ymao1 commented Nov 21, 2023

@elasticmachine merge upstream

@ymao1
Copy link
Contributor Author

ymao1 commented Nov 21, 2023

@elasticmachine merge upstream

@ymao1
Copy link
Contributor Author

ymao1 commented Nov 22, 2023

@elasticmachine merge upstream

@ymao1
Copy link
Contributor Author

ymao1 commented Nov 27, 2023

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 3.7MB 3.7MB +716.0B
infra 1.9MB 1.9MB +711.0B
observability 1.1MB 1.1MB +711.0B
securitySolution 12.8MB 12.8MB +711.0B
triggersActionsUi 1.4MB 1.4MB +711.0B
total +3.5KB

History

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

cc @ymao1

@ymao1 ymao1 merged commit 618cc48 into elastic:main Nov 27, 2023
26 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 27, 2023
@ymao1 ymao1 deleted the exclude-experimental-ecs-fields branch November 27, 2023 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitaliidm vitaliidm vitaliidm left review comments

@pmuellr pmuellr pmuellr approved these changes

Assignees

@ymao1 ymao1

Labels
backport:skip This commit does not require backporting Feature:Alerting/Alerts-as-Data Issues related to Alerts-as-data and RuleRegistry release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Response Ops][Alerting] Possible "experimental" ECS fields are not backwards compatible for alerts as data
6 participants
@ymao1 @elasticmachine @pmuellr @kibana-ci @vitaliidm @kibanamachine