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] added time_series_metric mapping for metric_type package field #126322

Merged
merged 2 commits into from
Feb 24, 2022

Conversation

juliaElastic
Copy link
Contributor

@juliaElastic juliaElastic commented Feb 24, 2022

Summary

Closes #115621

Moved metric_type to time_series_metric mapping, removed from meta.

Example:

- name: total.norm.pct
  type: scaled_float
  metric_type: gauge
  unit: percent
  format: percent

Resulting mapping:

  "properties": {
    "total": {
      "properties": {
        "norm": {
          "properties": {
            "pct": {
              "scaling_factor": 1000,
              "type": "scaled_float",
              "meta": {
                "unit": "percent"
              },
              "time_series_metric": {
                "type": "gauge"
              }
            }
          }
        }
      }
    }

Checklist

@juliaElastic juliaElastic added backport:skip This commit does not require backporting release_note:feature Makes this part of the condensed release notes v8.2.0 labels Feb 24, 2022
@juliaElastic juliaElastic requested a review from a team as a code owner February 24, 2022 10:05
@juliaElastic juliaElastic self-assigned this Feb 24, 2022
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Feb 24, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

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

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

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.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @juliaElastic

Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

Code looks good 🚀

@juliaElastic juliaElastic changed the title [Fleet] moved metric_type to time_series_metric mapping [Fleet] added time_series_metric mapping for metric_type package field Feb 24, 2022
@juliaElastic juliaElastic merged commit bb0e70f into elastic:main Feb 24, 2022
@juliaElastic juliaElastic deleted the new-mappings-metrics branch February 24, 2022 16:35
@jen-huang jen-huang added release_note:enhancement and removed release_note:feature Makes this part of the condensed release notes labels Feb 24, 2022
lucasfcosta pushed a commit to lucasfcosta/kibana that referenced this pull request Mar 2, 2022
elastic#126322)

* moved metric_type to time_series_metric mapping

* added back meta.metric_type for bwc, made time_series_metric to string
@juliaElastic
Copy link
Contributor Author

manual test case here: #115621 (comment)

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:enhancement Team:Fleet Team label for Observability Data Collection Fleet team v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for new mapping parameters for metrics defined in packages
7 participants