-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Using component for no op host instead of component test #6058
Changes from 7 commits
1220b5d
9ce7751
f0edd49
975f8fb
c026ceb
5532991
d29aab9
33a369e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,6 @@ import ( | |
|
||
"github.com/spf13/viper" | ||
"go.opentelemetry.io/collector/component" | ||
"go.opentelemetry.io/collector/component/componenttest" | ||
"go.opentelemetry.io/collector/config/configgrpc" | ||
"go.opentelemetry.io/collector/config/configtelemetry" | ||
"go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" | ||
|
@@ -47,6 +46,25 @@ type Factory struct { | |
config Config | ||
services *ClientPluginServices | ||
remoteConn *grpc.ClientConn | ||
host component.Host | ||
} | ||
|
||
type host struct { | ||
extensions map[component.ID]component.Component | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chahatsagarmain so we actually don't need to define a host at all. if you take a look at this method, you'll see that the host is available to us already. What you need to do is pass the host down from the method I linked above into this function and you can then pass it into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Passing as a params to Initialize gives declaration error for Factory but Creating a host in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mahadzaryab1 can the otel collector initialize a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you'll want to do something like this:
type Factory struct {
metricsFactory metrics.Factory
logger *zap.Logger
tracerProvider trace.TracerProvider
config Config
services *ClientPluginServices
remoteConn *grpc.ClientConn
host component.Host
}
func NewFactoryWithConfig(
cfg Config,
metricsFactory metrics.Factory,
logger *zap.Logger,
host component.Host,
) (*Factory, error) {
f := NewFactory()
f.config = cfg
f.host = host
if err := f.Initialize(metricsFactory, logger); err != nil {
return nil, err
}
return f, nil
}
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 - don't change the interface, but ok to change constructor |
||
|
||
func NewHost() component.Host { | ||
return &host{ | ||
extensions: make(map[component.ID]component.Component), | ||
} | ||
} | ||
|
||
func (h *host) GetExtensions() map[component.ID]component.Component { | ||
return h.extensions | ||
} | ||
|
||
func (h *host) AddExtensions(id component.ID, ext component.Component) { | ||
h.extensions[id] = ext | ||
} | ||
|
||
// NewFactory creates a new Factory. | ||
|
@@ -59,9 +77,11 @@ func NewFactoryWithConfig( | |
cfg Config, | ||
metricsFactory metrics.Factory, | ||
logger *zap.Logger, | ||
host component.Host, | ||
) (*Factory, error) { | ||
f := NewFactory() | ||
f.config = cfg | ||
f.host = host | ||
if err := f.Initialize(metricsFactory, logger); err != nil { | ||
return nil, err | ||
} | ||
|
@@ -98,7 +118,8 @@ func (f *Factory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger) | |
for _, opt := range opts { | ||
clientOpts = append(clientOpts, configgrpc.WithGrpcDialOption(opt)) | ||
} | ||
return f.config.ToClientConn(context.Background(), componenttest.NewNopHost(), telset, clientOpts...) | ||
|
||
return f.config.ToClientConn(context.Background(), f.host, telset, clientOpts...) | ||
} | ||
|
||
var err error | ||
|
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.
We don't need this type.