[WIP][collector] Switch to OTEL's http/grpc server #6277

chahatsagarmain · 2024-11-29T16:51:37Z

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

Signed-off-by: chahatsagarmain <[email protected]>

codecov · 2024-11-29T16:59:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.01%. Comparing base (4a14e87) to head (624bb69).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6277      +/-   ##
==========================================
+ Coverage   48.78%   49.01%   +0.23%     
==========================================
  Files         179      180       +1     
  Lines       10799    11060     +261     
==========================================
+ Hits         5268     5421     +153     
- Misses       5088     5190     +102     
- Partials      443      449       +6

Flag	Coverage Δ
badger_v1	`8.90% <ø> (+0.58%)`	⬆️
badger_v2	`1.63% <ø> (-0.05%)`	⬇️
cassandra-4.x-v1	`?`
cassandra-4.x-v1-manual	`14.84% <ø> (?)`
cassandra-4.x-v2	`?`
cassandra-4.x-v2-auto	`1.57% <ø> (?)`
cassandra-4.x-v2-manual	`1.57% <ø> (?)`
cassandra-5.x-v1	`?`
cassandra-5.x-v1-manual	`14.84% <ø> (?)`
cassandra-5.x-v2	`?`
cassandra-5.x-v2-auto	`1.57% <ø> (?)`
cassandra-5.x-v2-manual	`1.57% <ø> (?)`
elasticsearch-6.x-v1	`18.60% <ø> (+0.01%)`	⬆️
elasticsearch-7.x-v1	`18.68% <ø> (+<0.01%)`	⬆️
elasticsearch-8.x-v1	`18.84% <ø> (-0.02%)`	⬇️
elasticsearch-8.x-v2	`1.63% <ø> (-0.04%)`	⬇️
grpc_v1	`10.37% <ø> (+0.92%)`	⬆️
grpc_v2	`7.91% <ø> (+0.92%)`	⬆️
kafka-v1	`8.58% <ø> (-0.30%)`	⬇️
kafka-v2	`1.63% <ø> (-0.05%)`	⬇️
memory_v2	`1.62% <ø> (-0.05%)`	⬇️
opensearch-1.x-v1	`18.73% <ø> (-0.01%)`	⬇️
opensearch-2.x-v1	`18.73% <ø> (-0.01%)`	⬇️
opensearch-2.x-v2	`1.62% <ø> (-0.06%)`	⬇️
tailsampling-processor	`0.45% <ø> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yurishkuro · 2024-11-29T17:20:33Z

cmd/collector/app/handler/otlp_receiver.go

@@ -106,7 +104,7 @@ func applyGRPCSettings(cfg *configgrpc.ServerConfig, opts *flags.GRPCOptions) {
 		cfg.NetAddr.Endpoint = opts.HostPort
 	}
 	if opts.TLS.Enabled {
-		cfg.TLSSetting = applyTLSSettings(&opts.TLS)
+		cfg.TLSSetting = opts.TLS.ToOtelServerConfig()


the approach in the other PRs is to replace the config field type with configtls, can we use that here too?

If we use configtls.ServerConfig here

jaeger/cmd/collector/app/flags/flags.go

Line 145 in 641ddf2

TLS tlscfg.Options

but the config has no enabled field or insecure field like clientConfig . Then how should i deal with opts.TLS.Enabled

ToOtelServerConfig just returns nil if tls is not enabled , This will work .

Then how should i deal with opts.TLS.Enabled

we should deal with it the same way as in the other PRs. The interface in v1 is CLI flags, it doesn't matter if we translate them into internal TLS config of the one from OTEL.

Signed-off-by: chahatsagarmain <[email protected]>

yurishkuro · 2024-11-29T19:11:06Z

cmd/collector/app/flags/flags.go

@@ -142,7 +143,7 @@ type HTTPOptions struct {
 	// HostPort is the host:port address that the server listens on


I think we should get rid of these HTTPOptions/GRPCOptions types and use confighttp/configgrpc from OTEL. That will also take care of the TLS automatically.

See examples in #6121 and #6055

I booked #6279

so we replace HTTPOptions/GRPCOptions types with the ones from OTEL in CollectorOptions struct?

Signed-off-by: chahatsagarmain <[email protected]>

chahatsagarmain · 2024-11-29T21:06:22Z

@yurishkuro how do i handle tenancy for GRPC after utilzing configgrpc ?

cmd/collector/app/flags/flags.go

yurishkuro · 2024-11-29T21:14:00Z

cmd/collector/app/flags/flags.go

+	corsOpts := corsOTLPFlags.InitFromViper(v)
+	cOpts.OTLP.HTTP.CORS = corsOpts.ToOTELCorsConfig()


why isn't it part of initHTTPFromViper()?

cmd/collector/app/handler/otlp_receiver.go

yurishkuro · 2024-11-29T21:17:32Z

cmd/collector/app/server/grpc.go

@@ -52,9 +53,9 @@ func StartGRPCServer(params *GRPCServerParams) (*grpc.Server, error) {
 		MaxConnectionAgeGrace: params.MaxConnectionAgeGrace,
 	}))

-	if params.TLSConfig.Enabled {
+	if params.TLSConfig != nil {


please see the PRs I linked before - this code now becomes much simpler, we just use OTEL helpers to create listener and server, and TLS is handled automatically

cmd/collector/app/server/http.go

yurishkuro · 2024-11-29T21:18:43Z

pkg/config/corscfg/options.go

 type Options struct {
 	AllowedOrigins []string
 	AllowedHeaders []string
 }
+
+func (o *Options)	ToOTELCorsConfig() *confighttp.CORSConfig{


don't believe we need this, CORS is built into confighttp.Config

yurishkuro · 2024-11-29T21:20:42Z

how do i handle tenancy for GRPC after utilzing configgrpc ?

See examples in #6121 and #6055. There are standard inteceptors for tenancy that we add to both http and grpc.

Signed-off-by: chahatsagarmain <[email protected]>

yurishkuro · 2024-11-29T21:44:28Z

cmd/collector/app/collector.go

@@ -103,24 +99,24 @@ func (c *Collector) Start(options *flags.CollectorOptions) error {
 	c.spanHandlers = handlerBuilder.BuildHandlers(c.spanProcessor)

 	grpcServer, err := server.StartGRPCServer(&server.GRPCServerParams{


I would suggest changing server.GRPCServerParams to directly embed configgrpc struct, so that you don't have to do any field copying here

Signed-off-by: chahatsagarmain <[email protected]>

chahatsagarmain · 2024-11-30T22:22:59Z

jaeger/cmd/collector/app/handler/otlp_receiver.go

Line 63 in 624bb69

    
           fmt.Printf("OTLP HTTP Endpoint: %s\n", otlpReceiverConfig.HTTP.ServerConfig.Endpoint)

@yurishkuro the endpoint is being set correctly in test OTLP HTTP Endpoint: :0 OTLP GRPC Endpoint: :0 but the its throwing an error could not start OTLP receiver: could not start the OTLP receiver: listen: unknown network

yurishkuro · 2024-12-01T01:11:06Z

cmd/collector/app/collector.go

+		ServerConfig: &configgrpc.ServerConfig{
+			NetAddr: confignet.AddrConfig{
+				Endpoint: options.GRPC.NetAddr.Endpoint,
+			},
+			TLSSetting:        options.GRPC.TLSSetting,
+			MaxRecvMsgSizeMiB: options.GRPC.MaxRecvMsgSizeMiB,
+			Keepalive:         options.GRPC.Keepalive,
+		},


couldn't we just assign the whole struct?

Suggested change

ServerConfig: &configgrpc.ServerConfig{

NetAddr: confignet.AddrConfig{

Endpoint: options.GRPC.NetAddr.Endpoint,

},

TLSSetting: options.GRPC.TLSSetting,

MaxRecvMsgSizeMiB: options.GRPC.MaxRecvMsgSizeMiB,

Keepalive: options.GRPC.Keepalive,

},

ServerConfig: options.GRPC,

yurishkuro · 2024-12-01T01:11:23Z

cmd/collector/app/collector.go

 	})
 	if err != nil {
 		return fmt.Errorf("could not start gRPC server: %w", err)
 	}
 	c.grpcServer = grpcServer
-
 	httpServer, err := server.StartHTTPServer(&server.HTTPServerParams{


same as above

yurishkuro · 2024-12-01T01:11:54Z

cmd/collector/app/collector.go

 		c.logger.Info("Not listening for Zipkin HTTP traffic, port not configured")
 	} else {
+		fmt.Printf("%v zipkin\n", options.Zipkin.Endpoint)


add a log instead

yurishkuro · 2024-12-01T01:15:09Z

cmd/collector/app/flags/flags.go

+	}
+	opts.TLSSetting = tlsOpts.ToOtelServerConfig()
+	opts.NetAddr.Endpoint = ports.FormatHostPort(v.GetString(cfg.prefix + "." + flagSuffixHostPort))
+	opts.MaxRecvMsgSizeMiB = v.GetInt(cfg.prefix+"."+flagSuffixGRPCMaxReceiveMessageLength) * 1024 * 1024


Suggested change

opts.MaxRecvMsgSizeMiB = v.GetInt(cfg.prefix+"."+flagSuffixGRPCMaxReceiveMessageLength) * 1024 * 1024

opts.MaxRecvMsgSizeMiB = v.GetInt(cfg.prefix+"."+flagSuffixGRPCMaxReceiveMessageLength) / (1024 * 1024)

yurishkuro · 2024-12-01T01:17:10Z

cmd/collector/app/flags/flags.go

-	cOpts.Zipkin.KeepAlive = v.GetBool(flagZipkinKeepAliveEnabled)
-	cOpts.Zipkin.HTTPHostPort = ports.FormatHostPort(v.GetString(flagZipkinHTTPHostPort))
+	// cOpts.Zipkin. = v.GetBool(flagZipkinKeepAliveEnabled)
+	cOpts.Zipkin.Endpoint = ports.FormatHostPort(v.GetString(flagZipkinHTTPHostPort))


should't this be simply initHTTPFromViper(..., &cOpts.Zipkin, ...)?

yurishkuro · 2024-12-01T01:18:29Z

cmd/collector/app/flags/flags_test.go

@@ -107,7 +107,7 @@ func TestCollectorOptionsWithFlags_CheckMaxReceiveMessageLength(t *testing.T) {
 	_, err := c.InitFromViper(v, zap.NewNop())
 	require.NoError(t, err)

-	assert.Equal(t, 8388608, c.GRPC.MaxReceiveMessageLength)
+	assert.Equal(t, 8388608, c.GRPC.MaxRecvMsgSizeMiB)


shouldn't there be a change in the expectation here, due to different units?

yurishkuro · 2024-12-01T01:19:06Z

cmd/collector/app/flags/flags_test.go

-		"--collector.zipkin.keep-alive=false",
-	})
-	c.InitFromViper(v, zap.NewNop())
+// func TestCollectorOptionsWithFlags_CheckZipkinKeepAlive(t *testing.T) {


After using confighttp.ServerConfig for Zipkin how should i use keepalive ?

confighttp has liveness settings too, what's the problem?

yurishkuro · 2024-12-01T01:20:47Z

cmd/collector/app/server/grpc.go

-	MaxReceiveMessageLength int
-	MaxConnectionAge        time.Duration
-	MaxConnectionAgeGrace   time.Duration
+	*configgrpc.ServerConfig


Suggested change

*configgrpc.ServerConfig

configgrpc.ServerConfig

yurishkuro · 2024-12-01T01:21:33Z

cmd/collector/app/server/http.go

@@ -24,7 +25,7 @@ import (

 // HTTPServerParams to construct a new Jaeger Collector HTTP Server
 type HTTPServerParams struct {
-	TLSConfig        tlscfg.Options
+	TLSConfig        *configtls.ServerConfig


Suggested change

TLSConfig *configtls.ServerConfig

TLSConfig configtls.ServerConfig

yurishkuro · 2024-12-01T01:22:58Z

cmd/collector/app/server/http.go

-	if params.TLSConfig.Enabled {
-		tlsCfg, err := params.TLSConfig.Config(params.Logger) // This checks if the certificates are correctly provided
+	if params.TLSConfig != nil {
+		tlsCfg, err := params.TLSConfig.LoadTLSConfig(context.Background())


below you are creating a listener manually. The point was to use ToListener and ToServer from confighttp

the next thing to do after fixing all reviewed issues 👍

yurishkuro · 2024-12-01T01:24:41Z

could not start the OTLP receiver: listen: unknown network

Are you missing something like this?

$ rg '=.*TCP'
cmd/jaeger/internal/extension/jaegerquery/server_test.go
216:  tt.config.GRPC.NetAddr.Transport = confignet.TransportTypeTCP

use otel helper in collector

641ddf2

Signed-off-by: chahatsagarmain <[email protected]>

chahatsagarmain requested a review from a team as a code owner November 29, 2024 16:51

chahatsagarmain requested a review from joe-elliott November 29, 2024 16:51