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

[Cases] Removing unnecessary mapping fields #151511

Merged
merged 10 commits into from
Feb 28, 2023
Merged

[Cases] Removing unnecessary mapping fields #151511

merged 10 commits into from
Feb 28, 2023

Conversation

jonathan-buttner
Copy link
Contributor

@jonathan-buttner jonathan-buttner commented Feb 16, 2023 •
edited by cnasikas
Loading

This PR removes fields that do not need to be indexed from the various cases saved objects.

Fixes: #151501, #138727

@jonathan-buttner jonathan-buttner added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.8.0 labels Feb 16, 2023
@jonathan-buttner jonathan-buttner requested a review from a team as a code owner February 16, 2023 20:28
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops-cases (Feature:Cases)

adcoelho
adcoelho approved these changes Feb 20, 2023
View reviewed changes
Copy link
Contributor

@adcoelho adcoelho 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.

Double-checked the removed fields and the ones specified in the issue(except for cases.created_by) are all here 👍

Failing tests seem just to be snapshots as expected.

Why didn't we have dynamic: false for all mappings before?

@jonathan-buttner jonathan-buttner requested a review from a team as a code owner February 21, 2023 15:07
@jonathan-buttner
Copy link
Contributor Author

Why didn't we have dynamic: false for all mappings before?

Thanks for the review! If you could take one more look as I removed some created_by fields, that'd be great. I think we didn't have dynamic: false because we didn't know we needed it. We didn't get errors because we've always had all fields mapped (except a few which we have the dynamic: false set on those specifically).

@jonathan-buttner
Copy link
Contributor Author

jonathan-buttner commented Feb 21, 2023 •
edited
Loading

@cnasikas @kobelb The cases find API allows passing in the searchFields to specify which fields are used in a search. If users are leveraging that field to search some of the fields we're going to remove from the mapping this would cause a breaking change.

https://github.com/elastic/kibana/blob/main/x-pack/plugins/cases/common/api/cases/case.ts#L215

Should we leave all the case fields as mapped?

@cnasikas
Copy link
Member

cnasikas commented Feb 22, 2023 •
edited
Loading

@cnasikas @kobelb The cases find API allows passing in the searchFields to specify which fields are used in a search. If users are leveraging that field to search some of the fields we're going to remove from the mapping this would cause a breaking change.

https://github.com/elastic/kibana/blob/main/x-pack/plugins/cases/common/api/cases/case.ts#L215

Should we leave all the case fields as mapped?

Unfortunately, even though most of the fields we are removing are pretty unlikely to be used, this should be considered a breaking change. We should deprecate the query param. I opened an issue for that #151833.

cc @rudolf

@kobelb
Copy link
Contributor

kobelb commented Feb 22, 2023

@cnasikas @kobelb The cases find API allows passing in the searchFields to specify which fields are used in a search. If users are leveraging that field to search some of the fields we're going to remove from the mapping this would cause a breaking change.

https://github.com/elastic/kibana/blob/main/x-pack/plugins/cases/common/api/cases/case.ts#L215

Should we leave all the case fields as mapped?

Unfortunately, even though most of the fields we are removing are pretty unlikely to be used, this should be considered a breaking change. We should deprecate the query param. I opened an issue for that #151833.

Agreed. We should deprecate the searchFields query parameter; however, we can't remove those fields from being mapped without it being a breaking change.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/saved-objects-service.html#_mappings

id before after diff
cases-comments 37 14 -23
cases-configure 21 4 -17
cases-connector-mappings 6 2 -4
cases-user-actions 19 16 -3
total -47
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 428 430 +2

Total ESLint disabled count

id before after diff
securitySolution 506 508 +2

History

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

pgayvallet
pgayvallet approved these changes Feb 27, 2023
View reviewed changes
Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

Thanks for doing this

@cnasikas cnasikas linked an issue Feb 28, 2023 that may be closed by this pull request
Reduce field count for cases saved object types #138727
Closed
@jonathan-buttner jonathan-buttner merged commit 19ea090 into elastic:main Feb 28, 2023
@jonathan-buttner jonathan-buttner deleted the cases-remove-properties branch February 28, 2023 12:42
This was referenced Mar 6, 2023
Open
Merged
jonathan-buttner added a commit that referenced this pull request Mar 6, 2023
@jonathan-buttner
[Cases] Adding comments indicating which fields are not indexed (#152763
be430bc 
)

This PR adds comments to the various mapping files that had fields
removed as part of removing fields that should not be indexed.

This PR removed the fields from being mapped:
#151511

This serves to indicate which fields are stored in elasticsearch even
though they are not mapped. Once we have resolved this issue:
#152756 we can remove these
comments.
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
@jonathan-buttner @bmorelli25
[Cases] Removing unnecessary mapping fields (elastic#151511)
d8fc4b3 
This PR removes fields that do not need to be indexed from the various
cases saved objects.

Fixes: elastic#151501,
elastic#138727
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
@jonathan-buttner @bmorelli25
[Cases] Adding comments indicating which fields are not indexed (elas…
91a4096 
…tic#152763)

This PR adds comments to the various mapping files that had fields
removed as part of removing fields that should not be indexed.

This PR removed the fields from being mapped:
elastic#151511

This serves to indicate which fields are stored in elasticsearch even
though they are not mapped. Once we have resolved this issue:
elastic#152756 we can remove these
comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgayvallet pgayvallet pgayvallet approved these changes

@cnasikas cnasikas cnasikas approved these changes

@adcoelho adcoelho Awaiting requested review from adcoelho

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Cases Cases feature 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.8.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Cases] Remove superfluous fields in mappings Reduce field count for cases saved object types
7 participants
@jonathan-buttner @elasticmachine @cnasikas @kobelb @kibana-ci @pgayvallet @adcoelho