[component] Add MustNewType constructor for component.Type #9414

mx-psi · 2024-01-29T17:32:24Z

Description:

Adds component.MustNewType to create a type. This function panics if the type has invalid characters. Add similar functions component.MustNewID and component.MustNewIDWithName.
Adds component.Type.String to recover the string
Use component.MustNewType, component.MustNewID, component.MustNewIDWithName and component.Type.String everywhere in this codebase. To do this I changed component.Type into an opaque struct and checked for compile-time errors.

Some notes:

All components currently on core and contrib follow this rule. This is still breaking for other components.
A future PR will change this into a struct, to actually validate this (right now you can just do component.Type("anything") to bypass validation). I want to do this in two steps to avoid breaking contrib tests: we first introduce this function, and after that we change into a struct.

Link to tracking Issue: Updates #9208

codecov · 2024-01-29T17:37:00Z

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (3665732) 90.46% compared to head (78139c7) 90.41%.

Files	Patch %	Lines
component/identifiable.go	72.22%	4 Missing and 1 partial ⚠️
cmd/mdatagen/validate.go	0.00%	2 Missing and 1 partial ⚠️
component/config.go	86.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9414      +/-   ##
==========================================
- Coverage   90.46%   90.41%   -0.05%     
==========================================
  Files         344      344              
  Lines       18024    18058      +34     
==========================================
+ Hits        16306    16328      +22     
- Misses       1390     1399       +9     
- Partials      328      331       +3

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

mx-psi · 2024-01-29T17:38:30Z

I wonder if I should add a function to do component.NewID(component.MustType(<string literal>)) in one go, since it is a very common pattern in tests and internals.

component/config_test.go

component/config.go

evan-bradley · 2024-01-29T21:20:31Z

I wonder if I should add a function to do component.NewID(component.MustType(<string literal>)) in one go, since it is a very common pattern in tests and internals.

It sounds like we want to also validate the component name according to what you wrote in #9208, could we transition from NewID to MustNewID and validate component.MustNewID("mytype")? In my opinion we shouldn't require an extra function call to do the validation. This would also line up with component.MustNewIDWithName("mytype", "myname").

To make the change I ran: - rg 'component.NewID$component.MustNewType\(.*?$\)' -l | xargs sd 'component.NewID$component.MustNewType\((".*?")$\)' 'component.MustNewID($1)' - rg 'component.NewIDWithName$component.MustNewType\(.*?$, .*?\)' -l | xargs sd 'component.NewIDWithName$component.MustNewType\((.*?)$, (.*?)\)' 'component.MustNewIDWithName($1, $2)'

bogdandrutu

Thanks for this, some feedback:

I would prefer to also have a NewType with error since some may not feel comfortable with panic (not sure why, but good to have it I think).
Can metadatagen actually check the type and fail to generate code if the type is not ok?

component/identifiable.go

mx-psi · 2024-01-31T12:23:41Z

Thanks for this, some feedback:

I would prefer to also have a NewType with error since some may not feel comfortable with panic (not sure why, but good to have it I think).

Can metadatagen actually check the type and fail to generate code if the type is not ok?

Done :)

mx-psi · 2024-02-01T12:39:36Z

🤔 This test is failing consistently, but I am not sure how it is related to the changes on this PR

--- FAIL: TestTelemetryEnabled (0.22s)
    awsxray_test.go:87: 
        	Error Trace:	/tmp/opentelemetry-collector-contrib/exporter/awsxrayexporter/awsxray_test.go:87
        	Error:      	Not equal: 
        	            	expected: int(1)
        	            	actual  : int64(0)
        	Test:       	TestTelemetryEnabled
    awsxray_test.go:88: 
        	Error Trace:	/tmp/opentelemetry-collector-contrib/exporter/awsxrayexporter/awsxray_test.go:88
        	Error:      	Not equal: 
        	            	expected: int(1)
        	            	actual  : int64(0)
        	Test:       	TestTelemetryEnabled
    awsxray_test.go:89: 
        	Error Trace:	/tmp/opentelemetry-collector-contrib/exporter/awsxrayexporter/awsxray_test.go:89
        	Error:      	Should be true
        	Test:       	TestTelemetryEnabled
    awsxray_test.go:91: 
        	Error Trace:	/tmp/opentelemetry-collector-contrib/exporter/awsxrayexporter/awsxray_test.go:91
        	Error:      	Not equal: 
        	            	expected: int(1)
        	            	actual  : int64(0)
        	Test:       	TestTelemetryEnabled
FAIL
FAIL	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awsxrayexporter	0.810s

edit: Ah, I guess it comes from https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/2f5b0bc52cd160886b4e1ee03461704d6c440a8d/exporter/awsxrayexporter/awsxray_test.go#L73?

…Ds (#30994) **Description:** - Makes the test on awsxray exporter use the `exportertest.NewNopCreateSettings().ID`. This is because this is the ID passed to `newTracesExporter`: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/2f5b0bc52cd160886b4e1ee03461704d6c440a8d/exporter/awsxrayexporter/awsxray_test.go#L109 Currently, this makes no difference, but it will once the ID on NewNopCreateSettings is fixed. - Also fixes the hostmetrics receiver ID, just for fun :) **Link to tracking Issue:** Needed for open-telemetry/opentelemetry-collector/pull/9414

bogdandrutu

Can merge after fixing or disagreeing with my nit comment.

bogdandrutu · 2024-02-01T14:14:19Z

cmd/mdatagen/validate.go

+// can only contain ASCII alphanumeric characters and '_'.
+// This must be kept in sync with the regex in component/config.go.
+var typeRegexp = regexp.MustCompile(`^[a-zA-Z][0-9a-zA-Z_]*$`)
+
 func (md *metadata) validateType() error {


I would call NewType and ignore return value, only return error.

Agree that that would be better, but it's not easy to do: it would mean adding a replace statement on cmd/mdatagen/go.mod for component, and that makes go install fail. See e.g. open-telemetry/opentelemetry-collector-contrib/issues/27855 for a recent instance of this issue with telemetrygen

component/config.go

) **Description:** Before this change an empty name was passed to the scraperhelper. **Link to tracking Issue:** This is needed to make contrib tests pass on open-telemetry/opentelemetry-collector/pull/9414

**Description:** Same as open-telemetry#30917, before this change an empty name could be passed to the scraperhelper. I think this is the only other instance of this pattern **Link to tracking Issue**: This is needed to make contrib tests pass on open-telemetry/opentelemetry-collector/pull/9414

**Description:** Similar to open-telemetry#30917 and open-telemetry#30925, the ID was empty before. **Link to tracking Issue:** This is needed to make contrib tests pass on open-telemetry/opentelemetry-collector#9414

…Ds (open-telemetry#30994) **Description:** - Makes the test on awsxray exporter use the `exportertest.NewNopCreateSettings().ID`. This is because this is the ID passed to `newTracesExporter`: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/2f5b0bc52cd160886b4e1ee03461704d6c440a8d/exporter/awsxrayexporter/awsxray_test.go#L109 Currently, this makes no difference, but it will once the ID on NewNopCreateSettings is fixed. - Also fixes the hostmetrics receiver ID, just for fun :) **Link to tracking Issue:** Needed for open-telemetry/opentelemetry-collector/pull/9414

#31015) **Description:** Explicit uses `component.Type` when building string to compare to in test. Remove comment that will soon be outdated about the component.Type. **Link to tracking Issue:** Needed for open-telemetry/opentelemetry-collector/pull/9414

mx-psi · 2024-02-02T15:53:41Z

I have limited visibility into contrib tests because they are broken because of unrelated reasons, but I am reasonably sure that all the tests related to this have been fixed on contrib, so I will merge this by (my TZ) EOD

**Description:** Follow up to open-telemetry/opentelemetry-collector/pull/9414, adds the same changes in `mdatagen` regarding validation of `component.Type`s. On this PR the validation logic and templating is a bit more complex because of subcomponents. I will add these changes back on core once this PR is merged. **Link to tracking Issue:** open-telemetry/opentelemetry-collector/issues/9208

**Description:** Follow up to #9414 and open-telemetry/opentelemetry-collector-contrib/pull/31038. **Link to tracking Issue:** Fixes #9208.

mx-psi added 3 commits January 29, 2024 18:20

[component] Add MustType constructor for component.Type

f6ee81b

[cmd/mdatagen, *] Use component.MustType to build component.Type

8bbc8b9

Add changelog note

5c237bc

mx-psi requested review from a team, Aneurysm9 and evan-bradley and removed request for Aneurysm9 January 29, 2024 17:32

Correct changelog note

0dc8797

mx-psi commented Jan 29, 2024

View reviewed changes

component/config_test.go Show resolved Hide resolved

make gogenerate

91c9be8

mx-psi added this to the `go.opentelemetry.io/collector/component` 1.0 milestone Jan 29, 2024

TylerHelmuth reviewed Jan 29, 2024

View reviewed changes

component/config.go Show resolved Hide resolved

component/config.go Outdated Show resolved Hide resolved

bogdandrutu approved these changes Jan 29, 2024

View reviewed changes

component/config.go Outdated Show resolved Hide resolved

mx-psi added 4 commits January 30, 2024 13:16

Remove mention to '-' from documentation

796a4f6

Rename MustType to MustNewType

80e289d

Merge branch 'main' into mx-psi/component-type-validation

818c352

mx-psi changed the title ~~[component] Add MustType constructor for component.Type~~ [component] Add MustNewType constructor for component.Type Jan 30, 2024

bogdandrutu approved these changes Jan 30, 2024

View reviewed changes

evan-bradley approved these changes Jan 30, 2024

View reviewed changes

component/identifiable.go Outdated Show resolved Hide resolved

mx-psi added 3 commits January 31, 2024 13:15

Fix comment

ecf6908

Make NewType public

37eb056

Add validation also on mdatagen

03d90ac

mx-psi requested a review from TylerHelmuth January 31, 2024 12:18

Merge branch 'main' into mx-psi/component-type-validation

d663f2a

Attempt to fix contrib testS

e6bb5fb

mx-psi mentioned this pull request Feb 1, 2024

[chore][receiver/hostmetrics, exporter/awsxray] Fix some more empty IDs open-telemetry/opentelemetry-collector-contrib#30994

Merged

bogdandrutu approved these changes Feb 1, 2024

View reviewed changes

TylerHelmuth approved these changes Feb 1, 2024

View reviewed changes

component/config.go Outdated Show resolved Hide resolved

mx-psi mentioned this pull request Feb 2, 2024

[chore][exporter/splunk_hec] Explicit check for component.Type in test open-telemetry/opentelemetry-collector-contrib#31015

Merged

Move interface assertion to test file

c58f72c

TylerHelmuth approved these changes Feb 2, 2024

View reviewed changes

Merge branch 'main' into mx-psi/component-type-validation

78139c7

mx-psi merged commit 26c157e into open-telemetry:main Feb 2, 2024
35 of 46 checks passed

mx-psi deleted the mx-psi/component-type-validation branch February 2, 2024 16:33

github-actions bot modified the milestones: `go.opentelemetry.io/collector/component` 1.0, next release Feb 2, 2024

mx-psi mentioned this pull request Feb 5, 2024

[chore][mdatagen] Use MustNewType for metadata.Type open-telemetry/opentelemetry-collector-contrib#31038

Merged

mx-psi mentioned this pull request Feb 6, 2024

[component] Change component.Type underlying type to a struct #9472

Merged

mx-psi added a commit that referenced this pull request Mar 6, 2024

[component] Change component.Type underlying type to a struct (#9472)

b269362

**Description:** Follow up to #9414 and open-telemetry/opentelemetry-collector-contrib/pull/31038. **Link to tracking Issue:** Fixes #9208.

evan-bradley mentioned this pull request Jun 10, 2024

Track @evan-bradley interest and contributions for approver #10371

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[component] Add MustNewType constructor for component.Type #9414

[component] Add MustNewType constructor for component.Type #9414

mx-psi commented Jan 29, 2024 •

edited

Loading

codecov bot commented Jan 29, 2024 •

edited

Loading

mx-psi commented Jan 29, 2024

evan-bradley commented Jan 29, 2024

bogdandrutu left a comment •

edited

Loading

mx-psi commented Jan 31, 2024

mx-psi commented Feb 1, 2024 •

edited

Loading

bogdandrutu left a comment

bogdandrutu Feb 1, 2024

mx-psi Feb 2, 2024

mx-psi commented Feb 2, 2024 •

edited

Loading

[component] Add MustNewType constructor for component.Type #9414

[component] Add MustNewType constructor for component.Type #9414

Conversation

mx-psi commented Jan 29, 2024 • edited Loading

codecov bot commented Jan 29, 2024 • edited Loading

Codecov Report

mx-psi commented Jan 29, 2024

evan-bradley commented Jan 29, 2024

bogdandrutu left a comment • edited Loading

Choose a reason for hiding this comment

mx-psi commented Jan 31, 2024

mx-psi commented Feb 1, 2024 • edited Loading

bogdandrutu left a comment

Choose a reason for hiding this comment

bogdandrutu Feb 1, 2024

Choose a reason for hiding this comment

mx-psi Feb 2, 2024

Choose a reason for hiding this comment

mx-psi commented Feb 2, 2024 • edited Loading

mx-psi commented Jan 29, 2024 •

edited

Loading

codecov bot commented Jan 29, 2024 •

edited

Loading

bogdandrutu left a comment •

edited

Loading

mx-psi commented Feb 1, 2024 •

edited

Loading

mx-psi commented Feb 2, 2024 •

edited

Loading