Refactor Agent configuration

[yurishkuro]

Requirement - what kind of business use case are you trying to solve?

Enable gRPC communications between agent and collector (main issue #773).

Problem - what in Jaeger blocks you from solving the requirement?

The agent configuration app.Builder directly embeds TChannel-based reporter configuration app.reporter.tchannel.Builder, rather than making it pluggable like --processor.{format}.*** configurations. This makes the following flags into top-level options, even though they may not necessarily make sense to non-TChannel reporters:

--collector.host-port
--discovery.conn-check-timeout
--discovery.min-peers

Proposal - what do you suggest to solve the problem or improve the existing situation?

Create a nested hierarchy of reporters, e.g. to support the following config:

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

Any open questions to address

This will be a breaking change since the command line will be affected. We could potentially keep support for the old three options as "deprecated".

[isaachier]

My only issue with gRPC is how we can use a streaming protocol that we have no good way of instrumenting. From what I understand, there is no good hook for adding a context to a streaming request. If we have a solution to this, I would feel fine with this proposal.

[yurishkuro]

This issue is not about gRPC (#773 is), but about refactoring agent to allow different output transports.

[isaachier]

OK so then this is totally a non-issue.

[danehans]

@yurishkuro I would like to work on this issue.

[yurishkuro]

I am not sure if it's really a "good first issue", because it requires somewhat significant refactoring of the agent organization.

@danehans you can give it a try. I would suggest posting a summary of the planned changes here first before making them.

[danehans]

@yurishkuro thanks for the feedback. I've started digging in. If I get over my head, I'll send an update and work on a different issue. As you mention, I'll provide a summary of the changes before proceeding.

[jpkrohling]

@danehans if you get blocked, let us know.

[pavolloffay]

So new flags are

--reporter.tchannel.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)
      --reporter.tchannel.discovery.conn-check-timeout duration   sets the timeout used when establishing new connections (default 250ms)
      --reporter.tchannel.discovery.min-peers int                 if using service discovery, the min number of connections to maintain to the backend (default 3)

I am wondering whether --reporter.tchannel.collector.host-port could be renamed to --reporter.tchannel.host-port The collector seems a bit redundant there. cc @yurishkuro

[yurishkuro]

agreed

[pavolloffay]

Will remove and cut 1.8