-
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
[query] Use OTEL's helpers for http server #6121
Changes from 20 commits
e5fa520
2c15f77
fc87756
e45171c
bd25103
49ddae6
396f634
a8feb37
a1beee7
55ac282
48b8934
7862645
461c50f
019ff4a
ad7f508
c4d7ec9
2ef9db3
1024943
32a1327
00eef24
59da7c0
8e2eeb1
f8c5d5c
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 |
---|---|---|
|
@@ -91,8 +91,12 @@ | |
return nil, err | ||
} | ||
registerGRPCHandlers(grpcServer, querySvc, metricsQuerySvc, telset) | ||
|
||
httpServer, err := createHTTPServer(ctx, querySvc, metricsQuerySvc, options, tm, telset) | ||
var httpServer *httpServer | ||
if !separatePorts { | ||
httpServer, err = createHTTPServerLegacy(ctx, querySvc, metricsQuerySvc, options, tm, telset) | ||
} else { | ||
httpServer, err = createHTTPServerOTEL(ctx, querySvc, metricsQuerySvc, options, tm, telset) | ||
} | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
@@ -243,7 +247,7 @@ | |
return handler, staticHandlerCloser | ||
} | ||
|
||
func createHTTPServer( | ||
func createHTTPServerLegacy( | ||
ctx context.Context, | ||
querySvc *querysvc.QueryService, | ||
metricsQuerySvc querysvc.MetricsQueryService, | ||
|
@@ -254,18 +258,58 @@ | |
handler, staticHandlerCloser := initRouter(querySvc, metricsQuerySvc, queryOpts, tm, telset) | ||
handler = responseHeadersHandler(handler, queryOpts.HTTP.ResponseHeaders) | ||
handler = handlers.CompressHandler(handler) | ||
yurishkuro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
recoveryHandler := recoveryhandler.NewRecoveryHandler(telset.Logger, true) | ||
|
||
handler = recoveryhandler.NewRecoveryHandler(telset.Logger, true)(handler) | ||
errorLog, _ := zap.NewStdLogAt(telset.Logger, zapcore.ErrorLevel) | ||
server := &httpServer{ | ||
Server: &http.Server{ | ||
Handler: recoveryHandler(handler), | ||
Handler: handler, | ||
ErrorLog: errorLog, | ||
ReadHeaderTimeout: 2 * time.Second, | ||
}, | ||
staticHandlerCloser: staticHandlerCloser, | ||
} | ||
|
||
if queryOpts.HTTP.TLSSetting != nil { | ||
tlsCfg, err := queryOpts.HTTP.TLSSetting.LoadTLSConfig(ctx) // This checks if the certificates are correctly provided | ||
if err != nil { | ||
return nil, errors.Join(err, staticHandlerCloser.Close()) | ||
} | ||
server.TLSConfig = tlsCfg | ||
} | ||
return server, 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. misleading diff display by GH - this part above did not actually change |
||
|
||
func createHTTPServerOTEL( | ||
ctx context.Context, | ||
querySvc *querysvc.QueryService, | ||
metricsQuerySvc querysvc.MetricsQueryService, | ||
queryOpts *QueryOptions, | ||
tm *tenancy.Manager, | ||
telset telemetery.Setting, | ||
) (*httpServer, error) { | ||
handler, staticHandlerCloser := initRouter(querySvc, metricsQuerySvc, queryOpts, tm, telset) | ||
handler = recoveryhandler.NewRecoveryHandler(telset.Logger, true)(handler) | ||
hs, err := queryOpts.HTTP.ToServer( | ||
ctx, | ||
telset.Host, | ||
component.TelemetrySettings{ | ||
Logger: telset.Logger, | ||
TracerProvider: telset.TracerProvider, | ||
LeveledMeterProvider: func(_ configtelemetry.Level) metric.MeterProvider { | ||
return noop.NewMeterProvider() | ||
}, | ||
}, | ||
handler, | ||
) | ||
if err != nil { | ||
return nil, errors.Join(err, staticHandlerCloser.Close()) | ||
} | ||
server := &httpServer{ | ||
Server: hs, | ||
staticHandlerCloser: staticHandlerCloser, | ||
} | ||
|
||
// TODO why doesn't OTEL helper do that already? | ||
if queryOpts.HTTP.TLSSetting != nil { | ||
tlsCfg, err := queryOpts.HTTP.TLSSetting.LoadTLSConfig(ctx) // This checks if the certificates are correctly provided | ||
if err != nil { | ||
|
@@ -293,7 +337,7 @@ | |
return nil, err | ||
} | ||
|
||
s.httpConn, err = net.Listen("tcp", s.queryOptions.HTTP.Endpoint) | ||
s.httpConn, err = s.queryOptions.HTTP.ToListener(ctx) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -881,32 +881,30 @@ func TestServerHTTP_TracesRequest(t *testing.T) { | |
expectedTrace string | ||
}{ | ||
{ | ||
name: "different ports", | ||
name: "different ports (OTEL implementation)", | ||
httpEndpoint: ":8080", | ||
grpcEndpoint: ":8081", | ||
queryString: "/api/traces/123456aBC", | ||
expectedTrace: "/api/traces/{traceID}", | ||
}, | ||
{ | ||
name: "different ports for v3 api", | ||
name: "different ports (OTEL implementation) for v3 api", | ||
httpEndpoint: ":8080", | ||
grpcEndpoint: ":8081", | ||
queryString: "/api/v3/traces/123456aBC", | ||
expectedTrace: "/api/v3/traces/{trace_id}", | ||
}, | ||
{ | ||
name: "same port", | ||
httpEndpoint: ":8080", | ||
grpcEndpoint: ":8080", | ||
queryString: "/api/traces/123456aBC", | ||
expectedTrace: "/api/traces/{traceID}", | ||
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. why are we loosing this trace? That was my point of adding the test to main first - it should not change in this PR. 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. @yurishkuro We're losing this trace at the moment because removed the call to 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. so how do we explain that? The OTEL helper is supposed to add tracing middleware, why is it not working? 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. @yurishkuro The OTEL helper does add tracing. However, when the ports are the same, we're not using the OTEL helper to initialize the server and are our falling back to our custom implementation. We were previously using 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. Thinking more about this - is there a reason why we can't just use OTEL's helper to create the server even if the ports are the same? We can still use a different flow when we start the listener. 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. @yurishkuro I was actually wondering why we can't just use the server created by 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. How would we co-locate http and grpc servers on the same port in that case? We could if the listener is still done separately by our code. 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. @yurishkuro The listener is initialized separately and it already has a check for separate ports (see https://github.com/jaegertracing/jaeger/blob/main/cmd/query/app/server.go#L289). So can we just simplify the logic of creating the server to always create it using 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. give it a try. Or just add tracing handler to Legacy function and we can merge this PR, then try the listener separately. 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. @yurishkuro I removed the bifurcation for the creation of the server and it looks to be working as expected which makes sense. We can simply rely on OTEL's helpers to create the server. We only need special handling if the ports are the same (and that handling exists already in |
||
name: "same port (legacy implementation) [no tracing]", | ||
httpEndpoint: ":8080", | ||
grpcEndpoint: ":8080", | ||
queryString: "/api/traces/123456aBC", | ||
}, | ||
{ | ||
name: "same port for v3 api", | ||
httpEndpoint: ":8080", | ||
grpcEndpoint: ":8080", | ||
queryString: "/api/v3/traces/123456aBC", | ||
expectedTrace: "/api/v3/traces/{trace_id}", | ||
name: "same port (legacy implementation) for v3 api [no tracing]", | ||
httpEndpoint: ":8080", | ||
grpcEndpoint: ":8080", | ||
queryString: "/api/v3/traces/123456aBC", | ||
}, | ||
} | ||
|
||
|
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.
TODO traceResponseHandler should be moved to initRouter, it can apply to the whole server, not just API handler
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.
@yurishkuro do we want to do that in this PR or a different one?
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.
probably another, already too many changes here to keep track of
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.
#6141