-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
add stability level to component factory #5580
add stability level to component factory #5580
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5580 +/- ##
==========================================
+ Coverage 91.27% 91.33% +0.06%
==========================================
Files 191 191
Lines 11304 11384 +80
==========================================
+ Hits 10318 10398 +80
- Misses 785 786 +1
+ Partials 201 200 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for a case where I think we should error out
3795de6
to
dc73bfb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me. I would split the logging / zpages part.
@@ -304,6 +304,21 @@ func Build(ctx context.Context, settings component.TelemetrySettings, buildInfo | |||
return exps, nil | |||
} | |||
|
|||
func logStabilityMessage(logger *zap.Logger, sl component.StabilityLevel) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a "InfoMessage" (or find a better name) on component.StabilityLevel
(can even accept zap.Log or return the string) to help with this?
Reason: you forget to do the same in extensions
package, where you will need to duplicate this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I thought about it and ended up choosing to go this route, but you're right we'll need the same thing for extensions.
I'll follow up this PR with a fix for this that can be used in extensions
The following allows the setting of a stability level on the factory. This will let us consolidate warnings across all components to notify users of deprecated, unmaintained components.