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.

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 Jaeger Agent Configuration #434

Merged
merged 29 commits into from Dec 4, 2019

Conversation

joe-elliott
Copy link
Contributor

@joe-elliott joe-elliott commented Nov 25, 2019

Adds the ability to configure the Jaeger Agent protocols independently. Resolves #157.

Changes:

  • Added documentation about new configuration options
  • Added new options to config tests
  • Removed agent *agentapp.Agent because the agent did not support independently configuring the http server and agent protocols.
  • Added agentProcessors/agentServer to allow for independent configuration of remote sampling proxy as well as agent protocols.
  • Added structure to support future addition of jaeger http proxy for remote sampling/baggage.
  • Correctly set the SourceFormat to jaeger for spans ingested through the agent services.

Tests pass and this has been manually tested in our internal environment.

cc @annanay25

@codecov-io
Copy link

codecov-io commented Nov 25, 2019

Codecov Report

Merging #434 into master will increase coverage by 0.21%.
The diff coverage is 92.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #434      +/-   ##
==========================================
+ Coverage   75.27%   75.48%   +0.21%     
==========================================
  Files         120      120              
  Lines        7303     7338      +35     
==========================================
+ Hits         5497     5539      +42     
+ Misses       1542     1535       -7     
  Partials      264      264
Impacted Files Coverage Δ
internal/testutils/testutils.go 71.87% <100%> (+16.87%) ⬆️
receiver/jaegerreceiver/factory.go 92.13% <100%> (+1.46%) ⬆️
receiver/jaegerreceiver/trace_receiver.go 81.01% <88.23%> (+4.09%) ⬆️
...mplingprocessor/tailsamplingprocessor/processor.go 70.04% <0%> (+0.13%) ⬆️

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 e94dd19...37dae1d. Read the comment docs.

@joe-elliott
Copy link
Contributor Author

@bogdandrutu this PR allows for independently configurable jaeger agent protocols as well as sets up a place for @annanay25 to add the jaeger remote sampling configuration/functionality.

@annanay25
Copy link
Contributor

@joe-elliott - This is great! :)

About jaeger remote sampling configuration, I think its best to wait to reach consensus on where the package should live, and then the configuration options can be provided either as part of the receiver or the extension.

receiver/jaegerreceiver/jaeger_agent_test.go Outdated Show resolved Hide resolved
receiver/jaegerreceiver/jaeger_agent_test.go Outdated Show resolved Hide resolved
receiver/jaegerreceiver/trace_receiver.go Outdated Show resolved Hide resolved
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.

LGTM, but I am not very familiar with Jaeger receiver, so we need more reviews.

@bogdandrutu
Copy link
Member

@joe-elliott please pay the coverage cost :)

Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
…t it

Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
…ent and collector protocols

Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
… http server

Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
Copy link
Contributor Author

@tigrannajaryan thanks for the review
@bogdandrutu Coverage is significantly better now.

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @joe-elliott. LGTM, just a few nit suggestions and one Q.

receiver/jaegerreceiver/jaeger_agent_test.go Outdated Show resolved Hide resolved
receiver/jaegerreceiver/jaeger_agent_test.go Show resolved Hide resolved
receiver/jaegerreceiver/jaeger_agent_test.go Outdated Show resolved Hide resolved
receiver/jaegerreceiver/trace_receiver.go Outdated Show resolved Hide resolved
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
@pjanotti pjanotti merged commit c897290 into open-telemetry:master Dec 4, 2019
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
Handling of nil retrieved values by the config provider manager was causing the string "<nil>" to be used. Fixed the handling on the manager and removed the special case on the env variable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Jaeger Agent configuration options
6 participants