search

elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.65k stars 8.23k forks source link

[metrics_data_access] move MetricsTable components to package #170122

Open klacabane opened 1 year ago

klacabane commented 1 year ago

Summary

https://github.com/elastic/kibana/issues/166834 follow up

In https://github.com/elastic/kibana/issues/166834 we removed the apm->infra dependency by moving the necessary components out of infra plugin to metrics_data_access. The UI components are still part of the plugin start contract but should be moved to a package that can be statically imported; the observability package is a good place to host the components. At the moment the components are only used in apm plugin's Infrastructure tab.

Acceptance criteria

elasticmachine commented 1 year ago

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

elasticmachine commented 1 year ago

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

roshan-elastic commented 3 months ago

Thanks for this, I've reviewed this but in order to prioritise, more info needed in order to understand user value for this.

Are you able to summarise what this value this brings to the user?

jasonrhodes commented 2 months ago

@roshan-elastic this is tech debt, so no value to the user, but value to reducing the underlying complexity of the app, preventing bugs, making it simpler to develop new features that touch this code, etc. I'll leave it to @klacabane to weigh in on whether this still a good ROI on the chore/tech debt front! :)

klacabane commented 2 months ago

The point of this task is to adopt more precisely the tiered plugin system. Right now the implementation is slightly off and breaks a convention, this ticket aims at implementing an ideal state (see https://github.com/elastic/kibana/issues/166834#issuecomment-1735554060). My opinion would be to park this for the moment and re-evaluate if the components in question need to be used by more consumers as it's only used by apm atm

smith commented 2 months ago

IMO we should replace this table with some kind of "related entities" component and close anything related to this table that's not an immediate security risk.

roshan-elastic commented 2 months ago

Great - thanks all.

Playing this back:

jasonrhodes commented 2 months ago

Thanks, @klacabane — so for extra clarity for everyone, this ticket isn't about any changes to the MetricsTable component itself but rather about how to organize any such components or packages that use data from the data access plugins. If this component is being "phased out" and is only used in one place, there's probably not much harm in leaving it as-is (although clearly marking it as not for future use would be nice). The most important thing is to evangelize how to best separate packages so they have as small of a chance as possible to introduce circular dependencies, which brings Great Pain to us all every day. :)