Adds support for agent level tag #1396

annanay25 · 2019-03-03T19:44:30Z

Signed-off-by: Annanay annanay.a@media.net

Which problem is this PR solving?

Resolves Support for Agent level tag #833

Short description of the changes

Uses a --jaeger.tags flag to enable tag addition (Ex: --jaeger.tags=key=value). This can be used with env variables.

yurishkuro · 2019-03-03T21:31:29Z

cmd/agent/app/reporter/flags.go

 }

 // AddFlags adds flags for Options.
 func AddFlags(flags *flag.FlagSet) {
 	flags.String(reporterType, string(GRPC), fmt.Sprintf("Reporter type to use e.g. %s, %s", string(GRPC), string(TCHANNEL)))
+	flags.String(agentTags, "", fmt.Sprintf("Add agent-based tags. Ex: --jaeger.tags key1=value1,${envVar:defaultValue}"))


what does ,${envVar:defaultValue} mean?

Copied over from https://github.com/jaegertracing/jaeger-client-go/blob/c58709fe19d793cdd40ad8bfa6e645a40a2eccf8/config/config_env.go#L195-L199 -

// parseTags parses the given string into a collection of Tags. // Spec for this value: // - comma separated list of key=value // - value can be specified using the notation ${envVar:defaultValue}, where `envVar` // is an environment variable and `defaultValue` is the value to use in case the env var is not set

cmd/agent/app/reporter/flags.go

cmd/agent/app/reporter/grpc/reporter.go

yurishkuro · 2019-03-03T21:38:06Z

cmd/agent/app/reporter/grpc/reporter.go

+func addAgentTags(spans []*model.Span, agentTags []model.KeyValue) []*model.Span {
+	for _, span := range spans {
+		for _, tag := range agentTags{
+			span.Tags = append(span.Tags, tag)


top-level question for the PR: should these be added to Span tags or Process tags?

It might depend. For example in k8s we support deploying agent as a side-car but also as daemon set (1 agent per k8s node).

I think we would need an additional flag to be able to add it to process tags

I think we would need an additional flag to be able to add it to process tags

Is this the consensus?

cmd/agent/app/reporter/grpc/reporter.go

cmd/agent/app/reporter/tchannel/reporter.go

pavolloffay · 2019-03-05T08:14:19Z

cmd/agent/app/reporter/grpc/reporter.go

+func addAgentTags(spans []*model.Span, agentTags []model.KeyValue) []*model.Span {
+	for _, span := range spans {
+		for _, tag := range agentTags{
+			span.Tags = append(span.Tags, tag)


It might depend. For example in k8s we support deploying agent as a side-car but also as daemon set (1 agent per k8s node).

I think we would need an additional flag to be able to add it to process tags

cmd/agent/app/reporter/tchannel/reporter.go

annanay25 · 2019-03-05T17:32:03Z

Thanks for the reviews. Addressed comments. Just the top level question remains.

codecov · 2019-03-12T19:39:10Z

Codecov Report

❗ No coverage uploaded for pull request base (master@1157a7d). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff            @@
##             master   #1396   +/-   ##
========================================
  Coverage          ?    100%           
========================================
  Files             ?     165           
  Lines             ?    7538           
  Branches          ?       0           
========================================
  Hits              ?    7538           
  Misses            ?       0           
  Partials          ?       0

Impacted Files	Coverage Δ
cmd/agent/app/builder.go	`100% <100%> (ø)`
cmd/agent/app/reporter/flags.go	`100% <100%> (ø)`
cmd/agent/app/reporter/grpc/collector_proxy.go	`100% <100%> (ø)`
cmd/agent/app/reporter/grpc/reporter.go	`100% <100%> (ø)`

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 1157a7d...b8b7c5d. Read the comment docs.

yurishkuro

The change is not particularly complex, I think the coverage can be kept at 100.

We may want to consider excluding changes to TChannel reporter dealing with jaeger.thrift/zipkin.thrift, because we're planning to deprecate & remove TChannel reporter in the near future, so this will reduce the amount of tests needed to keep the coverage. But ok to keep too.

cmd/agent/app/reporter/flags.go

cmd/agent/app/reporter/grpc/reporter.go

cmd/agent/app/reporter/tchannel/reporter.go

Signed-off-by: Annanay <annanay.a@media.net>

Signed-off-by: Annanay <annanay.a@media.net> nit, fix function call Signed-off-by: Annanay <annanay.a@media.net> nit, fix function call Signed-off-by: Annanay <annanay.a@media.net> fix fmt, gosimple Signed-off-by: Annanay <annanay.a@media.net> make fmt

Signed-off-by: Annanay <annanay.a@media.net> Fix tag names Signed-off-by: Annanay <annanay.a@media.net>

Signed-off-by: Annanay <annanay.a@media.net>

annanay25 · 2019-03-13T18:48:54Z

Thanks for the review @yurishkuro!

I've added tests only for the grpc reporter for now, but do let me know if TChannel reporter tests are also required.

yurishkuro · 2019-03-14T03:37:18Z

If we're not adding tchannel tests, we shouldn't change tchannel code either, otherwise coverage will go down (which I can't see now because the bloody crossdock test failed again).

annanay25 · 2019-03-15T18:39:16Z

Codecov is at 73%, let me remove all changes to tchannel reporter.

Signed-off-by: Annanay <annanay.a@media.net>

annanay25 · 2019-03-15T18:57:18Z

Crossdock again 😅

yurishkuro · 2019-03-15T19:32:48Z

the main addition seems to be untested

Signed-off-by: Annanay <annanay.a@media.net>

cmd/agent/app/reporter/grpc/reporter.go

cmd/agent/app/reporter/flags.go

cmd/agent/app/reporter/flags_test.go

Signed-off-by: Annanay <annanay.a@media.net>

cmd/agent/app/reporter/flags.go

cmd/agent/app/reporter/flags_test.go

cmd/agent/app/reporter/grpc/reporter_test.go

cmd/agent/app/reporter/grpc/reporter.go

cmd/agent/app/reporter/flags.go

cmd/agent/app/reporter/grpc/reporter.go

Signed-off-by: Annanay <annanay.a@media.net>

cmd/agent/app/reporter/grpc/reporter.go

Signed-off-by: Annanay <annanay.a@media.net>

yurishkuro

🎉

annanay25 · 2019-03-19T18:20:28Z

Thanks! :)

yurishkuro reviewed Mar 3, 2019

View reviewed changes

pavolloffay reviewed Mar 4, 2019

View reviewed changes

cmd/agent/app/reporter/grpc/reporter.go Outdated Show resolved Hide resolved

pavolloffay reviewed Mar 4, 2019

View reviewed changes

cmd/agent/app/reporter/tchannel/reporter.go Outdated Show resolved Hide resolved

annanay25 changed the title ~~WIP - Adds support for agent level tag~~ Adds support for agent level tag Mar 4, 2019

annanay25 marked this pull request as ready for review March 4, 2019 18:13

annanay25 requested review from black-adder, jpkrohling, objectiser, tiffon and vprithvi as code owners March 4, 2019 18:13

annanay25 force-pushed the agent-tags branch from 7945b76 to 9d9aeb5 Compare March 4, 2019 18:31

pavolloffay reviewed Mar 5, 2019

View reviewed changes

annanay25 force-pushed the agent-tags branch 2 times, most recently from 3cd2caf to 82ba1d3 Compare March 10, 2019 20:10

yurishkuro reviewed Mar 12, 2019

View reviewed changes

Annanay added 10 commits March 13, 2019 23:59

Basic POC for agent based tags

cf27602

Signed-off-by: Annanay <annanay.a@media.net>

Address comments

651745d

Signed-off-by: Annanay <annanay.a@media.net>

Rebased

ffa421e

Signed-off-by: Annanay <annanay.a@media.net>

Fix tests and build errors

672a63f

Signed-off-by: Annanay <annanay.a@media.net>

Address comments

9517822

Signed-off-by: Annanay <annanay.a@media.net>

nit, fix tests

74273b2

Signed-off-by: Annanay <annanay.a@media.net> nit, fix function call Signed-off-by: Annanay <annanay.a@media.net> nit, fix function call Signed-off-by: Annanay <annanay.a@media.net> fix fmt, gosimple Signed-off-by: Annanay <annanay.a@media.net> make fmt

Start adding tests

dc0e9f1

Signed-off-by: Annanay <annanay.a@media.net> Fix tag names Signed-off-by: Annanay <annanay.a@media.net>

Fix TestCollectorProxy test

10bb8ad

Signed-off-by: Annanay <annanay.a@media.net>

Tags are added in send, not in EmitZipkinBatch

bea5bff

Signed-off-by: Annanay <annanay.a@media.net>

Addressed comments, added tests

2b8d41e

Signed-off-by: Annanay <annanay.a@media.net>

annanay25 force-pushed the agent-tags branch from 0489f2b to 2b8d41e Compare March 13, 2019 18:32

Remove debug print

fa44270

Signed-off-by: Annanay <annanay.a@media.net>

Remove all changes to tchannel reporter

0ed7344

Signed-off-by: Annanay <annanay.a@media.net>

Add tests, fix code coverage

8b2b2e4

Signed-off-by: Annanay <annanay.a@media.net>

yurishkuro reviewed Mar 16, 2019

View reviewed changes

cmd/agent/app/reporter/grpc/reporter.go Outdated Show resolved Hide resolved

cmd/agent/app/reporter/flags.go Outdated Show resolved Hide resolved

cmd/agent/app/reporter/flags_test.go Outdated Show resolved Hide resolved

Address comments, run make fmt

8f24218

Signed-off-by: Annanay <annanay.a@media.net>

yurishkuro reviewed Mar 17, 2019

View reviewed changes

Annanay added 2 commits March 18, 2019 19:50

Addressed comments

0459f5b

Signed-off-by: Annanay <annanay.a@media.net>

Address comments

b9884c1

Signed-off-by: Annanay <annanay.a@media.net>

yurishkuro reviewed Mar 18, 2019

View reviewed changes

cmd/agent/app/reporter/grpc/reporter.go Show resolved Hide resolved

Annanay and others added 2 commits March 19, 2019 00:37

Address comments

7979740

Signed-off-by: Annanay <annanay.a@media.net>

Merge branch 'master' into agent-tags

b8b7c5d

yurishkuro approved these changes Mar 18, 2019

View reviewed changes

yurishkuro merged commit 8e76a50 into jaegertracing:master Mar 18, 2019

yurishkuro mentioned this pull request Mar 19, 2019

Support for Agent level tag #833

Closed

annanay25 mentioned this pull request Mar 31, 2019

Document support for agent level tags jaegertracing/documentation#236

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for agent level tag #1396

Adds support for agent level tag #1396

annanay25 commented Mar 3, 2019 •

edited

Loading

yurishkuro Mar 3, 2019

annanay25 Mar 4, 2019

yurishkuro Mar 3, 2019

pavolloffay Mar 5, 2019

annanay25 Mar 5, 2019 •

edited

Loading

pavolloffay Mar 5, 2019

annanay25 commented Mar 5, 2019

codecov bot commented Mar 12, 2019 •

edited

Loading

yurishkuro left a comment

annanay25 commented Mar 13, 2019

yurishkuro commented Mar 14, 2019

annanay25 commented Mar 15, 2019

annanay25 commented Mar 15, 2019

yurishkuro commented Mar 15, 2019

yurishkuro left a comment

annanay25 commented Mar 19, 2019

Adds support for agent level tag #1396

Adds support for agent level tag #1396

Conversation

annanay25 commented Mar 3, 2019 • edited Loading

Which problem is this PR solving?

Short description of the changes

yurishkuro Mar 3, 2019

Choose a reason for hiding this comment

annanay25 Mar 4, 2019

Choose a reason for hiding this comment

yurishkuro Mar 3, 2019

Choose a reason for hiding this comment

pavolloffay Mar 5, 2019

Choose a reason for hiding this comment

annanay25 Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

pavolloffay Mar 5, 2019

Choose a reason for hiding this comment

annanay25 commented Mar 5, 2019

codecov bot commented Mar 12, 2019 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

annanay25 commented Mar 13, 2019

yurishkuro commented Mar 14, 2019

annanay25 commented Mar 15, 2019

annanay25 commented Mar 15, 2019

yurishkuro commented Mar 15, 2019

yurishkuro left a comment

Choose a reason for hiding this comment

annanay25 commented Mar 19, 2019

annanay25 commented Mar 3, 2019 •

edited

Loading

annanay25 Mar 5, 2019 •

edited

Loading

codecov bot commented Mar 12, 2019 •

edited

Loading