Allow agent to use a static list to connect to multiple collectors (#134) #137

yuekui2 · 2017-04-29T01:42:45Z

Fixes #134

CLAassistant · 2017-04-29T01:42:51Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

yurishkuro

thank you for sending a PR!

yurishkuro · 2017-04-29T05:13:13Z

cmd/agent/app/config.go

-	// Set this to communicate with Collector directly, without routing layer.
-	CollectorHostPort string `yaml:"collectorHostPort"`
+	// CollectorHostPorts is the static list of hostPorts for Jaeger Collector.
+	CollectorHostPorts string `yaml:"CollectorHostPorts"`


This will be a breaking change. Also, in YAML it would be more natural to declare this as []string. So I would suggest the following

keep collectorHostPort but document it as comma-separated string

change CollectorHostPorts to []string (yaml label should start with lower-case)

leave the builder binding the string flag to the original collectorHostPort

in the constructor, combing values from []string and from collectorHostPort (after splitting on ,)

Thanks for review. Just submit a new CL with the fix.

yurishkuro · 2017-05-01T21:32:19Z

cmd/agent/app/config.go

@@ -75,10 +76,13 @@ type Builder struct {
 	Processors     []ProcessorConfiguration    `yaml:"processors"`
 	SamplingServer SamplingServerConfiguration `yaml:"samplingServer"`

-	// CollectorHostPort is the hostPort for Jaeger Collector.
-	// Set this to communicate with Collector directly, without routing layer.
+	// CollectorHostPort is comma-separate string representing host:ports of


nit: comma-separated

yurishkuro · 2017-05-01T21:33:47Z

cmd/agent/app/flags.go

@@ -52,7 +52,7 @@ func (b *Builder) Bind(flags *flag.FlagSet) {
 		&b.CollectorHostPort,
 		"collector.host-port",
 		"",
-		"host:port of a single collector to connect to directly (e.g. when not using service discovery)")
+		"comma-separate string representing host:ports of a static list of collectors to connect to directly (e.g. when not using service discovery)")


nit: comma-separated

yurishkuro · 2017-05-01T21:35:13Z

cmd/agent/app/config.go

+		d := discovery.FixedDiscoverer(hostPorts)
+		notifier := &discovery.Dispatcher{}
+		b.WithDiscoverer(d).WithDiscoveryNotifier(notifier)
+	}


please add a test that verifies the merging behavior

added. During adding unittest, I realize there is potential partially overlapping case for the two sources of static collector lists, and the handling potentially be very complicated, for example, localhost:5700 vs 127.0.0.1:5700. I start to wonder that why do not we just use CollectotHostPort string, and get rid of CollectorHostPorts entirely. By this way, the logic will be a lot simpler.

yeah, I am not very happy with the duplication myself. The builder struct is designed to be yaml-friendly. We internally have a different main.go for the agent that includes some additional internal components, and a yaml-file-based configuration. And in yaml it's a lot nicer to define the addresses as a list

- collectorHostPorts: - hostA:57000 - hostB:57000 - hostC:57000

rather than comma-separated string

- collectorHostPort: hostA:57000,hostB:57000,hostC:57000

My preferred model would be to have only CollectorHostPorts []string. But I don't know how to bind it to a cmd line flag nicely.

If you want to drop the []string var and only keep the comma-separated string, I am fine with it.

Figure out the "ideal" solution can be achieved by a customized flag. Just submit a new version. Please help take a look.

coveralls · 2017-05-01T21:47:33Z

Coverage remained the same at 100.0% when pulling 89a32d1 on yuekui2:master into b916b37 on uber:master.

coveralls · 2017-05-01T23:19:08Z

Coverage remained the same at 100.0% when pulling 0b5b0f6 on yuekui2:master into b916b37 on uber:master.

yurishkuro · 2017-05-02T18:44:36Z

cmd/agent/app/flags_test.go

@@ -41,7 +42,7 @@ func TestBingFlags(t *testing.T) {
 		"-processor.jaeger-binary.workers=42",
 	})
 	assert.Equal(t, 3, len(cfg.Processors))
-	assert.Equal(t, "1.2.3.4:555", cfg.CollectorHostPort)
+	assert.Equal(t, "1.2.3.4:555,1.2.3.4:666", strings.Join(cfg.CollectorHostPorts, ","))


the test can be more structured:

assert.Equal(t, []string{"1.2.3.4:555", "1.2.3.4:666"}, cfg.CollectorHostPorts)

yurishkuro · 2017-05-02T18:50:59Z

cmd/agent/app/flags.go

-	flags.StringVar(
-		&b.CollectorHostPort,
+	flags.Var(
+		&b.CollectorHostPorts,


rather than adding staticCollectors type in config, I would define a type here, e.g. stringSliceFlag, which would take a pointer to a regular slice, e.g.

type stringSliceFlag type { slice *[]string } flags.Var( &stringSliceFlag{slice: &b.CollectorHostPorts},

This way the "flag"-ness is encapsulated in the flags file, without spilling into the config struct.

yurishkuro · 2017-05-02T18:54:10Z

cmd/agent/app/config_test.go

+
+	assert.Equal(
+		t,
+		"127.0.0.1:14267,127.0.0.1:14268,127.0.0.1:14269",


use structured comparison (see comment below)

yurishkuro · 2017-05-02T18:54:52Z

cmd/agent/app/config_test.go

+	assert.Equal(t, len(c), len(hostPorts))
+
+	hostPortsStr := strings.Join(hostPorts, ",")
+	assert.Equal(t, strings.Join(c, ","), hostPortsStr)


can compare slices directly, without string join

yurishkuro · 2017-05-02T18:55:25Z

cmd/agent/app/config.go

+// Set sets the flag value, part of the flag.Value interface.
+func (c *staticCollectors) Set(value string) error {
+	if len(*c) > 0 {
+		return errors.New("interval flag already set")


s/interval/comma-separated/

coveralls · 2017-05-02T22:31:03Z

Coverage decreased (-0.8%) to 99.174% when pulling 0d7ff12 on yuekui2:master into b916b37 on uber:master.

coveralls · 2017-05-03T00:26:38Z

Coverage remained the same at 100.0% when pulling fbf4911 on yuekui2:master into b916b37 on uber:master.

yurishkuro

overall looks good. Please rebase off master.

I think the reason the tests are failing is because of this code in crossdock/main.go:

// InitializeAgent initializes the jaeger agent.
func InitializeAgent(url string, logger *zap.Logger) services.AgentService {
        cmd := exec.Command("/bin/sh", "-c", fmt.Sprintf(agentCmd,
                "-collector.host-port=localhost:14267 -processor.zipkin-compact.server-host-port=test_driver:5775 "+

yurishkuro · 2017-05-03T01:13:35Z

cmd/agent/app/config_test.go

+
+	c, err := cfg.discoverer.Instances()
+	assert.NoError(t, err)
+	assert.Equal(t, len(c), len(hostPorts))


redundant when comparing arrays directly below

yurishkuro · 2017-05-03T01:13:53Z

cmd/agent/app/config.go

+	if len(b.CollectorHostPorts) != 0 {
+		d := discovery.FixedDiscoverer(b.CollectorHostPorts)
+		notifier := &discovery.Dispatcher{}
+		b.WithDiscoverer(d).WithDiscoveryNotifier(notifier)


nit: b = b.With...

yurishkuro · 2017-05-03T01:15:14Z

cmd/agent/app/flags_test.go

+	err = flags.Parse([]string{
+		"-collector.host-port=1.2.3.4:555,1.2.3.4:666",
+	})
+	assert.NotNil(t, err)


not sure this is needed

was for improving code coverage. Removed.

coveralls · 2017-05-05T01:27:01Z

Coverage decreased (-0.04%) to 99.958% when pulling 05482fb on yuekui2:master into b916b37 on uber:master.

yurishkuro · 2017-05-05T02:49:58Z

Please check where the coverage goes down, we want to keep at 100%.

Also, if you haven't, please sign the contributor license.

…aegertracing#134)

coveralls · 2017-05-05T17:38:27Z

Coverage remained the same at 100.0% when pulling e03fd15 on yuekui2:master into b916b37 on uber:master.

coveralls · 2017-05-05T17:47:43Z

Coverage remained the same at 100.0% when pulling 0463f7b on yuekui2:master into f209812 on uber:master.

black-adder · 2017-05-05T18:47:50Z

cmd/agent/app/config.go

 	discoveryMgr, err := b.enableDiscovery(b.channel, logger)
 	if err != nil {
 		return nil, errors.Wrap(err, "cannot enable service discovery")
 	}
 	var clientOpts *tchannelThrift.ClientOptions
-	if discoveryMgr == nil && b.CollectorHostPort != "" {
-		clientOpts = &tchannelThrift.ClientOptions{HostPort: b.CollectorHostPort}


we still need tchannelThrift client option if collectorhosts are static. This is the reason why the build is failing.

looks like a bug elsewhere, because we don't need clientOptions when using the real discovery system, so it should've worked by using fixed discoverer.

is the failure possible to repro on dev box? How should I proceed?
Thx.

@yurishkuro we don't need clientOptions when using the real discovery system because internally we use a different version of tchannel that binds to our internal discovery system. Here, we need to pass in the static port or else tchannel can't find collector.

@black-adder but that's what attaching a static discovery does, it updates the peer list afterwards.

I was wrong, there was nothing wrong with this... dunno how I came to that conclusion.

coveralls · 2017-05-08T20:37:04Z

Coverage remained the same at 100.0% when pulling a2fa6bd on yuekui2:master into 4a63496 on uber:master.

coveralls · 2017-05-08T22:11:57Z

Coverage remained the same at 100.0% when pulling 5a504a8 on yuekui2:master into 4dc8b7f on uber:master.

black-adder · 2017-05-08T22:15:18Z

sign the cla and we can land this

yuekui2 · 2017-05-09T00:19:50Z

I had signed a while ago. Somehow, this page always shows that I have not. When I click in the Details, it shows I have signed.

…aegertracing#134) (jaegertracing#137)

yurishkuro changed the title ~~issue #134 Allow agent to use a static list to connect to multiple co…~~ Allow agent to use a static list to connect to multiple collectors (#134) Apr 29, 2017

yurishkuro reviewed Apr 29, 2017

View reviewed changes

yurishkuro reviewed May 1, 2017

View reviewed changes

yurishkuro reviewed May 2, 2017

View reviewed changes

yurishkuro reviewed May 3, 2017

View reviewed changes

Allow agent to use a static list to connect to multiple collectors (j…

0463f7b

…aegertracing#134)

yuekui2 force-pushed the master branch from e03fd15 to 0463f7b Compare May 5, 2017 17:30

black-adder reviewed May 5, 2017

View reviewed changes

Merge branch 'master' into master

a2fa6bd

yurishkuro mentioned this pull request May 8, 2017

Provide docker image for agent, collector, and query service #152

Closed

4 tasks

Merge branch 'master' into master

5a504a8

black-adder approved these changes May 9, 2017

View reviewed changes

black-adder merged commit 3a42e0e into jaegertracing:master May 9, 2017

ideepika pushed a commit to ideepika/jaeger that referenced this pull request Oct 22, 2017

Allow agent to use a static list to connect to multiple collectors (j…

de466b6

…aegertracing#134) (jaegertracing#137)

Allow agent to use a static list to connect to multiple collectors (#134) #137

Allow agent to use a static list to connect to multiple collectors (#134) #137

Conversation

yuekui2 commented Apr 29, 2017 • edited by yurishkuro

CLAassistant commented Apr 29, 2017 • edited

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 1, 2017

coveralls commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 2, 2017

coveralls commented May 3, 2017

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 5, 2017

yurishkuro commented May 5, 2017

coveralls commented May 5, 2017

coveralls commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 8, 2017

coveralls commented May 8, 2017

black-adder commented May 8, 2017

yuekui2 commented May 9, 2017

yuekui2 commented Apr 29, 2017 •

edited by yurishkuro

CLAassistant commented Apr 29, 2017 •

edited