Make MakeFactoryMap generic and move to otelcol #12220

jade-guiton-dd · 2025-01-31T13:27:16Z

Description

At the moment, receiver, scraper, processor, exporter, and extension each have their own version of the MakeFactoryMap function, which takes a list of Factories and returns it as a map, with the component name as the key (and an error if there is overlap). Because all versions are near-identical except for the Factory type used, and this function is only used in tests and in the code generated by OCB (components.go), it was suggested to make it generic and move it to the otelcol package.

This PR does exactly that. The old MakeFactoryMap functions are deprecated (and may be removed alongside their tests in a future release). I also had to make a few other odd changes to fit the new dependency graph.

Link to tracking issue

Resolves #12222

…lcol

codecov · 2025-01-31T13:38:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.45%. Comparing base (765e4a5) to head (d2a04ba).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12220      +/-   ##
==========================================
+ Coverage   91.39%   91.45%   +0.05%     
==========================================
  Files         468      469       +1     
  Lines       25591    25590       -1     
==========================================
+ Hits        23390    23404      +14     
+ Misses       1785     1775      -10     
+ Partials      416      411       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jade-guiton-dd · 2025-01-31T16:13:00Z

Hm, CI says otelcorecol is out of date, but I get on my diff locally when running make genotelcorecol

Edit: The issue was that I was using a different version of Go from the CI, so the toolchain directive generated by ocb was different

jade-guiton-dd · 2025-02-03T11:29:38Z

I removed scraper from the test because the dependency changes were causing crosslink to add replaces where they weren't actually needed, might file an issue about that later. The generic code only relies on the component.Factory interface anyway, which scraper.Factory is a subinterface of, so this is probably fine?

mx-psi · 2025-02-05T11:56:49Z

@bogdandrutu will merge this on Friday, this seems fine to me but since otelcol is far from being stabilized we can iterate on the function if we need to

@jade-guiton-dd can you solve the merge conflicts?

bogdandrutu

The pattern to avoid circular deps that I've seen is to move tests in a package "builder_test" in the same directory.

bogdandrutu · 2025-02-05T15:38:06Z

service/internal/builders/builders.go

@@ -13,7 +13,7 @@ import (

 var (
 	errNilNextConsumer = errors.New("nil next Consumer")
-	nopType            = component.MustNewType("nop")
+	NopType            = component.MustNewType("nop")


Why this change?

I moved the tests into the separate builderstest package to avoid circular dependencies, and this variable is used in the tests, so I needed to make it exported to keep it accessible. Note that all of this is still under internal, so I don't think this expands the public API.

I based the builderstest naming convention on componenttest, connectortest, consumertest, etc. Do you think I should make it a child of builders instead of a sibling?

builderstest -> is the test files, not testing helpers

mx-psi · 2025-02-10T11:24:27Z

I want us to be able to mark extension as 1.0 as soon as we mark component 1.0, so I am marking this as release blocker for the next release. I believe the changes discussed are all internal, so this is more of a reminder.

codeboten

Change looks good, just need to update the deprecation version

codeboten · 2025-02-10T18:52:14Z

connector/connector.go

@@ -388,6 +388,8 @@ func NewFactory(cfgType component.Type, createDefaultConfig component.CreateDefa

 // MakeFactoryMap takes a list of connector factories and returns a map with factory type as keys.
 // It returns a non-nil error when there are factories with duplicate type.
+//
+// Deprecated: [v0.119.0] Use otelcol.MakeFactoryMap[connector.Factory] instead


Suggested change

// Deprecated: [v0.119.0] Use otelcol.MakeFactoryMap[connector.Factory] instead

// Deprecated: [v0.120.0] Use otelcol.MakeFactoryMap[connector.Factory] instead

codeboten · 2025-02-10T18:52:29Z

exporter/exporter.go

@@ -190,6 +190,8 @@ func NewFactory(cfgType component.Type, createDefaultConfig component.CreateDefa

 // MakeFactoryMap takes a list of factories and returns a map with Factory type as keys.
 // It returns a non-nil error when there are factories with duplicate type.
+//
+// Deprecated: [v0.119.0] Use otelcol.MakeFactoryMap[exporter.Factory] instead


Suggested change

// Deprecated: [v0.119.0] Use otelcol.MakeFactoryMap[exporter.Factory] instead

// Deprecated: [v0.120.0] Use otelcol.MakeFactoryMap[exporter.Factory] instead

codeboten · 2025-02-10T18:52:35Z

extension/extension.go

@@ -82,6 +82,8 @@ func NewFactory(

 // MakeFactoryMap takes a list of factories and returns a map with Factory type as keys.
 // It returns a non-nil error when there are factories with duplicate type.
+//
+// Deprecated: [v0.119.0] Use otelcol.MakeFactoryMap[extension.Factory] instead


Suggested change

// Deprecated: [v0.119.0] Use otelcol.MakeFactoryMap[extension.Factory] instead

// Deprecated: [v0.120.0] Use otelcol.MakeFactoryMap[extension.Factory] instead

codeboten · 2025-02-10T18:52:51Z

scraper/factory.go

@@ -138,6 +138,8 @@ func NewFactory(cfgType component.Type, createDefaultConfig component.CreateDefa

 // MakeFactoryMap takes a list of receiver factories and returns a map with factory type as keys.
 // It returns a non-nil error when there are factories with duplicate type.
+//
+// Deprecated: [v0.119.0] Use otelcol.MakeFactoryMap[scraper.Factory] instead


Suggested change

// Deprecated: [v0.119.0] Use otelcol.MakeFactoryMap[scraper.Factory] instead

// Deprecated: [v0.120.0] Use otelcol.MakeFactoryMap[scraper.Factory] instead

codeboten · 2025-02-10T18:52:56Z

receiver/receiver.go

@@ -200,6 +200,8 @@ func NewFactory(cfgType component.Type, createDefaultConfig component.CreateDefa

 // MakeFactoryMap takes a list of receiver factories and returns a map with factory type as keys.
 // It returns a non-nil error when there are factories with duplicate type.
+//
+// Deprecated: [v0.119.0] Use otelcol.MakeFactoryMap[receiver.Factory] instead


Suggested change

// Deprecated: [v0.119.0] Use otelcol.MakeFactoryMap[receiver.Factory] instead

// Deprecated: [v0.120.0] Use otelcol.MakeFactoryMap[receiver.Factory] instead

codeboten · 2025-02-10T18:53:01Z

processor/processor.go

@@ -193,6 +193,8 @@ func NewFactory(cfgType component.Type, createDefaultConfig component.CreateDefa

 // MakeFactoryMap takes a list of factories and returns a map with Factory type as keys.
 // It returns a non-nil error when there are factories with duplicate type.
+//
+// Deprecated: [v0.119.0] Use otelcol.MakeFactoryMap[processor.Factory] instead


Suggested change

// Deprecated: [v0.119.0] Use otelcol.MakeFactoryMap[processor.Factory] instead

// Deprecated: [v0.120.0] Use otelcol.MakeFactoryMap[processor.Factory] instead

Deprecate type-specific MakeFactoryMap and add generic version in ote…

c4f817d

…lcol

jade-guiton-dd added 4 commits January 31, 2025 14:49

Tidy modules, fix linting, and remove dead code to satisfy codecov

a90c556

Add version numbers to deprecation notices

8d4a602

make genotelcorecol

1f02076

Add changelog

6d5d946

jade-guiton-dd added 4 commits January 31, 2025 17:45

Revert change to toolchain directive

014e5fc

make crosslink

5766e39

make gotidy

fc69871

make genotelcorecol (bruh)

75da473

This comment was marked as resolved.

Sign in to view

jade-guiton-dd added 2 commits February 3, 2025 11:49

Remove scraper-related test

7020de9

Remove scraper from builder/main_test

9f923f5

jade-guiton-dd marked this pull request as ready for review February 3, 2025 11:29

jade-guiton-dd requested a review from a team as a code owner February 3, 2025 11:29

jade-guiton-dd requested review from djaglowski and bogdandrutu February 3, 2025 11:29

mx-psi approved these changes Feb 3, 2025

View reviewed changes

Merge branch 'main' into generic-makefactorymap

f49e9d8

bogdandrutu reviewed Feb 5, 2025

View reviewed changes

mx-psi requested a review from bogdandrutu February 7, 2025 09:12

Merge branch 'main' into generic-makefactorymap

d2a04ba

mx-psi added the release:blocker The issue must be resolved before cutting the next release label Feb 10, 2025

codeboten reviewed Feb 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make MakeFactoryMap generic and move to otelcol #12220

Make MakeFactoryMap generic and move to otelcol #12220

jade-guiton-dd commented Jan 31, 2025 •

edited

Loading

codecov bot commented Jan 31, 2025 •

edited

Loading

jade-guiton-dd commented Jan 31, 2025 •

edited

Loading

This comment was marked as resolved.

jade-guiton-dd commented Feb 3, 2025

mx-psi commented Feb 5, 2025

bogdandrutu left a comment

bogdandrutu Feb 5, 2025

jade-guiton-dd Feb 6, 2025

jade-guiton-dd Feb 6, 2025

bogdandrutu Feb 10, 2025

mx-psi commented Feb 10, 2025

codeboten left a comment

codeboten Feb 10, 2025

codeboten Feb 10, 2025

codeboten Feb 10, 2025

codeboten Feb 10, 2025

codeboten Feb 10, 2025

codeboten Feb 10, 2025

	// Deprecated: [v0.119.0] Use otelcol.MakeFactoryMap[connector.Factory] instead
	// Deprecated: [v0.120.0] Use otelcol.MakeFactoryMap[connector.Factory] instead

Make MakeFactoryMap generic and move to otelcol #12220

Are you sure you want to change the base?

Make MakeFactoryMap generic and move to otelcol #12220

Conversation

jade-guiton-dd commented Jan 31, 2025 • edited Loading

Description

Link to tracking issue

codecov bot commented Jan 31, 2025 • edited Loading

Codecov Report

jade-guiton-dd commented Jan 31, 2025 • edited Loading

This comment was marked as resolved.

jade-guiton-dd commented Feb 3, 2025

mx-psi commented Feb 5, 2025

bogdandrutu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mx-psi commented Feb 10, 2025

codeboten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jade-guiton-dd commented Jan 31, 2025 •

edited

Loading

codecov bot commented Jan 31, 2025 •

edited

Loading

jade-guiton-dd commented Jan 31, 2025 •

edited

Loading