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

[Infra UI] Refactor Inventory Models #46403

Merged
merged 17 commits into from
Oct 21, 2019

Conversation

simianhacker
Copy link
Member

@simianhacker simianhacker commented Sep 23, 2019

Summary

This PR refactors ALL the different models into one structure instead of everything being spread all over the project or hard coded in components. The goal is to put all these definitions in one common place and then re-use them on the client and server. These models define the following for each node type (view type):

  • Metric detail layout
  • Snapshot metrics for the inventory page
  • TSVB Metrics for the metric detail page
  • Toolbar for the inventory page (coming in follow up PR)

This should allow us to add new inventory views by just defining the file structures under inventory_models. The only time we would need to touch the react code is if there is a new section required like the APM section on the metric detail page OR a new toolbar section for the inventory page (required for AWS).

Additionally, this PR also consolidates all the different types (from GraphQL and pure Typescript files) scattered about into one place near the models. All the types are written in IO-TS. This will help with the transition from GraphQL.

Anatomy of an inventory model

<type> # Top level of the inventory type, examples include: host, container, pod, aws_ec2, etc
├── index.ts # Exports layout, metrics and toolbar
├── layout.ts # The layout for the metrics detail page
├── metrics # All the metric models for the inventory and detail page
│   ├── index.ts # exports all the metrics (snapshot & tsvb)
│   ├── snapshot/ # The models/aggregations for the inventory page
│   └── tsvb/ # The TSVB models for the metric detail page
└── toolbar.ts # The toolbar definition for the inventory page (to be added in follow up PR

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jasonrhodes
Copy link
Member

@simianhacker help me out, what's "DSL" stand for here? I only know this as "domain-specific language"…

@simianhacker
Copy link
Member Author

@jasonrhodes That's correct sort of like the ES Query DSL but really more accurate would just be Inventory Models.

@simianhacker simianhacker changed the title [Infra UI] Refactor Inventory DSL [Infra UI] Refactor Inventory Models Sep 26, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@simianhacker simianhacker self-assigned this Oct 15, 2019
@simianhacker simianhacker added Feature:Metrics UI Metrics UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.6.0 v8.0.0 labels Oct 15, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-logs-ui (Team:infra-logs-ui)

@simianhacker simianhacker marked this pull request as ready for review October 15, 2019 22:34
@simianhacker simianhacker requested a review from a team as a code owner October 15, 2019 22:34
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@afgomez
Copy link
Contributor

afgomez commented Oct 21, 2019

I did a first pass and for a person not familiar with the code I could get the gist of it, so 👍.

I don't feel comfortable to approve it yet though. Maybe you could walk me through the code on a call (or wait for someone who is more familiar with it).

Copy link
Contributor

@phillipb phillipb left a comment

Choose a reason for hiding this comment

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

I think my main takeaway is that all the layout's code should just be react components. I think doing that will help us with maintenance in the long run and make it easier for people to grok.

It's a large refractor and hard for me to understand completely how all of the pieces tie together, so I think it'd be good to add some docs. Mainly, I'd like to know what steps need to be taken to add a new type of inventory? What files would need to be created and where?


export const containerLayoutCreator: InfraMetricLayoutCreator = theme => [
export const layout: InventoryDetailLayoutCreator = theme => [
Copy link
Contributor

Choose a reason for hiding this comment

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

After thinking about this, I still think layouts are best expressed on the frontend as react components. Mainly, I think it gives us more flexibility, reduces complexity, and will gives us a better developer experience.

@@ -22,17 +25,17 @@ export const containerDiskIOOps: InfraMetricModelCreator = (timeField, indexPatt
{
field: 'docker.diskio.read.ops',
id: 'max-diskio-read-ops',
type: InfraMetricModelMetricType.max,
type: 'max',
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why remove enums?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those enums are managed by GraphQL, as part of cleaning up the tech debt I'm making an effort to move everything to IO-TS types which doesn't support enums, but each of those types have been replaced with a t.keyOf({ ... }) which restricts which string it can be.

@simianhacker
Copy link
Member Author

@phillipb Here is the issue for converting the layout JSON to a React component. #48808. I will work on adding a README.md file to the top of the inventory_models directory to explain the steps for adding additional sections.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@simianhacker
Copy link
Member Author

After discussing this with @phillipb, we've decided to defer writing the documentation till after we've gotten everything working. Here is the issue for adding documentation: #48823

@simianhacker simianhacker merged commit f916d92 into elastic:master Oct 21, 2019
simianhacker added a commit to simianhacker/kibana that referenced this pull request Nov 18, 2019
* [Infra UI] Refactor Inventory DSL

* Updating types

* Adding types

* moving from metric_models to inventory_models

* Fixing Kubernetes toolbars

* Add models to index file

* Removing snapshot metric aggregations in favor of inventory models

* Removing TSVB models and replacing with inventory models

* Fixing the glorious types

* Removing obsolete layouts

* removing obsolete message

* removing toolbar
simianhacker added a commit that referenced this pull request Nov 18, 2019
* [Infra UI] Refactor Inventory Models (#46403)

* [Infra UI] Refactor Inventory DSL

* Updating types

* Adding types

* moving from metric_models to inventory_models

* Fixing Kubernetes toolbars

* Add models to index file

* Removing snapshot metric aggregations in favor of inventory models

* Removing TSVB models and replacing with inventory models

* Fixing the glorious types

* Removing obsolete layouts

* removing obsolete message

* removing toolbar

* Removing obsolete translations

* Removing obsolete translations
@simianhacker simianhacker deleted the inventory-models branch April 17, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Metrics UI Metrics UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants