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] Add support for meta in fields.yml #82273

Closed
ph opened this issue Nov 2, 2020 · 21 comments · Fixed by #100931
Closed

[Fleet] Add support for meta in fields.yml #82273

ph opened this issue Nov 2, 2020 · 21 comments · Fixed by #100931
Assignees
Labels
Team:Fleet Team label for Observability Data Collection Fleet team

Comments

@ph
Copy link
Contributor

ph commented Nov 2, 2020

In elastic/elasticsearch#61941 a standardised way on how to use meta information on fields is documented. Recent support on meta was added on the package-spec in elastic/package-spec#42. When installing the template we should also take care of the meta fields.

The meta fields are defined in the fields.yaml like this:

- name: total.norm.pct
  type: scaled_float
  metric_type: gauge
  unit: percent
  format: percent
  description: |
     The percentage of CPU time spent by the process since the last event. This value is normalized by the number of CPU cores and it ranges from 0 to 100%.

That would translate into a mapping like, see the meta key:

"system": {
  "properties": {
    "total": {
      "properties": {
        "norm": {
          "properties": {
            "pct": {
              "scaling_factor": 1000,
              "type": "scaled_float",
              "meta": {
                "metric_type": "gauge",
                "unit": "percent",
              }
            }
          }
        }
      }
    }
@ph ph added the Team:Fleet Team label for Observability Data Collection Fleet team label Nov 2, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@ph
Copy link
Contributor Author

ph commented Nov 2, 2020

@exekias Can you verify I didn't do any mistakes in the transformation?

@exekias
Copy link
Contributor

exekias commented Nov 2, 2020

LGTM, thank you!

@ruflin
Copy link
Member

ruflin commented Nov 2, 2020

@exekias Do you know if we already have an issue in Kibana to add support for this to the Kibana index patterns (or similar)?

@wylieconlon
Copy link
Contributor

Yes, here #82318

@exekias
Copy link
Contributor

exekias commented Nov 4, 2020

Thanks @wylieconlon! I will also sync with @sorantis to see if metrics explorer can leverage this too

@exekias
Copy link
Contributor

exekias commented Nov 24, 2020

System integrations is already defining these fields: elastic/integrations#402

@mostlyjason
Copy link
Contributor

@sorantis mentioned this is required for the new rollups 2.0 and ILM to work properly with our integrations.

@ebeahan
Copy link
Member

ebeahan commented Apr 15, 2021

Is it meta or _meta for fields?

The docs reflect meta for the fields and _meta for the mapping metadata.

@exekias
Copy link
Contributor

exekias commented Apr 23, 2021

You are right @ebeahan! I have updated the description and title to reflect the correct API

@exekias exekias changed the title [Fleet] Add support for _meta in fields.yml [Fleet] Add support for meta in fields.yml Apr 23, 2021
@Zacqary Zacqary self-assigned this May 25, 2021
@Zacqary
Copy link
Contributor

Zacqary commented May 28, 2021

Running into an issue generating the mappings for the system package's fsstat data stream. Given a fields.yml of:

- name: system.fsstat
  type: group
  fields:
    - name: count
      type: long
      metric_type: gauge
      description: Number of file systems found.
    - name: total_files
      type: long
      metric_type: gauge
      description: Total number of files.
    - name: total_size
      type: group
      format: bytes
      unit: byte
      metric_type: gauge
      fields:
        - name: free
          type: long
          format: bytes
          unit: byte
          metric_type: gauge
          description: |
            Total free space.
        - name: used
          type: long
          format: bytes
          unit: byte
          metric_type: gauge
          description: |
            Total used space.
        - name: total
          type: long
          format: bytes
          unit: byte
          metric_type: gauge
          description: |
            Total space (used plus free).

Note specifically this bit:

    - name: total_size
      type: group
      format: bytes
      unit: byte
      metric_type: gauge
      fields:

This causes an illegal_argument_exception when trying to run putIndexTemplate, because a field mapping isn't supposed to contain both a properties and meta field. The mapping is coming out like this:

"total_size": {
    "properties": {
        "free": {
            "type": "long",
            "meta": {
                "metric_type": "gauge",
                "unit": "byte"
            }
        },
        "used": {
            "type": "long",
            "meta": {
                "metric_type": "gauge",
                "unit": "byte"
            }
        },
        "total": {
            "type": "long",
            "meta": {
                "metric_type": "gauge",
                "unit": "byte"
            }
        }
    },
    "meta": {
        "metric_type": "gauge",
        "unit": "byte"
    }
}

I can work around it by not mapping the unit or metric_type fields for type: group fields, but it makes me wonder why these field mappings are even there in the first place. Is this a mistake in the fields.yml or should we be handling a case where a group field has these meta fields?

Note that the child fields each have the same unit and metric_type properties defined. Does the package spec expect you to be able to define unit and metric_type for all children in a group at the top-level like this and have it automatically propagate downward? If so, why the redundant definition?

@Zacqary
Copy link
Contributor

Zacqary commented May 28, 2021

Other than the issue I mentioned in the previous comment, is there any acceptance criteria for this issue besides just updating the generateMappings function that runs when installing the package? Do we need any UI changes?

@ruflin
Copy link
Member

ruflin commented May 31, 2021

  • Can you reference the file where on the group level a unit is used? I think this is a bug and likely we need to validate this during package linting / spec.
  • I would not expect any UI changes to be required.

@Zacqary
Copy link
Contributor

Zacqary commented Jun 1, 2021

@ruflin
Copy link
Member

ruflin commented Jun 7, 2021

This definitively looks like a bug. I'll follow up.

@ruflin
Copy link
Member

ruflin commented Jun 7, 2021

I filed as a follow up: elastic/integrations#1091

@jsoriano
Copy link
Member

jsoriano commented Jun 15, 2021

@ph @ruflin @Zacqary the elasticsearch team is going to add new mapping parameters to annotate dimensions and metrics (elastic/elasticsearch#74014). When compared to meta, parameters have the advantage of allowing to implement hard validation, apart of giving metrics the level of first class citizens in ES indexes.

There will be a parameter for the metric type, that would overlap with the metric_type meta added here. What do you think about waiting till this approach is implemented, and use these parameters?

Another possible approach might be to go on with meta by now, in case Kibana is going to leverage it soon, and migrate to the mapping parameter when ready.

There are no plans for units (this is probably only needed for representation), so this information could continue in the unit meta. Same thing for format.

Thoughts?

@Zacqary
Copy link
Contributor

Zacqary commented Jun 15, 2021

@jsoriano As this change is already merged I think we should go with the migration approach once the metric_type parameter gets pushed. Let's keep tabs on that ES issue and implement the migration when it's ready.

@ruflin
Copy link
Member

ruflin commented Jun 16, 2021

++ on moving forward with the metric_type as we already have it in packages and it took some time to get it in. My assumption is that we will be able to have an overlap of the two approaches for some time.

@jsoriano
Copy link
Member

Sounds good, thanks!

@jsoriano
Copy link
Member

As discussed here, created issue to add support for the new mappings: #115621

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Fleet Team label for Observability Data Collection Fleet team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants