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

[extension] Add ModuleInfo to extension.Settings #10888

Merged
merged 6 commits into from
Aug 21, 2024

Conversation

dpaasman00
Copy link
Contributor

Description

Paired with @djaglowski on adding a new struct, ModuleInfo, to extension.Settings. The goal is to make the collector's component go modules available to extensions that may want to use that information (in particular the OpAMP extension).

We didn't want to modify the extension package but adding this information to the Settings struct seems to be the most painless way to make this information available. No function signatures need to be updated and existing extensions can just ignore the new field on the Settings struct unless they want the information.

Link to tracking issue

Fixes #10876

Testing

Updated existing tests.

@dpaasman00 dpaasman00 force-pushed the module-info-extensions branch from 9b2ec15 to 990621c Compare August 14, 2024 12:22
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any test cases we can add? If nothing else maybe a test that demonstrates how it would be used in extension creation?

extension/extension_test.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.62%. Comparing base (caae756) to head (6432d9e).
Report is 18 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10888      +/-   ##
==========================================
+ Coverage   91.60%   91.62%   +0.01%     
==========================================
  Files         404      404              
  Lines       18980    18990      +10     
==========================================
+ Hits        17387    17399      +12     
+ Misses       1234     1233       -1     
+ Partials      359      358       -1     

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

@dpaasman00 dpaasman00 force-pushed the module-info-extensions branch 2 times, most recently from ed1271f to 82d9fdb Compare August 15, 2024 00:36
@dpaasman00 dpaasman00 marked this pull request as ready for review August 15, 2024 00:36
@dpaasman00 dpaasman00 requested review from a team and djaglowski August 15, 2024 00:36
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'll recuse myself from approval since I contributed to it.

@djaglowski
Copy link
Member

cc: @open-telemetry/collector-maintainers

@dpaasman00 dpaasman00 force-pushed the module-info-extensions branch from 82d9fdb to 25a5bb6 Compare August 16, 2024 17:36
@@ -51,6 +51,15 @@ type ConfigWatcher interface {
NotifyConfig(ctx context.Context, conf *confmap.Conf) error
}

// ModuleInfo describes the go module for each component.
type ModuleInfo struct {
Receiver map[component.Type]string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this have more structure? For example, should we use https://pkg.go.dev/golang.org/x/[email protected]/module#Version (or something similar) instead?

I am a bit hesitant about using module.Version directly since that module has a 0.x version, but something similar may make sense

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not opposed to structuring it more but it wasn't clear to me either what that structure should be. I think it's worth noting that the module string is something of a standard in its own right, since its interpretable by go tooling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not opposed to structuring it more but it wasn't clear to me either what that structure should be

Hm, I guess a light wrapper around the module.Version I linked above makes sense.

But thinking about it more I remembered open-telemetry/opentelemetry-collector-contrib/issues/31136, where we put int as the type for a stepping number, that is commonly represented as a string, and then found out that there are edge cases where it's not representable as an int.

I guess a wrapper would make this situation manageable, but the lesson I learned there is that using a string may be better when you don't control the evolution of what you are representing.

This is all to say: if you feel like a string works, let's go with a string :)

@mx-psi mx-psi merged commit 6764622 into open-telemetry:main Aug 21, 2024
49 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make module info available to extensions
3 participants