- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.8k
otelcol: remove default telemetry factory #14062
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
base: main
Are you sure you want to change the base?
Conversation
| Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@            Coverage Diff             @@
##             main   #14062      +/-   ##
==========================================
+ Coverage   92.45%   92.47%   +0.01%     
==========================================
  Files         660      660              
  Lines       36218    36218              
==========================================
+ Hits        33486    33492       +6     
+ Misses       1911     1907       -4     
+ Partials      821      819       -2     ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
| Just waiting for open-telemetry/opentelemetry-collector-contrib#43718 to be merged now | 
5747c70    to
    1f6461c      
    Compare
  
    Remove the default telemetry factory, and require callers to specify one, to keep things maintainable.
1f6461c    to
    d46adfd      
    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.
LGTM. Do we want to wait until the previous change has been released in a tagged version before merging this?
| if factories.Telemetry == nil { | ||
| factories.Telemetry = otelconftelemetry.NewFactory() | ||
| factories.Telemetry = nopTelemetryFactory() | ||
| } | 
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.
Why not really ask users to provide even the no-op?
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.
i'm ok with setting this as a convenience in otelcoltest
| if factories.Telemetry == nil { | ||
| factories.Telemetry = otelconftelemetry.NewFactory() | ||
| factories.Telemetry = nopTelemetryFactory() | ||
| } | 
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.
i'm ok with setting this as a convenience in otelcoltest
| Waiting for the following release is fine by me. | 
Description
Remove the default telemetry factory, and require callers to specify one, to keep things maintainable.
Link to tracking issue
Fixes #14003
Testing
Documentation
N/A