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

Create new grpc storage configuration to align with OTEL #5331

Merged
merged 30 commits into from
May 20, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
4da9754
add more grpc config options
akagami-harsh Apr 5, 2024
7371b30
Merge branch 'main' into grpc-config
akagami-harsh Apr 5, 2024
5c07802
Merge branch 'main' into grpc-config
akagami-harsh Apr 6, 2024
3986d8a
reuse otel's client config
akagami-harsh Apr 7, 2024
6cfb2dd
fix
akagami-harsh Apr 7, 2024
45467be
fix
akagami-harsh Apr 7, 2024
940f02c
log not supported when using auth
akagami-harsh Apr 8, 2024
9edf36a
fix
akagami-harsh Apr 8, 2024
3695b30
Merge branch 'main' into grpc-config
akagami-harsh Apr 8, 2024
b4ba240
fix
akagami-harsh Apr 8, 2024
e4dbf8e
Merge branch 'main' into grpc-config
akagami-harsh Apr 15, 2024
03b0f4e
Merge branch 'main' into grpc-config
akagami-harsh May 9, 2024
947501e
add config V2
akagami-harsh May 10, 2024
5eb9218
Merge branch 'main' into grpc-config
akagami-harsh May 12, 2024
e476ece
fix
akagami-harsh May 12, 2024
54736cc
fix
akagami-harsh May 13, 2024
e14a3f2
fix
akagami-harsh May 13, 2024
15be346
fix
akagami-harsh May 16, 2024
71baf51
fix
akagami-harsh May 16, 2024
2139a46
Merge branch 'main' into grpc-config
akagami-harsh May 16, 2024
c4a34b0
add ToOtelClientConfig()
akagami-harsh May 19, 2024
ba44b7f
fix
akagami-harsh May 19, 2024
a2143b3
add remote conn in *ClientPluginServices
akagami-harsh May 19, 2024
148000c
refactor
yurishkuro May 20, 2024
14f9c94
finish-tests
yurishkuro May 20, 2024
ff9ea18
cleanup
yurishkuro May 20, 2024
dd8701e
simplify
yurishkuro May 20, 2024
cd77995
add tests
akagami-harsh May 20, 2024
7c6e513
fix
akagami-harsh May 20, 2024
f3c2ccf
Merge branch 'main' into grpc-config
akagami-harsh May 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/jaeger/internal/extension/jaegerstorage/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (
type Config struct {
Memory map[string]memoryCfg.Configuration `mapstructure:"memory"`
Badger map[string]badgerCfg.NamespaceConfig `mapstructure:"badger"`
GRPC map[string]grpcCfg.Configuration `mapstructure:"grpc"`
GRPC map[string]grpcCfg.ConfigV2 `mapstructure:"grpc"`
Opensearch map[string]esCfg.Configuration `mapstructure:"opensearch"`
Elasticsearch map[string]esCfg.Configuration `mapstructure:"elasticsearch"`
Cassandra map[string]cassandra.Options `mapstructure:"cassandra"`
Expand Down
2 changes: 1 addition & 1 deletion cmd/jaeger/internal/extension/jaegerstorage/extension.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@
cfg: s.config.Badger,
builder: badger.NewFactoryWithConfig,
}
grpcStarter := &starter[grpcCfg.Configuration, *grpc.Factory]{
grpcStarter := &starter[grpcCfg.ConfigV2, *grpc.Factory]{

Check warning on line 120 in cmd/jaeger/internal/extension/jaegerstorage/extension.go

View check run for this annotation

Codecov / codecov/patch

cmd/jaeger/internal/extension/jaegerstorage/extension.go#L120

Added line #L120 was not covered by tests
ext: s,
storageKind: "grpc",
cfg: s.config.GRPC,
Expand Down
1 change: 1 addition & 0 deletions cmd/remote-storage/app/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"go.uber.org/zap"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials"
_ "google.golang.org/grpc/encoding"
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
"google.golang.org/grpc/reflection"

"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
Expand Down
47 changes: 40 additions & 7 deletions plugin/storage/grpc/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,15 @@
package config

import (
"context"
"errors"
"fmt"
"time"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/component/componenttest"
"go.opentelemetry.io/collector/config/configgrpc"
"go.opentelemetry.io/collector/exporter/exporterhelper"
"go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"
Expand All @@ -41,6 +46,30 @@
remoteConn *grpc.ClientConn
}

type ConfigV2 struct {
Configuration `mapstructure:",squash"`
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved

configgrpc.ClientConfig `mapstructure:",squash"`
exporterhelper.TimeoutSettings `mapstructure:",squash"`
}

func (c *ConfigV2) translateToConfigV2(v1 Configuration) *ConfigV2 {
c.Endpoint = v1.RemoteServerAddr
c.Timeout = v1.RemoteConnectTimeout

c.ClientConfig.TLSSetting.ServerName = v1.RemoteTLS.ServerName
c.ClientConfig.TLSSetting.InsecureSkipVerify = v1.RemoteTLS.SkipHostVerify
c.ClientConfig.TLSSetting.CAFile = v1.RemoteTLS.CAPath
c.ClientConfig.TLSSetting.CertFile = v1.RemoteTLS.CertPath
c.ClientConfig.TLSSetting.KeyFile = v1.RemoteTLS.KeyPath
c.ClientConfig.TLSSetting.CipherSuites = v1.RemoteTLS.CipherSuites
c.ClientConfig.TLSSetting.MinVersion = v1.RemoteTLS.MinVersion
c.ClientConfig.TLSSetting.MaxVersion = v1.RemoteTLS.MaxVersion
c.ClientConfig.TLSSetting.ReloadInterval = v1.RemoteTLS.ReloadInterval

return c
}

// ClientPluginServices defines services plugin can expose and its capabilities
type ClientPluginServices struct {
shared.PluginServices
Expand All @@ -55,17 +84,21 @@
}

// Build instantiates a PluginServices
func (c *Configuration) Build(logger *zap.Logger, tracerProvider trace.TracerProvider) (*ClientPluginServices, error) {
return c.buildRemote(logger, tracerProvider, grpc.NewClient)
func (c *ConfigV2) Build(logger *zap.Logger, tracerProvider trace.TracerProvider) (*ClientPluginServices, error) {
akagami-harsh marked this conversation as resolved.
Show resolved Hide resolved
return c.buildRemote(logger, tracerProvider)
}

type newClientFn func(target string, opts ...grpc.DialOption) (conn *grpc.ClientConn, err error)

func (c *Configuration) buildRemote(logger *zap.Logger, tracerProvider trace.TracerProvider, newClient newClientFn) (*ClientPluginServices, error) {
func (c *ConfigV2) buildRemote(logger *zap.Logger, tracerProvider trace.TracerProvider) (*ClientPluginServices, error) {
c = c.translateToConfigV2(c.Configuration)
opts := []grpc.DialOption{
grpc.WithStatsHandler(otelgrpc.NewClientHandler(otelgrpc.WithTracerProvider(tracerProvider))),
grpc.WithBlock(),
}

if c.Auth != nil {
return nil, fmt.Errorf("authenticator is not supported")

Check warning on line 99 in plugin/storage/grpc/config/config.go

View check run for this annotation

Codecov / codecov/patch

plugin/storage/grpc/config/config.go#L99

Added line #L99 was not covered by tests
}

if c.RemoteTLS.Enabled {
tlsCfg, err := c.RemoteTLS.Config(logger)
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
Expand All @@ -83,7 +116,7 @@
opts = append(opts, grpc.WithStreamInterceptor(tenancy.NewClientStreamInterceptor(tenancyMgr)))
}
var err error
c.remoteConn, err = newClient(c.RemoteServerAddr, opts...)
c.remoteConn, err = c.ToClientConn(context.Background(), componenttest.NewNopHost(), component.TelemetrySettings{}, opts...)
if err != nil {
return nil, fmt.Errorf("error creating remote storage client: %w", err)
}
Expand All @@ -99,7 +132,7 @@
}, nil
}

func (c *Configuration) Close() error {
func (c *ConfigV2) Close() error {
var errs []error
if c.remoteConn != nil {
errs = append(errs, c.remoteConn.Close())
Expand Down
8 changes: 2 additions & 6 deletions plugin/storage/grpc/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,16 @@
package config

import (
"errors"
"testing"

"github.com/stretchr/testify/require"
"go.uber.org/zap"
"google.golang.org/grpc"
)

func TestBuildRemoteNewClientError(t *testing.T) {
// this is a silly test to verify handling of error from grpc.NewClient, which cannot be induced via params.
c := &Configuration{}
_, err := c.buildRemote(zap.NewNop(), nil, func(target string, opts ...grpc.DialOption) (conn *grpc.ClientConn, err error) {
return nil, errors.New("test error")
})
c := &ConfigV2{}
_, err := c.buildRemote(zap.NewNop(), nil)
require.Error(t, err)
require.Contains(t, err.Error(), "error creating remote storage client")
}
2 changes: 1 addition & 1 deletion plugin/storage/grpc/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func NewFactory() *Factory {

// NewFactoryWithConfig is used from jaeger(v2).
func NewFactoryWithConfig(
cfg config.Configuration,
cfg config.ConfigV2,
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
metricsFactory metrics.Factory,
logger *zap.Logger,
) (*Factory, error) {
Expand Down
17 changes: 12 additions & 5 deletions plugin/storage/grpc/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,9 @@ func TestGRPCStorageFactory(t *testing.T) {
}

func TestGRPCStorageFactoryWithConfig(t *testing.T) {
cfg := grpcConfig.ConfigV2{}
_, err := NewFactoryWithConfig(cfg, metrics.NullFactory, zap.NewNop())
require.ErrorContains(t, err, "grpc-plugin builder failed to create a store: error connecting to remote storage")
lis, err := net.Listen("tcp", ":0")
require.NoError(t, err, "failed to listen")

Expand All @@ -159,9 +162,11 @@ func TestGRPCStorageFactoryWithConfig(t *testing.T) {
}()
defer s.Stop()

cfg := grpcConfig.Configuration{
RemoteServerAddr: lis.Addr().String(),
RemoteConnectTimeout: 1 * time.Second,
cfg = grpcConfig.ConfigV2{
Configuration: grpcConfig.Configuration{
RemoteServerAddr: lis.Addr().String(),
RemoteConnectTimeout: 1 * time.Second,
},
}
f, err := NewFactoryWithConfig(cfg, metrics.NullFactory, zap.NewNop())
require.NoError(t, err)
Expand Down Expand Up @@ -312,8 +317,10 @@ func TestWithConfiguration(t *testing.T) {
func TestConfigureFromOptions(t *testing.T) {
f := Factory{}
o := Options{
Configuration: grpcConfig.Configuration{
RemoteServerAddr: "foo:1234",
Configuration: grpcConfig.ConfigV2{
Configuration: grpcConfig.Configuration{
RemoteServerAddr: "foo:1234",
},
},
}
f.configureFromOptions(o)
Expand Down
2 changes: 1 addition & 1 deletion plugin/storage/grpc/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const (
// Options contains GRPC plugins configs and provides the ability
// to bind them to command line flags
type Options struct {
Configuration config.Configuration `mapstructure:",squash"`
Configuration config.ConfigV2 `mapstructure:",squash"`
}

func tlsFlagsConfig() tlscfg.ClientFlagsConfig {
Expand Down
Loading