Support tracer env based initialization in hotrod #1115

eundoosong · 2018-10-12T10:47:18Z

Which problem is this PR solving?

Support tracer env based initialization in hotrod
Resolves: Make Go client support JAEGER_ENDPOINT env var and use it in hotrod #920

Short description of the changes

Remove jAgentHostPort(jaeger-agent.host-port) flag.
Replace config.Configuration() by config.FromEnv()
Move metricsFactory.Namespace inside Init()

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

…point_in_hotrod

eundoosong · 2018-10-12T11:56:20Z

Looks like the build error is caused by change in glide.lock
https://travis-ci.org/jaegertracing/jaeger/jobs/440583267
I just did glide update and seems the dependancies related to ingester were updated as well.
glide.lock is generated from glide.yaml. Is there any way to update go-client only?

pavolloffay · 2018-10-12T12:09:53Z

glide.lock

@@ -233,7 +233,7 @@ imports:
 - name: github.com/uber-go/atomic
  version: 8474b86a5a6f79c443ce4b2992817ff32cf208b8
 - name: github.com/uber/jaeger-client-go
-  version: b043381d944715b469fd6b37addfd30145ca1758
+  version: 1a782e2da844727691fef1757c72eb190c2909f0


Can you update the verson in glide.yml?

Yes I did, but still error from ingester.

This reverts commit 04a4652. Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

pavolloffay · 2018-10-12T12:10:48Z

readme needs to be updated too

pavolloffay · 2018-10-12T12:14:56Z

glide update will probably update other deps too. But you didn't add go client bump in glide.yml

eundoosong · 2018-10-12T12:31:38Z

@pavolloffay
Yes I think readme needs to be updated.
When is example-hotrod docker image tagged? I need to specify the verision(probably 1.8?).

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

pavolloffay · 2018-10-12T13:03:11Z

it's tagged during the release process - tag on github. Not need to tag it/change the version explicitly.

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

eundoosong · 2018-10-12T13:43:20Z

examples/hotrod/README.md

@@ -49,10 +49,11 @@ go run ./main.go all
 docker run \
  --rm \
  --link jaeger \
+  --env JAEGER_AGENT_HOST=jaeger \
+  --env JAEGER_AGENT_PORT=6831 \
  -p8080-8083:8080-8083 \
  jaegertracing/example-hotrod:1.6 \


1.6 version is old, so it needs to be updated to accept JAEGER_AGENT_XXX env.
Should I remove version tag so that latest is pulled?

This has to be updated. latest is fine or 1.8

Changed to latest

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

black-adder · 2018-10-12T14:17:21Z

let's pin sarama and sarama-cluster to 1.16.0 and 2.1.13 respectively, looks like there was a breaking change.

black-adder · 2018-10-12T14:22:00Z

More specifically https://github.com/bsm/sarama-cluster/blame/f7d869db45fd8d74bf62c02a9d17637f945668d0/partitions.go#L23 was added to the interface in the newest version of sarama-cluster

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

codecov · 2018-10-12T14:59:59Z

Codecov Report

Merging #1115 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1115   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         142     142           
  Lines        6743    6743           
======================================
  Hits         6743    6743

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 732ef79...3d3d8cf. Read the comment docs.

eundoosong · 2018-10-12T15:01:09Z

Pin sarama and sarama-cluster to 1.16.0 and 2.1.13 is done.
It's ready for review.

black-adder · 2018-10-12T18:33:05Z

@eundoosong thanks!

Add env based initialization in hotrod exmaple

35207d9

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

eundoosong requested review from black-adder, jpkrohling, pavolloffay, vprithvi and yurishkuro as code owners October 12, 2018 10:47

eundoosong force-pushed the remove_jaeger_endpoint_in_hotrod branch from 18be51c to 9ba407b Compare October 12, 2018 10:48

eundoosong added 2 commits October 12, 2018 19:52

Update README.md

04a4652

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

Add error handling when config.FromEnv fails

87116e9

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

eundoosong force-pushed the remove_jaeger_endpoint_in_hotrod branch 2 times, most recently from 07a2cb0 to 87116e9 Compare October 12, 2018 11:04

Merge remote-tracking branch 'upstream/master' into remove_jaeger_end…

30d87fc

…point_in_hotrod

pavolloffay changed the title ~~Support env based initialization in hotrod~~ Support tracer env based initialization in hotrod Oct 12, 2018

pavolloffay reviewed Oct 12, 2018

View reviewed changes

Revert "Update README.md"

aa7edfb

This reverts commit 04a4652. Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

eundoosong force-pushed the remove_jaeger_endpoint_in_hotrod branch from 18bad7a to aa7edfb Compare October 12, 2018 12:10

Bump go-client version

485c6c2

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

Update README.md

620fd7d

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

eundoosong force-pushed the remove_jaeger_endpoint_in_hotrod branch from 34c0f68 to 620fd7d Compare October 12, 2018 13:31

eundoosong commented Oct 12, 2018

View reviewed changes

Change to latest in README.md

266c14f

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

Pin sarama and sarama-cluster version

3d3d8cf

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

black-adder approved these changes Oct 12, 2018

View reviewed changes

black-adder merged commit fb10d4c into jaegertracing:master Oct 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support tracer env based initialization in hotrod #1115

Support tracer env based initialization in hotrod #1115

eundoosong commented Oct 12, 2018 •

edited

Loading

eundoosong commented Oct 12, 2018 •

edited

Loading

pavolloffay Oct 12, 2018

eundoosong Oct 12, 2018

pavolloffay commented Oct 12, 2018

pavolloffay commented Oct 12, 2018

eundoosong commented Oct 12, 2018

pavolloffay commented Oct 12, 2018

eundoosong Oct 12, 2018

pavolloffay Oct 12, 2018

eundoosong Oct 12, 2018

black-adder commented Oct 12, 2018

black-adder commented Oct 12, 2018

codecov bot commented Oct 12, 2018 •

edited

Loading

eundoosong commented Oct 12, 2018

black-adder commented Oct 12, 2018

Support tracer env based initialization in hotrod #1115

Support tracer env based initialization in hotrod #1115

Conversation

eundoosong commented Oct 12, 2018 • edited Loading

Which problem is this PR solving?

Short description of the changes

eundoosong commented Oct 12, 2018 • edited Loading

pavolloffay Oct 12, 2018

Choose a reason for hiding this comment

eundoosong Oct 12, 2018

Choose a reason for hiding this comment

pavolloffay commented Oct 12, 2018

pavolloffay commented Oct 12, 2018

eundoosong commented Oct 12, 2018

pavolloffay commented Oct 12, 2018

eundoosong Oct 12, 2018

Choose a reason for hiding this comment

pavolloffay Oct 12, 2018

Choose a reason for hiding this comment

eundoosong Oct 12, 2018

Choose a reason for hiding this comment

black-adder commented Oct 12, 2018

black-adder commented Oct 12, 2018

codecov bot commented Oct 12, 2018 • edited Loading

Codecov Report

eundoosong commented Oct 12, 2018

black-adder commented Oct 12, 2018

eundoosong commented Oct 12, 2018 •

edited

Loading

eundoosong commented Oct 12, 2018 •

edited

Loading

codecov bot commented Oct 12, 2018 •

edited

Loading