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

[Fleet] Set default settings in component template instead of the index template #111197

Conversation

nchaulet
Copy link
Member

@nchaulet nchaulet commented Sep 3, 2021

Description

Resolve #105603

Why? To allow to override settings in template-component@custom component template settings need to be defined in a component template.

That PR change that and put the default settings in the template-component@settings component templates.

I did some refacto by moving this his own function and added some unit test we were missing here.

Known issue: this is going to be applied to newly installed package only and not to already installed package.

The index lifecycle policy is still defined in the index template? should we move it to the component template too? Update all the index settings are now in the component template @settings.

How to test?

In a package:

  • you should be able to specify index template settings

In a custom component template:

  • You should be able to specify the settings index.number_of_replicas in a custom component template.

@nchaulet nchaulet added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Sep 3, 2021
@nchaulet nchaulet self-assigned this Sep 3, 2021
@nchaulet nchaulet requested a review from a team as a code owner September 3, 2021 18:21
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@nchaulet
Copy link
Member Author

nchaulet commented Sep 3, 2021

@elasticmachine merge upstream

@hop-dev
Copy link
Contributor

hop-dev commented Sep 7, 2021

@nchaulet (cc @joshdover) Regarding the Index Lifecycle Policy, I have been looking at how users can customise data retention for integrations as part of elastic/observability-docs#986.

In the simplest case, where they wish to customise the retention of data for an integration for all namespaces, they could use the *@custom component template to set their own ILM policy, but I am not sure how realistic a use case this is and if we want to cater for it @joshdover?

If it isn't much trouble I think I would lean towards moving it to the component template

@nchaulet
Copy link
Member Author

nchaulet commented Sep 7, 2021

@hop-dev I think it make sense too. I just moved the ILM settings to the component template.

@joshdover
Copy link
Contributor

In the simplest case, where they wish to customise the retention of data for an integration for all namespaces, they could use the *@custom component template to set their own ILM policy, but I am not sure how realistic a use case this is and if we want to cater for it @joshdover?

If it isn't much trouble I think I would lean towards moving it to the component template

Good catch @hop-dev, we need the custom component template to at least be able to set this for all namespaces. Thanks for making the change @nchaulet.

@joshdover joshdover self-requested a review September 7, 2021 13:19
ilmPolicy,
type,
}: {
type: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can type here only be logs or metrics? Should we express that in the type?

Copy link
Member Author

Choose a reason for hiding this comment

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

datastream.type is not typed to only logs and metrics, I think there is some other options too synthetics for example, I would keep this as string for now.

},
},
// This is the default from Beats? So far seems to be a good value
refresh_interval: '5s',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is now a good time to look at some of @ruflin 's recommendations here #104620 and trim some of these down? e.g potentially remove number_of_shards and refresh_interval

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Should we go ahead and incorporate the changes to the defaults that Adrien recommended here: #104620 (comment)

Comment on lines +67 to +68
// What should be our default for the compression?
codec: 'best_compression',
Copy link
Contributor

Choose a reason for hiding this comment

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

@ruflin Do you know if we really should be using best_compression here? It appears Beats does not use this value and it does slow down field retrieval (though I don't know by how much):

The default value compresses stored data with LZ4 compression, but this can be set to best_compression which uses DEFLATE for a higher compression ratio, at the expense of slower stored fields performance. If you are updating the compression type, the new one will be applied after segments are merged. Segment merging can be forced using force merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jpountz curious if you have any input on this as well

Copy link

Choose a reason for hiding this comment

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

In my opinion, it is sensible to use best_speed by default for Enterprise Search data and best_compression for Observability and Security data.

…e/default_settings.test.ts

Co-authored-by: Josh Dover <1813008+joshdover@users.noreply.github.com>
@nchaulet
Copy link
Member Author

nchaulet commented Sep 7, 2021

@joshdover @hop-dev For #104620 I think the changes for the default value could come in a followup PR, it seems related but I like the benefits of having two PR/commits here it make things easier to follow, track future bugs. I will be happy to tackle #104620 later, if you want.

@nchaulet nchaulet requested a review from joshdover September 7, 2021 14:43
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @nchaulet

@nchaulet nchaulet merged commit b5f8d2e into elastic:master Sep 7, 2021
@nchaulet nchaulet deleted the feature-105603-set-default-settings-in-component-template branch September 7, 2021 16:09
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 7, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Sep 7, 2021
…ex template (#111197) (#111419)

Co-authored-by: Nicolas Chaulet <nicolas.chaulet@elastic.co>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 8, 2021
…-link-to-kibana-app

* 'master' of github.com:elastic/kibana: (61 commits)
  [Logs UI] Fix alert previews for thresholds of `0` (elastic#111150)
  [Archive Migration][Partial] discover apps-discover (elastic#110437)
  [APM] Set start date of APM ML job to -4 weeks (elastic#111375)
  [ML] APM Latency Correlations: Code consolidation. (elastic#110790)
  [Discover] Fix indices permission for multiline test (elastic#111284)
  [Detection Rules] Add 7.15 rules (elastic#111464)
  [Security Solution][Endpoint][Host Isolation] Hide isolate host option in alert details rather than disabling (elastic#111064)
  React version of angular license view (elastic#111317)
  [APM] Fix link in readme (elastic#111362)
  [Security Solution] add agent field to generator (elastic#111428)
  [Dashboard] Retain Tags on Quicksave (elastic#111015)
  Reorder App Search ingestion methods (elastic#111361)
  Port performance docs to new docs system. (elastic#111063)
  [Security Solution][RAC] Fixes updatedAt loading bug (elastic#111010)
  [sample data] update web log geo.src field to match country code of geo.coordinates (elastic#110885)
  [Security solution] [Endpoint] Fix bad artifact migration (elastic#111294)
  Fix copy typo. (elastic#111203)
  [build] Remove empty optimize directory (elastic#111393)
  [Maps] fix term join not updating when editing right field (elastic#111030)
  [Fleet] Set default settings in component template instead of the index template (elastic#111197)
  ...

# Conflicts:
#	x-pack/plugins/reporting/public/management/__snapshots__/report_listing.test.tsx.snap
#	x-pack/plugins/reporting/public/management/report_listing.test.tsx
chrisronline pushed a commit to chrisronline/kibana that referenced this pull request Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fleet] Set default settings in component template instead of default template
6 participants