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 <-> infra circular dependencies #126578

Open
afharo opened this issue Mar 1, 2022 · 12 comments
Open

fleet <-> infra circular dependencies #126578

afharo opened this issue Mar 1, 2022 · 12 comments
Labels
bug Fixes for quality problems that affect the customer experience NeededFor:Core Team:Fleet Team label for Observability Data Collection Fleet team technical debt Improvement of the software architecture and operational architecture

Comments

@afharo
Copy link
Member

afharo commented Mar 1, 2022

We recently identified that circular dependencies in requiredBundles might affect Kibana's UI performance. Looking for any existing circular dependencies in Kibana #124860, we noticed that fleet requires on infra via the requiredBundles, while infra optionally depends osquery, which depends on fleet. Both via the optionalPlugins.

In the Core team we created #124923 to identify these use cases early in the development phase, so this doesn't occur again. However, in order to fail Kibana, we depend on this circular dependency to be fixed.

@afharo afharo added bug Fixes for quality problems that affect the customer experience Team:Fleet Team label for Observability Data Collection Fleet team NeededFor:Core labels Mar 1, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@joshdover
Copy link
Contributor

@afharo is there a transient dependency between infra -> fleet in requiredPlugins? I don't see a direct dependency in infra's kibana.json file today:

"requiredPlugins": [
"features",
"usageCollection",
"spaces",
"embeddable",
"data",
"dataEnhanced",
"visTypeTimeseries",
"alerting",
"triggersActionsUi",
"observability",
"ruleRegistry"
],

@afharo
Copy link
Member Author

afharo commented Mar 1, 2022

Oh! Sorry! Forgot to mention infra depends on osquery and osquery depends on fleet. Both dependencies are set via optionalPlugins. I'll update the description accordingly

@joshdover
Copy link
Contributor

Thanks for clarifying.

  • fleet -> infra
    • This is probably best candidate to change. We would need to extract the Logs component that we depend on from the infra plugin into a separate plugin or package.
  • infra -> osquery
    • Don't know much about this dependency
  • osquery -> fleet
    • This one makes sense and we shouldn't try to change it

@hop-dev
Copy link
Contributor

hop-dev commented Mar 1, 2022

We've looked at doing this before but never got to it:

#112402

@jen-huang jen-huang added the technical debt Improvement of the software architecture and operational architecture label Mar 1, 2022
@joshdover
Copy link
Contributor

Ah that's right. Looking at this again, I do think it would make more sense to pursue inverting the dependency between Fleet and Infra or try to hoist the LogStream and related utilities to higher-level dependency.

It could be more changes but it feels like the better long-term option to me. @jasonrhodes any idea how feasible it would be to extract the Logs UI bits out into it's own plugin or package?

@joshdover
Copy link
Contributor

@afharo Could you give us feedback on what this is blocking so we can prioritize it appropriately?

@miltonhultgren
Copy link
Contributor

@elastic/infra-monitoring-ui

@afharo
Copy link
Member Author

afharo commented Mar 15, 2022

@afharo Could you give us feedback on what this is blocking so we can prioritize it appropriately?

We'd like to implement #124923. It's a check to let developers know when circular dependencies are introduced. If we enable the check, we'll break CI.

The reason we decided to implement it is that we noticed other teams started off trying that approach (#123720). Thankfully, it already caused some unexpected issues on CI that didn't allow them to merge their PR 😇

@jasonrhodes
Copy link
Member

Sorry for the response delay here. We've tried to figure something like this out for quite a long time, the conversation which can be tracked here #97108

@afgomez and I have been working on it the most, and have decided to wait until core dependencies move into packages so that we can move things like the Log Stream component into a package itself.

That said, if it's truly blocking things in Kibana, we can try to think through new ways to fix this once again. @afharo I assume this is still a problem you'd like to see solved ASAP?

@afharo
Copy link
Member Author

afharo commented Apr 20, 2022

@jasonrhodes we lowered the priority of our issue during our last grooming. However, it may be a potential issue with @elastic/kibana-operations' attempt to make every plugin a package.

I'll let them fill in re prioritization.

@jasonrhodes
Copy link
Member

Well, our situation is blocked on the ability to make every plugin a package, so you might say we have a ... circular dependency here. We'd gladly work with operations on getting around that hurdle so that we can package-ify most of our code, which I think seems to be the only way around a lot of this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience NeededFor:Core Team:Fleet Team label for Observability Data Collection Fleet team technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

No branches or pull requests

7 participants