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

No meta info for groups #1091

Closed
ruflin opened this issue Jun 7, 2021 · 10 comments · Fixed by #1114
Closed

No meta info for groups #1091

ruflin opened this issue Jun 7, 2021 · 10 comments · Fixed by #1114
Labels
Team:Integrations Label for the Integrations team

Comments

@ruflin
Copy link
Contributor

ruflin commented Jun 7, 2021

In elastic/kibana#82273 the issue was raised that units are set on a group object: https://github.com/elastic/integrations/blob/master/packages/system/data_stream/fsstat/fields/fields.yml#L15-L16

First, I think this should not happen and we should fix the package. @fearful-symmetry I guess this falls on your plate?

Second, we should have a discussion if we should validate this also in the package spec?  @mtojek @ycombinator

Last, to support also older packages, do we need a workaround in Kibana (@Zacqary )?

@ruflin ruflin added the Team:Integrations Label for the Integrations team label Jun 7, 2021
@elasticmachine
Copy link

Pinging @elastic/integrations (Team:Integrations)

@mtojek
Copy link
Contributor

mtojek commented Jun 7, 2021

Second, we should have a discussion if we should validate this also in the package spec?

I think this can be handled with semantic validation rules.

EDIT:

I'm going with semantic rules.

@fearful-symmetry
Copy link
Contributor

So, is the issue that we don't want unit and metric_type in groups at all? Agreed we should add some validation rules, until then @mtojek should I just start fixing things manually?

@mtojek
Copy link
Contributor

mtojek commented Jun 8, 2021

So, is the issue that we don't want unit and metric_type in groups at all?

Yes.

Agreed we should add some validation rules, until then @mtojek should I just start fixing things manually?

It's up to you. You can start fixing the system package if you know all these places. Otherwise please wait until the PR for semantic validation is merged, so it will detect all these places.

@ruflin
Copy link
Contributor Author

ruflin commented Jun 8, 2021

@fearful-symmetry What I'm trying to understand on my end is: Do we need the units? I assume we put them there for a reason?

@mtojek
Copy link
Contributor

mtojek commented Jun 8, 2021

Side note: the check I proposed in elastic/package-spec#183 forbids all unit/metric types for group fields.

Do we need the units? I assume we put them there for a reason?

I assume that was a typo, right?

@fearful-symmetry
Copy link
Contributor

@fearful-symmetry What I'm trying to understand on my end is: Do we need the units? I assume we put them there for a reason?

@ruflin most of those fields weren't added by me if I remember correctly, so I really can't say. It looks like @exekias added most of the ones to system.

@ruflin
Copy link
Contributor Author

ruflin commented Jun 8, 2021

Are we in agreement we don't need this and we should get rid of it?

@fearful-symmetry
Copy link
Contributor

Agreed. Unless there's a blocker, I'm going to wait for the validation PR before trying to clear things out.

@mtojek
Copy link
Contributor

mtojek commented Jun 9, 2021

@fearful-symmetry I will try to update it in the elastic/integrations as well.

EDIT:

PR: #1114

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants