Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add config parameters for jaeger receiver #2068

Merged
merged 5 commits into from
Nov 12, 2020
Merged

Add config parameters for jaeger receiver #2068

merged 5 commits into from
Nov 12, 2020

Conversation

pkositsyn
Copy link
Contributor

@pkositsyn pkositsyn commented Nov 5, 2020 •
edited
Loading

Description: Adding a feature - extended config for jaeger receiver

Link to tracking Issue: #2045

Testing: Added test for config

@pkositsyn pkositsyn requested a review from a team November 5, 2020 10:28
@tigrannajaryan tigrannajaryan self-assigned this Nov 6, 2020
tigrannajaryan
tigrannajaryan reviewed Nov 10, 2020
View reviewed changes
Copy link
Member

@tigrannajaryan tigrannajaryan left a 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.

receiver/jaegerreceiver/config.go
@@ -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"`
Copy link
Member

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.

Copy link
Contributor Author

@pkositsyn pkositsyn Nov 10, 2020 •
edited
Loading

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

Copy link
Member

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.

@tigrannajaryan
Copy link
Member

@pavolloffay can you please review Jaeger receiver changes?

@tigrannajaryan
Copy link
Member

@Vemmy124 the build failed.

@pkositsyn
Copy link
Contributor Author

pkositsyn commented Nov 10, 2020 •
edited
Loading

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

@pkositsyn
Add set socket buffer options
d8714db 
Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>
@codecov
Copy link

codecov bot commented Nov 10, 2020 •
edited
Loading

Codecov Report

Merging #2068 (518a282) into master (d322a41) will decrease coverage by 0.01%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
receiver/jaegerreceiver/trace_receiver.go 75.60% <57.14%> (-1.13%) ⬇️
receiver/jaegerreceiver/config.go 100.00% <100.00%> (ø)
receiver/jaegerreceiver/factory.go 94.30% <100.00%> (+0.19%) ⬆️
translator/internaldata/resource_to_oc.go 91.54% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d322a41...518a282. Read the comment docs.

@pkositsyn
Copy link
Contributor Author

@tigrannajaryan @pavolloffay Fixed build and ready for review

pavolloffay
pavolloffay reviewed Nov 11, 2020
View reviewed changes
Copy link
Member

@pavolloffay pavolloffay left a 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.

receiver/jaegerreceiver/config.go
@@ -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"`
Copy link
Member

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.

receiver/jaegerreceiver/factory.go Outdated Show resolved Hide resolved
receiver/jaegerreceiver/factory.go Outdated Show resolved Hide resolved
receiver/jaegerreceiver/factory.go Outdated Show resolved Hide resolved
@pkositsyn
minor style changes
bc64f81 
Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>
@pkositsyn
merge conflict fix
dc39a4d 
Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>
@pkositsyn
Copy link
Contributor Author

@tigrannajaryan @pavolloffay I think everything is resolved now. Do we need to update README as well or these parameters aren't that important?

@bogdandrutu
Copy link
Member

I think we should update the readme with all the params.

@pkositsyn
Add advanced config parameters to README
518a282 
Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>
@pkositsyn
Copy link
Contributor Author

Updated readme

@pkositsyn
Copy link
Contributor Author

@bogdandrutu @tigrannajaryan Can we merge this? Would be great to have config in v0.15.0

@tigrannajaryan tigrannajaryan merged commit 857bcc8 into open-telemetry:master Nov 12, 2020
@pkositsyn pkositsyn deleted the jaeger_receiver_config branch November 12, 2020 22:36
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
@pmcollins
add support for installing dotnet instrumentation (open-telemetry#2068)
7e90099 
add support for installing dotnet instrumentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavolloffay pavolloffay pavolloffay left review comments

@tigrannajaryan tigrannajaryan tigrannajaryan approved these changes

Assignees

@tigrannajaryan tigrannajaryan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pkositsyn @tigrannajaryan @bogdandrutu @pavolloffay