Skip to content

Commit

Permalink
[Fleet] experimental toggles for doc-value-only (#149131)
Browse files Browse the repository at this point in the history
## Summary

Closes #144357

WIP. Review can be started, but still requires a lot of testing and
fixing the issue below.

How to test locally:
- Turn on `experimentalDataStreamSettings` feature flag
- Go to Add integration, System integration
- On the first data stream, turn on the Doc value only switches, Save
- The mapping changes are visible under Stack Management / Index
Management / Component Templates e.g. `logs-system.auth@package`
- The numeric switch sets `index:false` on all numeric field mappings
(long, double, etc.)
- The other switch sets `index:false` on all other field type mappings
that support it (keyword, ip, date, etc.)
- The new mappings will take effect after rollover

<img width="475" alt="image"
src="https://user-images.githubusercontent.com/90178898/213206641-13ead2fc-f079-407c-9c0e-c58f99dd4903.png">
<img width="1037" alt="image"
src="https://user-images.githubusercontent.com/90178898/213495546-9962c458-590b-4787-bf2d-9f19abea3f67.png">

What works:
- When turning the new doc-value-only numeric and other checkboxes on or
off, the corresponding mapping changes are done in the component
template
- The logic also reads the package spec's template and preserves the
`index:false` values regardless of the switch (tested manually by
setting `@timestamp` field to `index:false` in the template, there is
also the `original` field in `logs-system.auth@package` stream that is
set to `index:false` in the package by default.

```
"original": {
    "index": false,
    "doc_values": false,
    "type": "keyword"
},
```

Pending:
- Add/update unit and integration tests to verify the mapping change
logic - DONE
- Manual testing (turning the switches on/off, create/update package
policy, upgrade package) - DONE
- Clarify TODOs in the code about the supported types - DONE
- Hitting an issue when turning on `doc-value-only` for "other" types
(keyword, date, etc.). Could be that one of the fields doesn't support
`index:false` setting. Didn't experience this when turning on only the
numeric types. - FIXED
```
illegal_argument_exception: [illegal_argument_exception] Reason: updating component template [logs-system.auth@package] results in invalid composable template [logs-system.auth] after templates are merged
```

EDIT: found the root cause of this: `Caused by:
java.lang.IllegalArgumentException: data stream timestamp field
[@timestamp] is not indexed`


### Checklist

Delete any items that are not applicable to this PR.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

Co-authored-by: Kibana Machine <[email protected]>
  • Loading branch information
juliaElastic and kibanamachine authored Jan 25, 2023
1 parent e2824c3 commit 179b36f
Show file tree
Hide file tree
Showing 12 changed files with 510 additions and 55 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ describe('checking migration metadata changes on all registered SO types', () =>
"endpoint:user-artifact": "f94c250a52b30d0a2d32635f8b4c5bdabd1e25c0",
"endpoint:user-artifact-manifest": "8c14d49a385d5d1307d956aa743ec78de0b2be88",
"enterprise_search_telemetry": "fafcc8318528d34f721c42d1270787c52565bad5",
"epm-packages": "7d80ba3f1fcd80316aa0b112657272034b66d5a8",
"epm-packages": "21e096cf4554abe1652953a6cd2119d68ddc9403",
"epm-packages-assets": "9fd3d6726ac77369249e9a973902c2cd615fc771",
"event_loop_delays_daily": "d2ed39cf669577d90921c176499908b4943fb7bd",
"exception-list": "fe8cc004fd2742177cdb9300f4a67689463faf9c",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ describe('toPackagePolicy', () => {
features: {
synthetic_source: true,
tsdb: false,
doc_value_only_numeric: false,
doc_value_only_other: false,
},
},
],
Expand All @@ -144,6 +146,8 @@ describe('toPackagePolicy', () => {
features: {
synthetic_source: true,
tsdb: false,
doc_value_only_numeric: false,
doc_value_only_other: false,
},
},
]);
Expand Down
6 changes: 5 additions & 1 deletion x-pack/plugins/fleet/common/types/models/epm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,11 @@ export type PackageInfo =
| Installable<Merge<ArchivePackage, EpmPackageAdditions>>;

// TODO - Expand this with other experimental indexing types
export type ExperimentalIndexingFeature = 'synthetic_source' | 'tsdb';
export type ExperimentalIndexingFeature =
| 'synthetic_source'
| 'tsdb'
| 'doc_value_only_numeric'
| 'doc_value_only_other';

export interface ExperimentalDataStreamFeature {
data_stream: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ describe('ExperimentDatastreamSettings', () => {
features: {
synthetic_source: false,
tsdb: false,
doc_value_only_numeric: false,
doc_value_only_other: false,
},
},
]}
Expand Down Expand Up @@ -141,6 +143,8 @@ describe('ExperimentDatastreamSettings', () => {
features: {
synthetic_source: true,
tsdb: false,
doc_value_only_numeric: false,
doc_value_only_other: false,
},
},
];
Expand Down Expand Up @@ -169,6 +173,8 @@ describe('ExperimentDatastreamSettings', () => {
features: {
synthetic_source: false,
tsdb: false,
doc_value_only_numeric: false,
doc_value_only_other: false,
},
},
]);
Expand All @@ -178,6 +184,8 @@ describe('ExperimentDatastreamSettings', () => {
features: {
synthetic_source: true,
tsdb: false,
doc_value_only_numeric: false,
doc_value_only_other: false,
},
},
]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,18 @@ export const ExperimentDatastreamSettings: React.FunctionComponent<Props> = ({
const isSyntheticSourceEnabledByDefault =
registryDataStream.elasticsearch?.source_mode === 'synthetic' || isTimeSeriesEnabledByDefault;

const docValueOnlyNumericExperimentalValue = getExperimentalFeatureValue(
'doc_value_only_numeric',
experimentalDataFeatures ?? [],
registryDataStream
);

const docValueOnlyOtherExperimentalValue = getExperimentalFeatureValue(
'doc_value_only_other',
experimentalDataFeatures ?? [],
registryDataStream
);

const newExperimentalIndexingFeature = {
synthetic_source:
typeof syntheticSourceExperimentalValue !== 'undefined'
Expand All @@ -73,6 +85,14 @@ export const ExperimentDatastreamSettings: React.FunctionComponent<Props> = ({
? isTimeSeriesEnabledByDefault
: getExperimentalFeatureValue('tsdb', experimentalDataFeatures ?? [], registryDataStream) ??
false,
doc_value_only_numeric:
typeof docValueOnlyNumericExperimentalValue !== 'undefined'
? docValueOnlyNumericExperimentalValue
: false,
doc_value_only_other:
typeof docValueOnlyOtherExperimentalValue !== 'undefined'
? docValueOnlyOtherExperimentalValue
: false,
};

const onIndexingSettingChange = (
Expand Down Expand Up @@ -178,6 +198,40 @@ export const ExperimentDatastreamSettings: React.FunctionComponent<Props> = ({
/>
</EuiToolTip>
</EuiFlexItem>
<EuiFlexItem>
<EuiSwitch
checked={newExperimentalIndexingFeature.doc_value_only_numeric ?? false}
data-test-subj="packagePolicyEditor.docValueOnlyNumericExperimentalFeature.switch"
label={
<FormattedMessage
id="xpack.fleet.packagePolicyEditor.experimentalFeatures.docValueOnlyNumericLabel"
defaultMessage="Doc value only (numeric types)"
/>
}
onChange={(e) => {
onIndexingSettingChange({
doc_value_only_numeric: e.target.checked,
});
}}
/>
</EuiFlexItem>
<EuiFlexItem>
<EuiSwitch
checked={newExperimentalIndexingFeature.doc_value_only_other ?? false}
data-test-subj="packagePolicyEditor.docValueOnlyOtherExperimentalFeature.switch"
label={
<FormattedMessage
id="xpack.fleet.packagePolicyEditor.experimentalFeatures.docValueOnlyOtherLabel"
defaultMessage="Doc value only (other types)"
/>
}
onChange={(e) => {
onIndexingSettingChange({
doc_value_only_other: e.target.checked,
});
}}
/>
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlexItem>
);
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/fleet/server/saved_objects/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,7 @@ const getSavedObjectTypes = (
data_stream: { type: 'keyword' },
features: {
type: 'nested',
dynamic: false,
properties: {
synthetic_source: { type: 'boolean' },
tsdb: { type: 'boolean' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,8 @@ describe('EPM index template install', () => {
features: {
synthetic_source: false,
tsdb: false,
doc_value_only_numeric: false,
doc_value_only_other: false,
},
},
});
Expand Down Expand Up @@ -241,6 +243,8 @@ describe('EPM index template install', () => {
features: {
synthetic_source: false,
tsdb: false,
doc_value_only_numeric: false,
doc_value_only_other: false,
},
},
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ import {
import { getESAssetMetadata } from '../meta';
import { retryTransientEsErrors } from '../retry';

import {
applyDocOnlyValueToMapping,
forEachMappings,
} from '../../../experimental_datastream_features_helper';

import {
generateMappings,
generateTemplateName,
Expand Down Expand Up @@ -262,6 +267,7 @@ export function buildComponentTemplates(params: {
defaultSettings,
mappings,
pipelineName,
experimentalDataStreamFeature,
} = params;
const packageTemplateName = `${templateName}${PACKAGE_TEMPLATE_SUFFIX}`;
const userSettingsTemplateName = `${templateName}${USER_SETTINGS_TEMPLATE_SUFFIX}`;
Expand All @@ -275,6 +281,23 @@ export function buildComponentTemplates(params: {

const indexTemplateMappings = registryElasticsearch?.['index_template.mappings'] ?? {};

const isDocValueOnlyNumericEnabled =
experimentalDataStreamFeature?.features.doc_value_only_numeric === true;
const isDocValueOnlyOtherEnabled =
experimentalDataStreamFeature?.features.doc_value_only_other === true;

if (isDocValueOnlyNumericEnabled || isDocValueOnlyOtherEnabled) {
forEachMappings(mappings.properties, (mappingProp, name) =>
applyDocOnlyValueToMapping(
mappingProp,
name,
experimentalDataStreamFeature,
isDocValueOnlyNumericEnabled,
isDocValueOnlyOtherEnabled
)
);
}

const mappingsProperties = merge(mappings.properties, indexTemplateMappings.properties ?? {});

const mappingsDynamicTemplates = uniqBy(
Expand All @@ -286,8 +309,8 @@ export function buildComponentTemplates(params: {
const isSyntheticSourceEnabledByDefault = registryElasticsearch?.source_mode === 'synthetic';

const sourceModeSynthetic =
params.experimentalDataStreamFeature?.features.synthetic_source !== false &&
(params.experimentalDataStreamFeature?.features.synthetic_source === true ||
experimentalDataStreamFeature?.features.synthetic_source !== false &&
(experimentalDataStreamFeature?.features.synthetic_source === true ||
isSyntheticSourceEnabledByDefault ||
isTimeSeriesEnabledByDefault);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import type {
MappingProperty,
PropertyName,
} from '@elastic/elasticsearch/lib/api/typesWithBodyKey';

import type { ExperimentalDataStreamFeature } from '../../common/types';

export const forEachMappings = (
mappingProperties: Record<PropertyName, MappingProperty>,
process: (prop: MappingProperty, name: string) => void
) => {
Object.keys(mappingProperties).forEach((mapping) => {
const property = mappingProperties[mapping] as any;
if (property.properties) {
forEachMappings(property.properties, process);
} else {
process(property, mapping);
}
});
};

export const applyDocOnlyValueToMapping = (
mappingProp: MappingProperty,
name: string,
featureMap: ExperimentalDataStreamFeature,
isDocValueOnlyNumericChanged: boolean,
isDocValueOnlyOtherChanged: boolean
) => {
const mapping = mappingProp as any;

const numericTypes = [
'long',
'integer',
'short',
'byte',
'double',
'float',
'half_float',
'scaled_float',
'unsigned_long',
];
if (isDocValueOnlyNumericChanged && numericTypes.includes(mapping.type ?? '')) {
updateMapping(mapping, featureMap.features.doc_value_only_numeric);
}

const otherSupportedTypes = ['date', 'date_nanos', 'boolean', 'ip', 'geo_point', 'keyword'];
if (
isDocValueOnlyOtherChanged &&
name !== '@timestamp' &&
otherSupportedTypes.includes(mapping.type ?? '')
) {
updateMapping(mapping, featureMap.features.doc_value_only_other);
}
};

const updateMapping = (mapping: { index?: boolean }, featureValue: boolean) => {
if (featureValue === false && mapping.index === false) {
delete mapping.index;
}
if (featureValue && !mapping.index) {
mapping.index = false;
}
};
Loading

0 comments on commit 179b36f

Please sign in to comment.