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

cmd/query: init tracer config from env #1800

Closed

Conversation

holycheater
Copy link

Which problem is this PR solving?

Resolves #1044

Getting a lot of these errors in jaeger-query docker image:
{"level":"error","ts":1568465347.7179556,"caller":"zap/logger.go:33","msg":"error when flushing the buffer: write udp 127.0.0.1:59514->127.0.0.1:6831: write: connection refused","stacktrace":"github.com/jaegertracing/jaeger/vendor/github.com/uber/jaeger-client-go/log/zap.(*Logger).Error\n\t/home/travis/gopath/src/github.com/jaegertracing/jaeger/vendor/github.com/uber/jaeger-client-go/log/zap/logger.go:33\ngitpro.ttaallkk.top/jaegertracing/jaeger/vendor/github.com/uber/jaeger-client-go.(*remoteReporter).processQueue.func1\n\t/home/travis/gopath/src/github.com/jaegertracing/jaeger/vendor/github.com/uber/jaeger-client-go/reporter.go:257\ngitpro.ttaallkk.top/jaegertracing/jaeger/vendor/github.com/uber/jaeger-client-go.(*remoteReporter).processQueue\n\t/home/travis/gopath/src/github.com/jaegertracing/jaeger/vendor/github.com/uber/jaeger-client-go/reporter.go:267"}

Short description of the changes

Using jaeger-client-go FromEnv configuration

},
RPCMetrics: true,
}.NewTracer(
cfg, err := jaegerClientConfig.FromEnv()
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, this is not backwards compatible. This really needs to be fixed in the client, so that FromEnv could be called on an already populated config as a for of override.

Copy link
Author

Choose a reason for hiding this comment

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

Making changes to client for this single application seems like an overkill
I've made changes in tracer config initialization, should be backwards-compatible now

@codecov
Copy link

codecov bot commented Sep 14, 2019

Codecov Report

Merging #1800 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1800      +/-   ##
==========================================
- Coverage   98.21%   98.17%   -0.05%     
==========================================
  Files         195      195              
  Lines        9602     9602              
==========================================
- Hits         9431     9427       -4     
- Misses        134      137       +3     
- Partials       37       38       +1
Impacted Files Coverage Δ
cmd/query/app/static_handler.go 83.33% <0%> (-3.51%) ⬇️

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 3fa1dfa...2c5732a. Read the comment docs.

@holycheater holycheater force-pushed the query-tracer-conf branch 2 times, most recently from 14b57fa to b7b5b1a Compare September 14, 2019 20:06
Signed-off-by: Alexander Saltykov <alexander-s@yandex-team.ru>
@yurishkuro
Copy link
Member

Sorry, I was probably unclear. I don't think this repository needs to solve it by re-implementing env var lookup. The issue is in the jaeger-client-go: jaegertracing/jaeger-client-go#430

@pavolloffay
Copy link
Member

Done in #1919

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.

Allow configuration of jaeger client in the query service
3 participants