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

[dot-kibana-split] Adapt usages of core.savedObjects.getKibanaIndex to use the correct index #155155

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Apr 18, 2023

this PR is targeting the dot-kibana-split feature branch and not main

Summary

Part of #154888

In #154888, we're going to split the .kibana savedObject index into multiple ones. For this reason, calls to core.savedObjects.getKibanaIndex will not necessarily return the correct value (e.g types that were moved out of this index)

This PR introduces the following SO APIs:

  • getDefaultIndex
  • getIndexForType
  • getIndicesForTypes
  • getAllIndices

And adapt plugins code to replace usages of core.savedObjects.getKibanaIndex with the proper alternative

@pgayvallet pgayvallet changed the title Add SO service APIs to retrieve the indices associated with one or more types [kibana-split] Adapt usages of core.savedObjects.getKibanaIndex to use the correct index Apr 18, 2023
@pgayvallet pgayvallet force-pushed the kbn-so-split-get-kibana-index-api-change branch from 7d87a8e to a3a2c2d Compare April 19, 2023 06:17
@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Saved Objects release_note:skip Skip the PR/issue when compiling release notes v8.8.0 labels Apr 19, 2023
Copy link
Contributor Author

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

Self review

Comment on lines +226 to +238
/**
* Returns the (alias to the) index that the specified saved object type is stored in.
*
* @param type The SO type to retrieve the index/alias for.
*/
getIndexForType: (type: string) => string;
/**
* Returns the (alias to the) index that the specified saved object type is stored in.
*
* @remark if multiple types are living in the same index, duplicates will be removed.
* @param types The SO types to retrieve the index/alias for.
*/
getIndicesForTypes: (types: string[]) => string[];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TSDoc was added fairly quickly, please tell me if we want to rephrase stuff in here.

Copy link
Contributor

Choose a reason for hiding this comment

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

NIT plurals here would be nice:

 /**
  * Returns the (aliases to the) indices that the specified saved object types are stored in.
  *
  * @remark an index that matches multiple types will only be returned once, aka duplicates will be removed.
  * @param types The SO types to retrieve the indices/aliases for.
  */

Comment on lines +83 to +85
const indices = await getIndexForTypes(['dashboard', 'visualization', 'search']);
return {
index: kibanaIndex,
index: indices[0],
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 couldn't change the shape of the collector's data, so index has to stay a single string. In practice it's fine, given all the types counted here are in a single index.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, the getKibanaSavedObjectCounts() will aggregate data from all SO indices under the hood.
Then, from the stats perspective, it is fine if we still report them as being part of the .kibana index. Correct?
Ping @afharo for awareness

@@ -98,7 +98,7 @@ export class UsageCollectionPlugin implements Plugin<UsageCollectionSetup> {

public setup(core: CoreSetup): UsageCollectionSetup {
const config = this.initializerContext.config.get<ConfigType>();
const kibanaIndex = core.savedObjects.getKibanaIndex();
const kibanaIndex = core.savedObjects.getDefaultIndex();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only used to generate the stats shape, so using the default index here is fine.

@@ -61,13 +61,11 @@ export class Plugin implements CorePlugin<IEventLogService, IEventLogClientServi
}

setup(core: CoreSetup): IEventLogService {
const kibanaIndex = core.savedObjects.getKibanaIndex();

const kibanaIndex = core.savedObjects.getDefaultIndex();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only used for stats shape too, using the default index here is fine

@pgayvallet pgayvallet marked this pull request as ready for review April 19, 2023 08:49
@pgayvallet pgayvallet requested review from a team as code owners April 19, 2023 08:49
@pgayvallet pgayvallet requested review from a team as code owners April 19, 2023 08:49
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@pgayvallet pgayvallet requested a review from a team as a code owner April 19, 2023 08:49
@pgayvallet pgayvallet changed the title [kibana-split] Adapt usages of core.savedObjects.getKibanaIndex to use the correct index [dot-kibana-split] Adapt usages of core.savedObjects.getKibanaIndex to use the correct index Apr 19, 2023
Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

ML changes LGTM

Copy link
Contributor

@thomheymann thomheymann left a comment

Choose a reason for hiding this comment

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

Security changes LGTM!

@@ -138,7 +137,12 @@ export class KibanaUsageCollectionPlugin implements Plugin {
registerUsageCountersUsageCollector(usageCollection);

registerOpsStatsCollector(usageCollection, metric$);
registerKibanaUsageCollector(usageCollection, kibanaIndex);

const getIndexForTypes = (types: string[]) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT we could rename this to getIndicesForTypes, for consistency.

@@ -43,7 +43,7 @@ export async function getKibanaSavedObjectCounts(

export function registerKibanaUsageCollector(
usageCollection: UsageCollectionSetup,
kibanaIndex: string
getIndexForTypes: (types: string[]) => Promise<string[]>
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT same as the other comment, we could rename to getIndicesForTypes.

@@ -23,6 +23,7 @@ import { type RawPackageInfo, Env } from '@kbn/config';
import { ByteSizeValue } from '@kbn/config-schema';
import { REPO_ROOT } from '@kbn/repo-info';
import { getEnvOptions } from '@kbn/config-mocks';
import { SavedObjectsType, MAIN_SAVED_OBJECT_INDEX } from '@kbn/core-saved-objects-server';
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT SavedObjectsType can be imported as a type

Copy link
Contributor

@gsoldevila gsoldevila left a comment

Choose a reason for hiding this comment

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

LGTM with a few NITs

@pgayvallet pgayvallet merged commit 93e96aa into elastic:dot-kibana-split Apr 19, 2023
@pgayvallet
Copy link
Contributor Author

Merging.

7iozuy

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

API count

id before after diff
@kbn/core-saved-objects-server 497 504 +7

ESLint disabled line counts

id before after diff
securitySolution 435 434 -1

References to deprecated APIs

id before after diff
@kbn/core-plugins-server-internal 10 12 +2
@kbn/core-saved-objects-server-internal 6 7 +1
@kbn/core-saved-objects-server-mocks 0 2 +2
total +5

Total ESLint disabled count

id before after diff
securitySolution 515 514 -1

History

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

mohamedhamed-ahmed

This comment was marked as duplicate.

gsoldevila added a commit that referenced this pull request Apr 25, 2023
## Description 

Fix #104081

This PR move some of the SO types from the `.kibana` index into the
following ones:
- `.kibana_alerting_cases`
- `.kibana_analytics`
- `.kibana_security_solution`
- `.kibana_ingest`

This split/reallocation will occur during the `8.8.0` Kibana upgrade
(*meaning: from any version older than `8.8.0` to any version greater or
equal to `8.8.0`*)

**This PR main changes are:**
- implement the changes required in the SO migration algorithm to
support this reallocation
- update the FTR tools (looking at you esArchiver) to support these new
indices
- update hardcoded references to `.kibana` and usage of the
`core.savedObjects.getKibanaIndex()` to use new APIs to target the
correct index/indices
- update FTR datasets, tests and utility accordingly 

## To reviewers

**Overall estimated risk of regressions: low**

But, still, please take the time to review changes in your code. The
parts of the production code that were the most impacted are the
telemetry collectors, as most of them were performing direct requests
against the `.kibana` index, so we had to adapt them. Most other
contributor-owned changes are in FTR tests and datasets.

If you think a type is misplaced (either we missed some types that
should be moved to a specific index, or some types were moved and
shouldn't have been) please tell us, and we'll fix the reallocation
either in this PR or in a follow-up.

## .Kibana split

The following new indices are introduced by this PR, with the following
SO types being moved to it. (any SO type not listed here will be staying
in its current index)

Note: The complete **_type => index_** breakdown is available in [this
spreadsheet](https://docs.google.com/spreadsheets/d/1b_MG_E_aBksZ4Vkd9cVayij1oBpdhvH4XC8NVlChiio/edit#gid=145920788).

#### `.kibana_alerting_cases`
- action
- action_task_params
- alert
- api_key_pending_invalidation
- cases
- cases-comments
- cases-configure
- cases-connector-mappings
- cases-telemetry
- cases-user-actions
- connector_token
- rules-settings
- maintenance-window

#### `.kibana_security_solution`
- csp-rule-template
- endpoint:user-artifact
- endpoint:user-artifact-manifest
- exception-list
- exception-list-agnostic
- osquery-manager-usage-metric
- osquery-pack
- osquery-pack-asset
- osquery-saved-query
- security-rule
- security-solution-signals-migration
- siem-detection-engine-rule-actions
- siem-ui-timeline
- siem-ui-timeline-note
- siem-ui-timeline-pinned-event

#### `.kibana_analytics`

- canvas-element
- canvas-workpad-template
- canvas-workpad
- dashboard
- graph-workspace
- index-pattern
- kql-telemetry
- lens
- lens-ui-telemetry
- map
- search
- search-session
- search-telemetry
- visualization

#### `.kibana_ingest`

- epm-packages
- epm-packages-assets
- fleet-fleet-server-host
- fleet-message-signing-keys
- fleet-preconfiguration-deletion-record
- fleet-proxy
- ingest_manager_settings
- ingest-agent-policies
- ingest-download-sources
- ingest-outputs
- ingest-package-policies

## Tasks / PRs

### Sub-PRs

**Implementation**
- 🟣 #154846
- 🟣 #154892
- 🟣 #154882
- 🟣 #154884
- 🟣 #155155

**Individual index split**
- 🟣 #154897
- 🟣 #155129
- 🟣 #155140
- 🟣 #155130

### Improvements / follow-ups 

- 👷🏼 Extract logic into
[runV2Migration](#154151 (comment))
@gsoldevila
- Make `getCurrentIndexTypesMap` resillient to intermittent failures
#154151 (comment)
- 🚧 Build a more structured
[MigratorSynchronizer](#154151 (comment))
- 🟣 #155035
- 🟣 #155116
- 🟣 #155366
## Reallocation tweaks

Tweaks to the reallocation can be done after the initial merge, as long
as it's done before the public release of 8.8

- `url` should get back to `.kibana` (see
[comment](#154888 (comment)))

## Release Note

For performance purposes, Kibana is now using more system indices to
store its internal data.

The following system indices will be created when upgrading to `8.8.0`:

- `.kibana_alerting_cases`
- `.kibana_analytics`
- `.kibana_security_solution`
- `.kibana_ingest`

---------

Co-authored-by: pgayvallet <[email protected]>
Co-authored-by: Christos Nasikas <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Georgii Gorbachev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Saved Objects release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants