[AGTHEAL-115] refactor(core): migrate comp/core/flare to V2 component architecture - #49769
[AGTHEAL-115] refactor(core): migrate comp/core/flare to V2 component architecture#49769louis-cqrl wants to merge 1 commit into
Conversation
442b4ed to
c033c2d
Compare
| fx.Supply(flareParams), | ||
| flarfx.Module(), |
There was a problem hiding this comment.
Do not use Supply please ! It divide the init for params into two calls that can be widely apart in different helpers making troubleshooting the init very complicated. The idea of taking the params in the module init is intentional.
| fillerModule := fxutil.Component(fillers...) | ||
| // testDeps mirrors Requires but embeds fx.In so fxutil.Test can populate it field-by-field. | ||
| type testDeps struct { | ||
| fx.In |
There was a problem hiding this comment.
Let's not use FX in the tests. FX should remain contained to the fx* folders.
8892dbb to
0a73cc0
Compare
| @@ -15,12 +15,16 @@ import ( | |||
| "go.uber.org/fx" | |||
There was a problem hiding this comment.
mock/ must not import go.uber.org/fx. From the component docs: fx/ must be the only folder importing and referencing Fx. MockModule() and MockProvides{fx.Out ...} belong in fx/, not in mock/. The mock/ folder should only implement flare.Component and expose a plain New(t *testing.T) Provides constructor.
There was a problem hiding this comment.
Fixed — removed go.uber.org/fx, fxutil, MockModule(), and MockProvides{fx.Out} from mock/mock.go. The mock package now has zero fx dependency.
| } | ||
|
|
||
| // MockModule defines the fx options for the mock component. | ||
| func MockModule() fxutil.Module { |
There was a problem hiding this comment.
MockModule() belongs in fx/ (or a fx-mock/ sub-package), not in mock/. The mock/ package should have zero fx dependency.
Also: NewMock() has no *testing.T parameter. The component docs require the standalone constructor to be:
func New(t *testing.T) Provides {
return Provides{Comp: &MockFlare{}}
}This lets impl/ tests call flaremock.New(t) directly without going through fx.
There was a problem hiding this comment.
Fixed — MockModule() removed entirely (no callers). NewMock() renamed to New(t \*testing.T) Provides with a plain Provides struct (no fx.Out). The single caller in agentprofiling_test.go updated to flaremock.New(t).Comp.
|
|
||
| api "github.com/DataDog/datadog-agent/comp/api/api/def" | ||
| apiutils "github.com/DataDog/datadog-agent/comp/api/api/utils" | ||
| "github.com/DataDog/datadog-agent/comp/core/config" |
There was a problem hiding this comment.
This imports the root comp/core/config package instead of comp/core/config/def.
Per the component import graph rules: impl/ must depend on other components only through their def/ package — never the root package or impl/. Change to:
config "github.com/DataDog/datadog-agent/comp/core/config/def"There was a problem hiding this comment.
Acknowledged — comp/core/config/def doesn't exist on main yet (it's in a separate pending PR #49787). Added a TODO comment in impl.go to migrate to config/def once that PR is merged. Will update this import then.
Split comp/core/flare into def/impl/mock/fx sub-packages following the V2 component pattern, removing fx.In/fx.Out from impl and exporting Requires/Provides structs. Update all callers to import from the new sub-packages.
| log: deps.Log, | ||
| config: deps.Config, | ||
| params: deps.Params, | ||
| providers: fxutil.GetAndFilterGroup(deps.Providers), |
There was a problem hiding this comment.
Why replace the function by an internal helper ?
|
|
||
| // Module returns an fxutil.Module that registers the extra flare providers | ||
| // (providers not owned by a specific component) into the flare group. | ||
| func Module() fxutil.Module { | ||
| return fxutil.Component( | ||
| fx.Provide( | ||
| fx.Annotate( | ||
| func(wm option.Option[workloadmeta.Component], ipcComp ipc.Component) []*flaretypes.FlareFiller { | ||
| return ExtraFlareProviders(wm, ipcComp) | ||
| }, | ||
| fx.ResultTags(`group:"flare,flatten"`), | ||
| ), | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Using FX within pkg should be avoided. We want to move toward a state where pkg is a bunch of stateless helpers.
Why can't we move this within the flare comp ? This is the legacy part of the flare that will, maybe one day, be migrated by each teams in within their comp.
nathan-b
left a comment
There was a problem hiding this comment.
ARUN files look clean. I like that you pulled the tests along on this one.
|
This pull request has been automatically marked as stale because it has not had activity in the past 15 days. It will be closed in 30 days if no further activity occurs. If this pull request is still relevant, adding a comment or pushing new commits will keep it open. Also, you can always reopen the pull request if you missed the window. Thank you for your contributions! |
1 similar comment
|
This pull request has been automatically marked as stale because it has not had activity in the past 15 days. It will be closed in 30 days if no further activity occurs. If this pull request is still relevant, adding a comment or pushing new commits will keep it open. Also, you can always reopen the pull request if you missed the window. Thank you for your contributions! |
What does this PR do?
Fully migrates `comp/core/flare` from the V1 component architecture to V2:
Motivation
Align `comp/core/flare` with the V2 component architecture used across the codebase. This is a complete migration — no V1 relics remain.
Describe how you validated your changes
Additional Notes
`FlareSource` was moved from `comp/core/flare/helpers` to `comp/core/flare/types` as part of this migration — `types` is the correct home for types that should be usable without pulling in implementation dependencies.