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

[configrpc] Consider adding option wrapper on ToServer and ToClientConn #9480

Closed
mx-psi opened this issue Feb 6, 2024 · 12 comments · Fixed by #11069 or #11359
Closed

[configrpc] Consider adding option wrapper on ToServer and ToClientConn #9480

mx-psi opened this issue Feb 6, 2024 · 12 comments · Fixed by #11069 or #11359
Assignees
Labels
area:config good first issue Good for newcomers release:required-for-ga Must be resolved before GA release

Comments

@mx-psi
Copy link
Member

mx-psi commented Feb 6, 2024

Instead of passing grpc.ServerOptions to each, we could pass new ToServerOptions or ToClientOptions and require wrapping grpc.ServerOptions into these, to make the interface more flexible and allow for other kinds of options in the future.

@TylerHelmuth
Copy link
Member

@mx-psi can you elaborate a little more on what you'd like to see?

For our ServerConfig.ToServer for example, we end up calling grpc.NewServer and passing it a []grpc.ServerOption. But grpc.ServerOption is an interface that we can't implement in configgrpc. We need to end up with grpc.ServerOption to call grpc.NewServer, so would we be creating a wrapper of every Option function in grpc? Something like:

type ToServerOption func() grpc.ServerOption

func MaxConcurrentStreams(n uint32) ToServerOption {
	return func() grpc.ServerOption {
		return grpc.MaxConcurrentStreams(n)
	}
}

Since grpc.ServerOption isn't extendable I don't see how we make our own package more extendable.

I believe grpc.DialOption has the same limitation.

@TylerHelmuth
Copy link
Member

Are you thinking we add something like confighttp has and then add wrappers for each grpc.ServerOption?

@mx-psi
Copy link
Member Author

mx-psi commented Feb 29, 2024

Are you thinking we add something like confighttp has and then add wrappers for each grpc.ServerOption?

Yes, that's what I was thinking:

type ToServerOption ... // opaque type here, likely a sealed interface

func WithgRPCServerOption(opt grpc.ServerOption) ToServerOption {
	return // wrap the option here into a `ToServerOption`
}

// Maybe add other options in the future that are not grpc.ServerOption

@TylerHelmuth
Copy link
Member

@mx-psi i am worried that this solution will mean we have to remember to update our options whenever a new grpc option comes out. If we are ever behind it will mean a grpc option exists that isn't usable with configgrpc. Is that ok?

@mx-psi
Copy link
Member Author

mx-psi commented Feb 29, 2024

@TylerHelmuth We would just have just a single wrapper WithGRPCServerOption(opt grpc.ServerOption) ToServerOption, so there's no need to keep up to date, right?

@TylerHelmuth
Copy link
Member

@mx-psi if we can get away with just the 1 generic wrapper that's great. Based on my brief experimentation the lack of exported fields in grpc and the need to end up with []grpc.ServerOption before calling grpc.NewServer will pose a challenge I think, but worth looking into more.

@TylerHelmuth
Copy link
Member

@mx-psi we could do something like this:

// toServerOptions has options that change the behavior of the gRPC server
// returned by ServerConfig.ToServer().
type toServerOptions struct {
	grpcServerOptions []grpc.ServerOption
}

// ToServerOption is an option to change the behavior of the gRPC server
// returned by ServerConfig.ToServer().
type ToServerOption func(opts *toServerOptions)

func WithgRPCServerOption(opt grpc.ServerOption) ToServerOption {
	return func(opts *toServerOptions) {
		opts.grpcServerOptions = append(opts.grpcServerOptions, opt)
	}
}

// ToServer returns a grpc.Server for the configuration
func (gss *ServerConfig) ToServer(_ context.Context, host component.Host, settings component.TelemetrySettings, extraOpts ...ToServerOption) (*grpc.Server, error) {
        // this is an existing function that makes a bunch of grpc.ServerOptions based on the host/settings.
	opts, err := gss.toServerOption(host, settings)
	if err != nil {
		return nil, err
	}
	tso := toServerOptions{}
	for _, otp := range extraOpts {
		otp(&tso)
	}
	opts = append(opts, tso.grpcServerOptions...)
	return grpc.NewServer(opts...), nil
}

