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

[Fleet] added time_series_metric mapping for metric_type package field #126322

Merged
merged 2 commits into from
Feb 24, 2022
Merged

[Fleet] added time_series_metric mapping for metric_type package field #126322

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3679b56
moved metric_type to time_series_metric mapping
juliaElastic Feb 24, 2022
f68d086
added back meta.metric_type for bwc, made time_series_metric to string
juliaElastic Feb 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 29 additions & 22 deletions x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,42 +714,54 @@ describe('EPM template', () => {
expect(mappings).toEqual(expectedMapping);
});

it('processes meta fields', () => {
const metaFieldLiteralYaml = `
- name: fieldWithMetas
type: integer
unit: byte
it('tests processing metric_type field', () => {
const literalYml = `
- name: total.norm.pct
type: scaled_float
metric_type: gauge
`;
const metaFieldMapping = {
unit: percent
format: percent
`;
const expectedMapping = {
properties: {
fieldWithMetas: {
type: 'long',
meta: {
metric_type: 'gauge',
unit: 'byte',
total: {
properties: {
norm: {
properties: {
pct: {
scaling_factor: 1000,
type: 'scaled_float',
meta: {
unit: 'percent',
},
time_series_metric: {
type: 'gauge',
},
},
},
},
},
},
},
};
const fields: Field[] = safeLoad(metaFieldLiteralYaml);
const fields: Field[] = safeLoad(literalYml);
const processedFields = processFields(fields);
const mappings = generateMappings(processedFields);
expect(JSON.stringify(mappings)).toEqual(JSON.stringify(metaFieldMapping));
expect(mappings).toEqual(expectedMapping);
});

it('processes meta fields with only one meta value', () => {
it('processes meta fields', () => {
const metaFieldLiteralYaml = `
- name: fieldWithMetas
type: integer
metric_type: gauge
unit: byte
`;
const metaFieldMapping = {
properties: {
fieldWithMetas: {
type: 'long',
meta: {
metric_type: 'gauge',
unit: 'byte',
},
},
},
Expand All @@ -765,16 +777,13 @@ describe('EPM template', () => {
- name: groupWithMetas
type: group
unit: byte
metric_type: gauge
fields:
- name: fieldA
type: integer
unit: byte
metric_type: gauge
- name: fieldB
type: integer
unit: byte
metric_type: gauge
`;
const metaFieldMapping = {
properties: {
Expand All @@ -783,14 +792,12 @@ describe('EPM template', () => {
fieldA: {
type: 'long',
meta: {
metric_type: 'gauge',
unit: 'byte',
},
},
fieldB: {
type: 'long',
meta: {
metric_type: 'gauge',
unit: 'byte',
},
},
Expand Down
4 changes: 3 additions & 1 deletion x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,9 @@ export function generateMappings(fields: Field[]): IndexTemplateMappings {
case 'scaled_float':
fieldProps.type = 'scaled_float';
fieldProps.scaling_factor = field.scaling_factor || DEFAULT_SCALING_FACTOR;
if (field.metric_type) {
fieldProps.time_series_metric = { type: field.metric_type };
}
break;
case 'text':
const textMapping = generateTextMapping(field);
Expand Down Expand Up @@ -193,7 +196,6 @@ export function generateMappings(fields: Field[]): IndexTemplateMappings {
break;
default: {
const meta = {};
if ('metric_type' in field) Reflect.set(meta, 'metric_type', field.metric_type);
Copy link
Contributor Author

@juliaElastic juliaElastic Feb 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

question: can metric_type just be removed from meta or does it need to be kept/migrated for BWC?

it looks like the change of adding time_series_metric causes errors when installing system or apm package:

 │ proc [kibana] [2022-02-24T13:30:17.599+01:00][ERROR][plugins.fleet] Error: Error installing system 1.11.0: illegal_argument_exception: [illegal_argument_exception] Reason: composable template [metrics-system.filesystem] template after composition with component templates [metrics-system.filesystem@settings, metrics-system.filesystem@custom, .fleet_component_template-1] is invalid
   │ proc [kibana]     at ensureInstalledPackage (/Users/juliabardi/kibana/kibana/x-pack/plugins/fleet/server/services/epm/packages/install.ts:129:11)

Copy link
Member

Choose a reason for hiding this comment

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

question: can metric_type just be removed from meta or does it need to be kept/migrated for BWC?

I don't know of anything using the meta field, but maybe we can keep it for some time just in case? On this comment #82273 (comment) @ruflin also seemed to prefer keeping both approaches during some time.

it looks like the change of adding time_series_metric causes errors when installing system or apm package:

Regarding the error, could it be that aggregations is mandatory when using the "object" format? In the examples in elastic/elasticsearch#74014, this format is only used when aggregations is also present.

Maybe we have to use this format when there are no aggregations as is the case now:

              "time_series_metric": "gauge"

@csoulios is it ok to set only the metric type without aggregations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, it works if I change to string like "time_series_metric": "gauge". If later we need to support aggregations, is it going to be possible to change from string to object?

Copy link

@csoulios csoulios Feb 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Currently, the time_series_metric field only supports a string value that can be any of the gauge, counter, histogram, summary. The object format is not currently supported.

I would like to ask which of the above values do you plan to use.

See elastic/elasticsearch#76766

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'm not sure what are all the possible values of metric_type/time_series_metric, maybe @jsoriano knows?
It looks like the string format works, so we can go ahead with that, let me know if any concerns.

Copy link
Member

Choose a reason for hiding this comment

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

By now we only allow counter and gauge in packages.

if ('unit' in field) Reflect.set(meta, 'unit', field.unit);
fieldProps.meta = meta;
}
Expand Down