-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Enable gRPC reflection service on collector/query #3526
Conversation
Signed-off-by: Yuri Shkuro <github@ysh.us>
server := grpc.NewServer() | ||
defer server.Stop() | ||
|
||
listener, err := net.Listen("tcp", ":0") |
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.
removed manual setup, using official StartGRPCServer
Codecov Report
@@ Coverage Diff @@
## main #3526 +/- ##
==========================================
- Coverage 96.47% 96.47% -0.01%
==========================================
Files 262 263 +1
Lines 15338 15376 +38
==========================================
+ Hits 14798 14834 +36
- Misses 456 458 +2
Partials 84 84
Continue to review full report at Codecov.
|
} | ||
|
||
// StartGRPCServer based on the given parameters | ||
func StartGRPCServer(params *GRPCServerParams) (*grpc.Server, error) { | ||
var server *grpc.Server | ||
var grpcOpts []grpc.ServerOption | ||
|
||
grpcOpts = append(grpcOpts, grpc.MaxRecvMsgSize(params.MaxReceiveMessageLength)) | ||
if params.MaxReceiveMessageLength > 0 { | ||
grpcOpts = append(grpcOpts, grpc.MaxRecvMsgSize(params.MaxReceiveMessageLength)) |
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.
Adding a 0 value as option actually breaks gRPC message handling/ I guess it wasn't a problem in prod code since the default comes from a CLI flag, but in tests it was annoying having to keep setting this param.
Handler: handler.NewGRPCHandler(logger, &mockSpanProcessor{}), | ||
SamplingStore: &mockSamplingStore{}, | ||
Logger: logger, | ||
MaxReceiveMessageLength: 1024 * 1024, |
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.
made this the only place where param is initialized in tests, to have code coverage for the if statement
Signed-off-by: Yuri Shkuro <github@ysh.us>
@albertteoh please review |
Resolves #3417