Is this what you had in mind?

@TylerHelmuth
Copy link
Member

This pattern conflicts with #9479 tho, but it could be made to fit the interface pattern instead.

@mx-psi
Copy link
Member Author

mx-psi commented Mar 21, 2024

@TylerHelmuth Yeah, that's what I had in mind! Sorry that I wasn't very clear in the previous messages, my bad 😅

@mx-psi mx-psi moved this to Todo in Collector: v1 Apr 18, 2024
@mx-psi mx-psi added the good first issue Good for newcomers label Jul 17, 2024
@atoulme
Copy link
Contributor

atoulme commented Jul 21, 2024

@legendary-acp go for it

@jade-guiton-dd
Copy link
Contributor

I'll take this one.

@mx-psi mx-psi moved this from Todo to In Progress in Collector: v1 Sep 5, 2024
@mx-psi mx-psi moved this from In Progress to Waiting for reviews in Collector: v1 Sep 6, 2024
@github-project-automation github-project-automation bot moved this from Waiting for reviews to Done in Collector: v1 Sep 19, 2024
@mx-psi mx-psi reopened this Sep 20, 2024
@github-project-automation github-project-automation bot moved this from Done to Blocked in Collector: v1 Sep 20, 2024
@mx-psi
Copy link
Member Author

mx-psi commented Sep 20, 2024

This still needs removing and renaming the deprecated APIs, reopening

bogdandrutu pushed a commit that referenced this issue Sep 27, 2024
#11271)

#### Description

`ClientConfig.ToClientConn` and `ServerConfig.ToServer` were deprecated
in v0.110.0 in favor of `ClientConfig.ToClientConnWithOptions` and
`ServerConfig.ToServerWithOptions` which use a more flexible option type
(#11069). The original functions are now removed, and the new ones are
renamed to the old names. The `WithOptions` names are kept as deprecated
aliases for now.

Note: The "4 lines missing coverage" are from the `WithOptions` function
aliases.

#### Link to tracking issue
Updates #9480

#### Next steps

- `collector-contrib` will need to be updated to rename uses of the
`WithOptions` functions
- The newly deprecated aliases will then need to be removed in the next
release
@mx-psi mx-psi closed this as completed in 0e9f548 Oct 4, 2024
@github-project-automation github-project-automation bot moved this from Blocked to Done in Collector: v1 Oct 4, 2024
jackgopack4 pushed a commit to jackgopack4/opentelemetry-collector that referenced this issue Oct 8, 2024
open-telemetry#11271)

#### Description

`ClientConfig.ToClientConn` and `ServerConfig.ToServer` were deprecated
in v0.110.0 in favor of `ClientConfig.ToClientConnWithOptions` and
`ServerConfig.ToServerWithOptions` which use a more flexible option type
(open-telemetry#11069). The original functions are now removed, and the new ones are
renamed to the old names. The `WithOptions` names are kept as deprecated
aliases for now.

Note: The "4 lines missing coverage" are from the `WithOptions` function
aliases.

#### Link to tracking issue
Updates open-telemetry#9480

#### Next steps

- `collector-contrib` will need to be updated to rename uses of the
`WithOptions` functions
- The newly deprecated aliases will then need to be removed in the next
release
jackgopack4 pushed a commit to jackgopack4/opentelemetry-collector that referenced this issue Oct 8, 2024
#### Description

This PR removes
`ClientConfig.ToClientConnWithOptions`/`ServerConfig.ToServerWithOptions`,
deprecated aliases of
`ClientConfig.ToClientConn`/`ServerConfig.ToServer`, which were
introduced in v0.111.0. This was part of the three step process to
change the option type used by these methods (introduce new methods
using the new option type and deprecate the old ones / remove old
methods, rename new methods to the old names, add deprecated aliases for
the new names / remove aliases).

#### Link to tracking issue

Fixes open-telemetry#9480
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment