-
Notifications
You must be signed in to change notification settings - Fork 1.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
Add config parameters for jaeger receiver #2068
Add config parameters for jaeger receiver #2068
Conversation
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.
Mostly LGTM, just one question on the usage of "UDP" in the names.
@@ -34,8 +33,19 @@ type RemoteSamplingConfig struct { | |||
type Protocols struct { | |||
GRPC *configgrpc.GRPCServerSettings `mapstructure:"grpc"` | |||
ThriftHTTP *confighttp.HTTPServerSettings `mapstructure:"thrift_http"` | |||
ThriftBinary *confignet.TCPAddr `mapstructure:"thrift_binary"` | |||
ThriftCompact *confignet.TCPAddr `mapstructure:"thrift_compact"` | |||
ThriftBinary *ProtocolUDP `mapstructure:"thrift_binary"` |
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.
Is Thrift a UDP-based protocol? For some reason I thought it is TCP-based.
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.
I believe it's not
opentelemetry-collector/receiver/jaegerreceiver/trace_receiver.go
Lines 104 to 105 in bf818a2
agentTransportBinary = "udp_thrift_binary" | |
agentTransportCompact = "udp_thrift_compact" |
The confignet.TCPAddr
structure is used just to store endpoint in this case. Inappropriate place for it in my opinion
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.
Thrift binary and compact are indeed UDP based.
@pavolloffay can you please review Jaeger receiver changes? |
@Vemmy124 the build failed. |
I would also add socket buffer size configurable as it is in https://github.com/jaegertracing/jaeger/blob/681a1eb5c05da2d6b339c665ae1de0e9ca3b7368/cmd/agent/app/servers/thriftudp/socket_buffer.go |
Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>
Codecov Report
@@ Coverage Diff @@
## master #2068 +/- ##
==========================================
- Coverage 92.10% 92.09% -0.02%
==========================================
Files 279 280 +1
Lines 17024 17036 +12
==========================================
+ Hits 15680 15689 +9
- Misses 925 927 +2
- Partials 419 420 +1
Continue to review full report at Codecov.
|
@tigrannajaryan @pavolloffay Fixed build and ready for review |
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.
LGTM, just some nits.
@@ -34,8 +33,19 @@ type RemoteSamplingConfig struct { | |||
type Protocols struct { | |||
GRPC *configgrpc.GRPCServerSettings `mapstructure:"grpc"` | |||
ThriftHTTP *confighttp.HTTPServerSettings `mapstructure:"thrift_http"` | |||
ThriftBinary *confignet.TCPAddr `mapstructure:"thrift_binary"` | |||
ThriftCompact *confignet.TCPAddr `mapstructure:"thrift_compact"` | |||
ThriftBinary *ProtocolUDP `mapstructure:"thrift_binary"` |
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.
Thrift binary and compact are indeed UDP based.
Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>
Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>
@tigrannajaryan @pavolloffay I think everything is resolved now. Do we need to update README as well or these parameters aren't that important? |
I think we should update the readme with all the params. |
Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>
Updated readme |
@bogdandrutu @tigrannajaryan Can we merge this? Would be great to have config in v0.15.0 |
add support for installing dotnet instrumentation
Description: Adding a feature - extended config for jaeger receiver
Link to tracking Issue: #2045
Testing: Added test for config