-
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
Conversation
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
plugin/storage/grpc/factory.go
Outdated
@@ -98,7 +107,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...) | |||
noophost := NewNopHost() |
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 also have a usage of componenttest.NewNopHost
in https://github.com/jaegertracing/jaeger/blob/main/cmd/jaeger/internal/integration/span_writer.go#L55. Maybe we can move the NopHost
to a shared package rather than just having it live here so we don't have to define it every time we need it?
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.
the point was to use the real Host provided by OTEL collector. There's no reason to define our own noop host.
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.
looks like we have another noop host in jaeger as well - https://github.com/jaegertracing/jaeger/blob/main/cmd/collector/app/handler/otlp_receiver.go#L185
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.
So we need host implementation here with its related methods ?
plugin/storage/grpc/factory.go
Outdated
@@ -98,7 +107,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...) | |||
noophost := NewNopHost() |
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.
@chahatsagarmain instead of creating a nophost here, you can just pass component.Host
into NewFactoryWithConfig
from https://github.com/jaegertracing/jaeger/blob/main/cmd/jaeger/internal/extension/jaegerstorage/extension.go#L115
## Which problem is this PR solving? - Part of jaegertracing#6040 ## Description of the changes - Added the [attributesprocessor](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/attributesprocessor/README.md) to the Jaeger V2 binary to replace `--collector.tags`. See the [migration guide](https://docs.google.com/document/d/18B1yTMewRft2N0nW9K-ecVRTt5VaNgnrPTW1eL236t4/edit?usp=sharing) for more details. ## How was this change tested? - ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
@mahadzaryab1 can you check the host interface commit , I have created the host interface with extensions method . do i need to add the same for factories here ? |
plugin/storage/grpc/factory.go
Outdated
type host struct { | ||
extensions map[component.ID]component.Component | ||
} |
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.
@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 Initialize
and then into ToClientConn
. Does that make sense?
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.
Passing as a params to Initialize gives declaration error for Factory but Creating a host in NewFactoryWithConfig
wouldnt that also be a no op host?
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.
@mahadzaryab1 can the otel collector initialize a component.Host
in collectorParams
which can be used here ?
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 think you'll want to do something like this:
- Update
Factory
struct to contain the host
type Factory struct {
metricsFactory metrics.Factory
logger *zap.Logger
tracerProvider trace.TracerProvider
config Config
services *ClientPluginServices
remoteConn *grpc.ClientConn
host component.Host
}
- Update
NewFactoryWithConfig
so that it takes in the host and sets it in the factory
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
}
- Use
f.host
inInitialize
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.
+1 - don't change the interface, but ok to change constructor
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
@@ -126,7 +126,7 @@ func (s *storageExt) Start(_ context.Context, _ component.Host) error { | |||
factory, err = badger.NewFactoryWithConfig(*cfg.Badger, mf, s.telset.Logger) | |||
case cfg.GRPC != nil: | |||
//nolint: contextcheck | |||
factory, err = grpc.NewFactoryWithConfig(*cfg.GRPC, mf, s.telset.Logger) | |||
factory, err = grpc.NewFactoryWithConfig(*cfg.GRPC, mf, s.telset.Logger, grpc.NewHost()) |
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.
you don't need to create a new host, its passed into this method. Just change the _
in the function signature to be host
and pass that in here.
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
plugin/storage/grpc/factory.go
Outdated
host component.Host | ||
} | ||
|
||
type host struct { |
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.
Signed-off-by: chahatsagarmain <chahatsagar2003@gmail.com>
please ensure all commits are signed (DCO check is failing) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6058 +/- ##
==========================================
+ Coverage 96.90% 96.92% +0.01%
==========================================
Files 349 351 +2
Lines 16588 16676 +88
==========================================
+ Hits 16075 16163 +88
Misses 329 329
Partials 184 184
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
replaced by #6085 |
Which problem is this PR solving?
Description of the changes
How was this change tested?