-
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
Merged
Merged
Changes from 14 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
e5fa520
Use OTEL Helper For Creating HTTP Server
mahadzaryab1 2c15f77
Remove OTEL HTTP Handler
mahadzaryab1 fc87756
Add Route Tag Handler Back
mahadzaryab1 e45171c
Remove Commented Out Test
mahadzaryab1 bd25103
Move Handling Logic Inline
mahadzaryab1 49ddae6
Factor Common Functionality Into Helper
mahadzaryab1 396f634
Fix Linting
mahadzaryab1 a8feb37
Merge branch 'main' into http-otel
mahadzaryab1 a1beee7
Move Tenancy Handler To Server
mahadzaryab1 55ac282
Revert "Move Tenancy Handler To Server"
mahadzaryab1 48b8934
Merge branch 'main' into http-otel
mahadzaryab1 7862645
Merge branch 'main' into http-otel
yurishkuro 461c50f
fix merge issues
yurishkuro 019ff4a
Fix Lint Error
mahadzaryab1 ad7f508
Add Span Name Handler
mahadzaryab1 c4d7ec9
Add More Tests
mahadzaryab1 2ef9db3
Add More Tests
mahadzaryab1 1024943
Fix Lint
mahadzaryab1 32a1327
Merge branch 'main' into http-otel
mahadzaryab1 00eef24
Fix Naming
mahadzaryab1 59da7c0
Only Use OTEL's Helper To Create Server
mahadzaryab1 8e2eeb1
Revert Test Name
mahadzaryab1 f8c5d5c
Merge branch 'main' into http-otel
mahadzaryab1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
} | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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