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.

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

[RAM] ResponseOps saved object updates #152857

Merged
merged 44 commits into from
Apr 18, 2023

Conversation

XavierM
Copy link
Contributor

@XavierM XavierM commented Mar 7, 2023

Summary

This PR is just the first phase for response ops to go through their saved object attributes. The idea is to comment out all the attributes that we all agree that we do not need to filter/search/sort/aggregate on.

After, in a second phase/PR, we will create a new file who will represent all of attributes in our saved object as a source of truth. Then, we will generate our SO mappings from this source of truth to register our saved object.

Phase 3, we will try to generate also our type from our source of truth.

@XavierM XavierM 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) v8.8.0 labels Mar 7, 2023
@XavierM XavierM requested review from a team as code owners March 7, 2023 21:40
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Mar 7, 2023
@XavierM XavierM marked this pull request as draft March 7, 2023 21:40
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@XavierM XavierM force-pushed the alerting-saved-object-updates branch from ec7fe69 to fdf0ab5 Compare March 7, 2023 21:41
@XavierM XavierM requested review from a team and spong and removed request for a team March 7, 2023 21:41
fkanout

This comment was marked as outdated.

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

left comments on some additional fields we may be able to comment out

other than that, LGTM; going to pound on it for a while now ...

// traceparent: {
// type: 'text',
// },
params: {
Copy link
Member

Choose a reason for hiding this comment

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

params and state are JSON bags, so likely don't need to be indexed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was failing test because we are using _source to get this attributes and since they are not mapped anymore, source was not working anymore.

Copy link
Member

Choose a reason for hiding this comment

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

hmmm .... that doesn't seem right - the _source should always contain the JSON blob we index. It should be there regardless if it's indexed/mapped any more.

But ... not hard for me to believe that someone would use a runtime field on here, which would be a little easier to do with a field, than if it was in _source - certainly faster if for some reason this is done at runtime. Maybe telemetry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me re-check/search and give you the right answer here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok You are right! we were able to not index these two, I tried multiple time and tests are not failing!

@XavierM
Copy link
Contributor Author

XavierM commented Apr 13, 2023

@JiaweiWu Do you see any problem with that 0199f45 ?

@JiaweiWu
Copy link
Contributor

@JiaweiWu Do you see any problem with that 0199f45 ?

Looks good, we don't do any sorting/filtering on rule settings at the moment.

Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

Woohoo! Got rid of the JSON bags!

LGTM!

@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
action 6 4 -2
action_task_params 5 - -5
alert 101 68 -33
connector_token 7 3 -4
rules-settings 9 - -9
total -53
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 432 435 +3

Total ESLint disabled count

id before after diff
securitySolution 512 515 +3

History

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

@XavierM XavierM merged commit ad2d5ad into elastic:main Apr 18, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 18, 2023
Comment on lines 137 to 140
monitoring: {
properties: {
run: {
properties: {
Copy link
Contributor

Choose a reason for hiding this comment

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

@XavierM Sorry I'm a bit late with this comment since the PR has been already merged, but:

Until it's too late and we can't introduce non-additive changes to the mappings anymore, can we consolidate and refactor the monitoring and lastRun objects? These two objects contain similar data, and it's confusing for a lot of devs, including me, @jpdjere, @maximpn etc.

You can see how our code looks like that has to deal with these two objects, it's more complex than should be:

https://github.com/elastic/kibana/blob/7e633bb063051f60e5c26a1ed9fa243c2cd5b1bb/x-pack/plugins/security_solution/server/lib/detection_engine/rule_monitoring/logic/rule_execution_log/create_rule_execution_summary.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants