[agent] Make timeout for reporting configurable

[gouthamve]

So, this is less a fix for #1026 but more to kick off the discussion around: #927

While the original proposal has:

reporters:
    - kind: tchannel
      # tchannel-specific parameters, e.g.
      collector:
        hostPort: ...
      discovery:
        minPeers: 10
    - kind: grps
      # grpc-specific parameters

I kind of like:

tchannel:
# tchannel-config

grpc:
# grpc-config

I took a stab at implementing my version and turns out it's super straight forward and is very little changed code. But if the preference is to have the originally proposed format, I'll make the changes. But now I'm curious, can we 2 different tchannel reporters?

Note: This will be a breaking change.

[codecov]

Codecov Report

Merging #1034 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1034   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         159     159           
  Lines        7126    7136   +10     
======================================
+ Hits         7126    7136   +10

Impacted Files	Coverage Δ
cmd/agent/app/reporter/tchannel/flags.go	100% <100%> (ø)	⬆️
cmd/agent/app/reporter/tchannel/builder.go	100% <100%> (ø)	⬆️
cmd/agent/app/reporter/tchannel/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 9f8d324...34d298d. Read the comment docs.

[yurishkuro]

Thanks for looking into this one.

can we 2 different tchannel reporters

We have some deployments at Uber where we're using two reporters of the same type (well, only tchannel reporter is supported anyway today) to send the data to two locations. However, it is done using the otherReporters feature:

jaeger/cmd/agent/app/builder.go

Lines 103 to 106 in 0c86682

	func (b *Builder) WithReporter(r reporter.Reporter) *Builder {
	b.otherReporters = append(b.otherReporters, r)
	return b
	}

I believe we have a custom config file format that we parse in a custom main. We debated that we could achieve that functionality by forwarding spans to a dedicated splitter, but that seems like more complicated deployment vs. a small code change in the agent. Ideally, we would like to preserve that forking functionality in the agent.

Have you looked into going with the list approach as proposed? Another alternative is to use nested union-like config, instead of kind: tchannel qualifier, e.g.

  reporters:
    - collectorHostPort: ...
      tchannel:
        # tchannel-specific parameters
        discovery:
          minPeers: 10
    - collectorHostPort: ...
      grpc:
        # grpc-specific parameters

[gouthamve]

I think this nested union-like config approach better :) I'll make the changes.

[yurishkuro]

The tricky part is to decide how cli flags will support a list. If that proves to be too ugly, we could draw a line and say we only support one reporter by default. At Uber we can continue using the custom config and main for multiple reporters.

[gouthamve]

Yeah, spent an hour+ trying to make the flags and config match up and not fail tests, couldn't get it to work.

Prefer if there is only reporter being supported.

[yurishkuro]

let's go with just one reporter, as long as programmatically we can add more (as we can today). Could you post how the CLI / config will look ?

[gouthamve]

➜  jaeger git:(add-agent-timeout) ✗ ./agent --help
Jaeger agent is a daemon program that runs on every host and receives tracing data submitted by Jaeger client libraries.

.....

Flags:
      --collector.host-port string                            comma-separated string representing host:ports of a static list of collectors to connect to directly (e.g. when not using service discovery)
      --collector.timeout duration                            sets the timeout used when establishing reporting spans (default 1s)
      --config-file string                                    Configuration file in JSON, TOML, YAML, HCL, or Java properties formats (default none). See spf13/viper for precedence.
      --discovery.conn-check-timeout duration                 sets the timeout used when establishing new connections (default 250ms)
      --discovery.min-peers int                               if using service discovery, the min number of connections to maintain to the backend (default 3)
  -h, --help                                                  help for jaeger-agent
      --http-server.host-port string                          host:port of the http server (e.g. for /sampling point and /baggage endpoint) (default ":5778")
.....

So, now this just adds a simple --collector.timeout flag. But I can make the flags to be: --collector.tchannel.* if you want.

This is how the config will look like:

jaeger/cmd/agent/app/builder_test.go

Lines 55 to 61 in 66ed190

	tchannel:
	collectorHostPorts:
	- 127.0.0.1:14267
	- 127.0.0.1:14268
	- 127.0.0.1:14269
	minPeers: 4
	collectorServiceName: some-collector-service

[pavolloffay]

The next week I will have time to look at #927

[gouthamve]

I'm running this in prod for the past 10 days and everything works great, we used to drop a ton of batches due to timeouts, but right now those occurrences are super rare when running with 5s timeout.

[pavolloffay]

@gouthamve sorry for a longer delay. If i understand it correctly tchannel flags are prefixed with tchannel.

If we cannot make it work like it's in #1034 (comment). I think this could go in as there are no other options. cc @yurishkuro

[gouthamve]

They are not prefixed right now. Changing flags is a breaking change. I could do it if that is preferred though.

[pavolloffay]

@gouthamve #1092 addresses agent refactoring. Once that is in this will have to be rebased.

[pavolloffay]

@gouthamve could you please rebase this on the current master? We have refactored agent flags now it should be easy to add new ones.

[gouthamve]

@pavolloffay Rebased :) I'll run this internally and report on how it performs.

[gouthamve]

https://github.com/jaegertracing/jaeger/blob/master/cmd/agent/app/reporter/tchannel/flags.go#L82-L83

would always overwrite this:
https://github.com/jaegertracing/jaeger/blob/master/cmd/agent/app/reporter/tchannel/flags.go#L70-L77
which means people using the deprecated flags are essentially getting the wrong values.

Fix is in the last commit. It wasn't caught before as flag values from the previous iteration are carried forward to the next iteration.

[gouthamve]

Hmm, the test failure looks like a flake.

[pavolloffay]

@gouthamve are you also fixing #1192 in this PR?

[gouthamve]

Yes, I need to re-init the flags for my tests to pass

[pavolloffay]

LGTM, thanks also for fixing the broken flags. A bonus would be a test verifying the timeout.

@yurishkuro could you please also have a look? I might lack some tchannel exprience.

[yurishkuro]

lgtm overall, a couple minor nits

[yurishkuro]

nit: remove blank line

[yurishkuro]

Strictly speaking this does not solve the overriding problem if the manually provided value was the same as the default value, but it's still an improvement. The key thing is that we just need to remove the legacy flags soon.

[gouthamve]

Rebased and feedback incorporated

[yurishkuro]

Thanks!