Support collector tags, similar to agent tags #1854

radekg · 2019-10-10T17:56:52Z

Which problem is this PR solving?

This PR implements the collector Jaeger tags: Resolves #1844.

Short description of the changes

Adds support for collector --jaeger.tags attribute. Reuses Jaeger tags handling code from the agent.

yurishkuro · 2019-10-10T20:46:09Z

cmd/agent/app/reporter/flags.go

 	return b
 }
-
-// Parsing logic borrowed from jaegertracing/jaeger-client-go
-func parseAgentTags(agentTags string) map[string]string {


Note that this package currently has 100% coverage

$ go test -cover ./cmd/agent/app/reporter/ ok github.com/jaegertracing/jaeger/cmd/agent/app/reporter 0.069s coverage: 100.0% of statements

which means there is a test covering this function, which should also move to the new location.

I am not sure if moving this test out of there is a good idea? It does what it should be doing, parses agent options with tags. I've added additional tests to cmd/flags. I could remove TestBindFlags test from cmd/agent/app/reporter/flags_test.go but I think that would leave the actual option handling without coverage?

Moving the complete test from cmd/agent/app/reporter/flags_test.go to cmd/flags/flags_test.go does not seem to be an option as it results in circular import.

yurishkuro · 2019-10-10T20:48:18Z

cmd/collector/app/span_processor.go

@@ -159,6 +161,11 @@ func (sp *spanProcessor) enqueueSpan(span *model.Span, originalFormat SpanFormat
 	//add format tag
 	span.Tags = append(span.Tags, model.String("internal.span.format", string(originalFormat)))

+	// append the collector tags
+	for k, v := range sp.collectorTags {


add // TODO add support for deduping tags, https://github.com/jaegertracing/jaeger/issues/1778

Done. I have moved collector tags appending to a separate function to make it testable.

jpkrohling

Nice job, @radekg! This LGTM, just make sure to run make fmt, as Travis is failing with:

Go fmt, license check, or import ordering failures, run 'make fmt'

Also, make sure to add a sign-off to your commit, otherwise the DCO check will fail. Click on "Details" for that check how to get that done.

radekg · 2019-10-11T21:09:15Z

@yurishkuro I'm not sure if the tests are failing because of my changes, first look suggests it's not my changes but I could be wrong.

codecov · 2019-10-11T21:23:54Z

Codecov Report

Merging #1854 into master will decrease coverage by 1.44%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master    #1854      +/-   ##
==========================================
- Coverage   98.44%   96.99%   -1.45%     
==========================================
  Files         199      203       +4     
  Lines        9892    10061     +169     
==========================================
+ Hits         9738     9759      +21     
- Misses        118      264     +146     
- Partials       36       38       +2

Impacted Files	Coverage Δ
cmd/collector/app/options.go	`100% <100%> (ø)`	⬆️
cmd/collector/app/builder/span_handler_builder.go	`100% <100%> (ø)`	⬆️
cmd/collector/app/builder/builder_flags.go	`100% <100%> (ø)`	⬆️
cmd/all-in-one/setupcontext/setupcontext.go	`100% <100%> (ø)`
cmd/collector/app/span_processor.go	`100% <100%> (ø)`	⬆️
cmd/agent/app/reporter/flags.go	`78.57% <72.72%> (-21.43%)`	⬇️
cmd/flags/flags.go	`43.47% <90.9%> (ø)`
cmd/flags/admin.go	`0% <0%> (ø)`
cmd/flags/service.go	`0% <0%> (ø)`
... and 2 more

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 e9216d9...5f07676. Read the comment docs.

yurishkuro · 2019-10-13T22:26:03Z

@radekg yes, the failure is due to this change, because in all-in-one the flags from agent and collector are combined.

+docker logs 45563e652ba5f0640d4d26904178ab60d5ee9647896330923ead9db0e960e79d
2019/10/13 21:31:09 maxprocs: Leaving GOMAXPROCS=2: CPU quota undefined
flag redefined: jaeger.tags
panic: flag redefined: jaeger.tags

jpkrohling · 2019-10-14T09:31:31Z

The fuzzing job got cancelled because two previous jobs failed, which I manually restarted (and have now succeeded). I manually just triggered the fuzz job as well.

radekg · 2019-10-14T11:30:56Z

Thank you @jpkrohling. Tests are passing now. This feature is complete and ready for review. @yurishkuro, I've renamed the collector flag to --collector.jaeger.tags, wasn't initially aware of the clash in all-in-one case. Obviously, it would not be possible to share the flag in that case.

objectiser · 2019-10-14T15:38:37Z

Wondering if would be better to call the new flag collector.tags and then rename (via deprecation) jaeger.tags to agent.tags?

radekg · 2019-10-14T18:14:25Z

@objectiser this is a good idea, done in the latest commit.

pavolloffay · 2019-10-15T08:52:20Z

Wondering if would be better to call the new flag collector.tags and then rename (via deprecation) jaeger.tags to agent.tags?

What is the motivation behind this? Both add tags to process object.

radekg · 2019-10-15T09:02:17Z

Is it reasonable to assume a user might want to add certain tags on the agent and others on the collector?
Edit: one possibility would be, the user might be using all-in-one for setup prototyping and use that as a basis for the distributed infrastructure.

objectiser · 2019-10-15T09:03:48Z

@pavolloffay When used together in the all-in-one it just makes it clear that one is being set by the collector and the other when the agent is used. jaeger.tags is unclear in this context - and use of jaeger in the name is irrelevant anyway, as it is obviously a jaeger parameter.

jpkrohling · 2019-10-15T09:04:33Z

Is it reasonable to assume a user might want to add certain tags on the agent and others on the collector?

If they are in different processes, yes. If they are in the same process (all-in-one), I don't think it makes much sense to add tags to the agent and to the collector.

objectiser · 2019-10-15T09:06:28Z

If it is possible to skip adding the flag to the agent, when running inside all-in-one, then that would probably be the best solution.

radekg · 2019-10-15T09:50:58Z

There are 2 options here:

keep separate flags, maintains predictability in the behavior, agent tags belong to an agent, collector tags to a collector, prevents a potential problem the user would be facing when migrating from all-in-one to distributed
use a single flag for both components, the precedence does not really matter, the complexity for the user arises when moving from all-in-one (for example: agent_tag_1=x,collector_tag_1=y), where the user might have to split agent and collector related tags, this is inconvenient because it is not possible to copy paste settings (think helm charts, docker compose and so on), another use case here would be having a local dev setup and a production setup, migrating dev setup to prod takes that additional step of having to manually amend the config; from experience, condition based configuration is often a source of problems

It is possible to find out if the flag was defined by using flag.Lookup. The question would be which flags take precedence in all-in-one but that feels like sprinkling magic instead of having a predictable interface.

The question is: which solution is preferred. My preference would be to keep separate flags.

radekg · 2019-10-23T11:05:50Z

Hi everyone, what's the status on this? It's not clear to me in what direction should this be taken.

jpkrohling · 2019-10-23T11:10:21Z

Sorry, I missed this one. I'd say that each can have its own flag, but the all-in-one should not accept the agent's flag.

radekg · 2019-10-24T11:48:12Z

@jpkrohling not accept as in ignore or not even have a flag available.

…aegertracing#1832 (jaegertracing#1842) Signed-off-by: Michael Burman <[email protected]> Signed-off-by: radekg <[email protected]>

… 0 (jaegertracing#1868) Signed-off-by: Juraci Paixão Kröhling <[email protected]> Signed-off-by: radekg <[email protected]>

Signed-off-by: Pavol Loffay <[email protected]> Signed-off-by: radekg <[email protected]>

* Use Elasticsearch 6 in xdock Signed-off-by: Pavol Loffay <[email protected]> * Fix issues and use single node ES Signed-off-by: Pavol Loffay <[email protected]> Signed-off-by: radekg <[email protected]>

…rocess Signed-off-by: radekg <[email protected]>

Signed-off-by: radekg <[email protected]>

jpkrohling · 2019-10-28T11:27:15Z

The build is failing with:

*** directories without *_test.go files:

./cmd/all-in-one/setupcontext

error: at least one *_test.go file must be in all directories with go files so that they are counted for code coverage

       if no tests are possible for a package (e.g. it only defines types), create empty_test.go

Is that new? I don't remember seeing this requirement before...

radekg · 2019-10-28T13:11:09Z

Maybe something to do with code coverage running only on touched paths? Full run of tests covers this. I’ll add a test.

yurishkuro · 2019-10-28T13:49:00Z

It's now new, we had this validation for a long time. Go does not report coverage for packages with no tests, creating misleading numbers. A rudimentary test file would fix it.

jpkrohling · 2019-10-28T13:51:00Z

And directories with .nocover would bypass this error, I assume. That's probably why I don't remember seeing that message before.

yurishkuro · 2019-10-28T14:00:57Z

Yes but we should really get rid of those nocover files, since we relaxed 100% coverage requirement.

Signed-off-by: radekg <[email protected]>

radekg · 2019-10-28T14:18:37Z

Test added.

Signed-off-by: radekg <[email protected]>

yurishkuro · 2019-10-28T15:27:15Z

.travis.yml

+    if: branch = master AND type IN (push)
+    dist: bionic
+    go: 1.12.x
+    script: ./scripts/travis/fuzzit.sh fuzzing


These changes don't belong to this PR. Please sync with master

Signed-off-by: radekg <[email protected]>

radekg · 2019-11-29T16:51:52Z

ping

jpkrohling

LGTM. @yurishkuro, were all your concerns addressed?

jpkrohling · 2019-12-05T09:42:30Z

I'm merging this, as I don't think there's any remaining concern.

radekg · 2019-12-05T11:11:35Z

Nice, thank you @jpkrohling.

radekg requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners October 10, 2019 17:56

yurishkuro changed the title ~~Collector tags. https://github.com/jaegertracing/jaeger/issues/1844~~ Support collector tags, similar to agent tags Oct 10, 2019

yurishkuro mentioned this pull request Oct 10, 2019

Suport for collector level tags #1844

Closed

yurishkuro reviewed Oct 10, 2019

View reviewed changes

jpkrohling reviewed Oct 11, 2019

View reviewed changes

radekg force-pushed the collector-tags branch from d6e8587 to 2410270 Compare October 11, 2019 20:33

yurishkuro mentioned this pull request Oct 13, 2019

Enable logs from all-in-one integration test #1857

Merged

burmanm and others added 6 commits October 28, 2019 10:55

Add Truncate and ReadOnly options to Badger storage backend, closes j…

1bcd99c

…aegertracing#1832 (jaegertracing#1842) Signed-off-by: Michael Burman <[email protected]> Signed-off-by: radekg <[email protected]>

Change the default value of the flag '--ingester.deadlockInterval' to…

f182e99

… 0 (jaegertracing#1868) Signed-off-by: Juraci Paixão Kröhling <[email protected]> Signed-off-by: radekg <[email protected]>

Use node 10 when building docker images (jaegertracing#1871)

02e431f

Signed-off-by: Pavol Loffay <[email protected]> Signed-off-by: radekg <[email protected]>

Use Elasticsearch 6 in xdock (jaegertracing#1872)

4ba0823

* Use Elasticsearch 6 in xdock Signed-off-by: Pavol Loffay <[email protected]> * Fix issues and use single node ES Signed-off-by: Pavol Loffay <[email protected]> Signed-off-by: radekg <[email protected]>

all-in-one setup context remove testing, add collector tags to span p…

a97f7b7

…rocess Signed-off-by: radekg <[email protected]>

format

944bebe

Signed-off-by: radekg <[email protected]>

radekg force-pushed the collector-tags branch from b9afa8e to 944bebe Compare October 28, 2019 09:58

Add rudimentary test for code coverage

263df5d

Signed-off-by: radekg <[email protected]>

make fmt neeeds to be run after git add

54c3a45

Signed-off-by: radekg <[email protected]>

yurishkuro reviewed Oct 28, 2019

View reviewed changes

radekg and others added 4 commits October 28, 2019 16:28

fix failing test

94c389e

Signed-off-by: radekg <[email protected]>

Merge branch 'master' into collector-tags

0fc7625

Merged with upstream/master

45e3828

Signed-off-by: radekg <[email protected]>

Merge branch 'master' into collector-tags

2bf33de

jpkrohling approved these changes Dec 2, 2019

View reviewed changes

jpkrohling added 2 commits December 2, 2019 14:12

Merge branch 'master' into collector-tags

2cda551

Merge branch 'master' into collector-tags

5f07676

jpkrohling merged commit 730170d into jaegertracing:master Dec 5, 2019

radekg deleted the collector-tags branch December 5, 2019 11:11

pavolloffay added this to the Release 1.16 milestone Dec 17, 2019

pavolloffay mentioned this pull request May 5, 2020

Support adding process tags in OTEL via env variable #2220

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support collector tags, similar to agent tags #1854

Support collector tags, similar to agent tags #1854

radekg commented Oct 10, 2019 •

edited by yurishkuro

Loading

yurishkuro Oct 10, 2019

radekg Oct 11, 2019 •

edited

Loading

yurishkuro Oct 10, 2019

radekg Oct 11, 2019 •

edited

Loading

jpkrohling left a comment

radekg commented Oct 11, 2019

codecov bot commented Oct 11, 2019 •

edited

Loading

yurishkuro commented Oct 13, 2019

jpkrohling commented Oct 14, 2019 •

edited

Loading

radekg commented Oct 14, 2019

objectiser commented Oct 14, 2019

radekg commented Oct 14, 2019

pavolloffay commented Oct 15, 2019

radekg commented Oct 15, 2019 •

edited

Loading

objectiser commented Oct 15, 2019

jpkrohling commented Oct 15, 2019

objectiser commented Oct 15, 2019

radekg commented Oct 15, 2019

radekg commented Oct 23, 2019

jpkrohling commented Oct 23, 2019

radekg commented Oct 24, 2019

jpkrohling commented Oct 28, 2019 •

edited

Loading

radekg commented Oct 28, 2019

yurishkuro commented Oct 28, 2019

jpkrohling commented Oct 28, 2019

yurishkuro commented Oct 28, 2019

radekg commented Oct 28, 2019

yurishkuro Oct 28, 2019

radekg commented Nov 29, 2019

jpkrohling left a comment

jpkrohling commented Dec 5, 2019

radekg commented Dec 5, 2019

Support collector tags, similar to agent tags #1854

Support collector tags, similar to agent tags #1854

Conversation

radekg commented Oct 10, 2019 • edited by yurishkuro Loading

Which problem is this PR solving?

Short description of the changes

yurishkuro Oct 10, 2019

Choose a reason for hiding this comment

radekg Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

yurishkuro Oct 10, 2019

Choose a reason for hiding this comment

radekg Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

radekg commented Oct 11, 2019

codecov bot commented Oct 11, 2019 • edited Loading

Codecov Report

yurishkuro commented Oct 13, 2019

jpkrohling commented Oct 14, 2019 • edited Loading

radekg commented Oct 14, 2019

objectiser commented Oct 14, 2019

radekg commented Oct 14, 2019

pavolloffay commented Oct 15, 2019

radekg commented Oct 15, 2019 • edited Loading

objectiser commented Oct 15, 2019

jpkrohling commented Oct 15, 2019

objectiser commented Oct 15, 2019

radekg commented Oct 15, 2019

radekg commented Oct 23, 2019

jpkrohling commented Oct 23, 2019

radekg commented Oct 24, 2019

jpkrohling commented Oct 28, 2019 • edited Loading

radekg commented Oct 28, 2019

yurishkuro commented Oct 28, 2019

jpkrohling commented Oct 28, 2019

yurishkuro commented Oct 28, 2019

radekg commented Oct 28, 2019

yurishkuro Oct 28, 2019

Choose a reason for hiding this comment

radekg commented Nov 29, 2019

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling commented Dec 5, 2019

radekg commented Dec 5, 2019

radekg commented Oct 10, 2019 •

edited by yurishkuro

Loading

radekg Oct 11, 2019 •

edited

Loading

radekg Oct 11, 2019 •

edited

Loading

codecov bot commented Oct 11, 2019 •

edited

Loading

jpkrohling commented Oct 14, 2019 •

edited

Loading

radekg commented Oct 15, 2019 •

edited

Loading

jpkrohling commented Oct 28, 2019 •

edited

Loading