Refactor agent configuration #1092

pavolloffay · 2018-09-28T08:17:14Z

Resolves: #927

agent tchannel flags moved to reporter.tchannel.*
agent reporters have to be specified explicitly b.WithReporters - if none added create fails
agent client configuration manager have to be specified explicitly b.WithClientConfigManager - if none added create fails

jpkrohling · 2018-09-28T09:49:47Z

cmd/agent/app/builder.go

@@ -100,8 +98,11 @@ type HTTPServerConfiguration struct {
 }

 // WithReporter adds auxiliary reporters.
-func (b *Builder) WithReporter(r reporter.Reporter) *Builder {
-	b.otherReporters = append(b.otherReporters, r)
+func (b *Builder) WithReporter(r reporter.ReporterBuilder) *Builder {


For multiple builders, I think this is meant to be called more than once, right?

b.WithReporter(reporterBuilderA).WithReporter(reporterBuilderB)

Wouldn't it be better to accept multiple builders at once? Like:

b.WithReporters(reporterBuilderA, reporterBuilderB)

+1, can make it a vararg

cmd/agent/app/reporter/config.go

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

cmd/all-in-one/main.go

jpkrohling · 2018-09-28T09:58:32Z

cmd/all-in-one/main.go

+			if len(tb.CollectorHostPorts) == 0 {
+				tb.CollectorHostPorts = append(tb.CollectorHostPorts, fmt.Sprintf("127.0.0.1:%d", cOpts.CollectorPort))
+			}
+		}


There's no other possible reporter builder yet, but you never know the future... Perhaps it would be better to add a default case where, to log a warning in in case the type isn't recognized.

cmd/agent/app/builder.go

yurishkuro · 2018-09-28T17:26:06Z

cmd/agent/app/builder.go

+		rs = append(rs, r)
+		switch val := rb.(type) {
+		case *tchreporter.Builder:
+			server = b.HTTPServer.GetHTTPServer(val.CollectorServiceName, r.(*tchreporter.Reporter).Channel(), b.metricsFactory)


I think this is a problem. The abstraction wasn't clear here previously, i.e. both the reporter (invoked from UDP servers) and the HTTP sampling/config endpoint were relying on the TChannel connection to the collector. If everything was wired manually in the main() then we could've cleaned it up by instantiating the connection first and passing it to both the reporter and the http config server. But with the configuration approach it was less obvious how to do that. Yet I think we should address it now since we're making a breaking change (from configuration file pov) anyway.

codecov · 2018-10-02T14:30:19Z

Codecov Report

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

@@          Coverage Diff           @@
##           master   #1092   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         142     144    +2     
  Lines        6743    6769   +26     
======================================
+ Hits         6743    6769   +26

Impacted Files	Coverage Δ
cmd/agent/app/flags.go	`100% <ø> (ø)`	⬆️
cmd/agent/app/reporter/tchannel/flags.go	`100% <100%> (ø)`
cmd/agent/app/reporter/tchannel/collector_proxy.go	`100% <100%> (ø)`
cmd/agent/app/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 fb10d4c...8af5d3e. Read the comment docs.

pavolloffay · 2018-10-02T14:49:06Z

@yurishkuro @jpkrohling PR updated. I think all concerns were addressed.

yurishkuro · 2018-10-03T19:33:05Z

cmd/agent/app/builder.go

@@ -125,33 +116,36 @@ func (b *Builder) getMetricsFactory() (metrics.Factory, error) {
 		return nil, err
 	}

-	return baseFactory.Namespace("agent", nil), nil
+	b.metricsFactory = baseFactory.Namespace("agent", nil)


Unexpected side effect. Is it necessary? Can we not pass it explicitly as before?

I'd rather do it in one way, either always pass explicitly (e.g. to CreateAgent) or change to InitMetricsFactory and never pass to member methods (but pretty much everywhere else we pass metrics and logger explicitly).

yurishkuro · 2018-10-03T19:40:11Z

docker-compose/jaeger-docker-compose.yml

@@ -25,7 +25,7 @@ services:

    jaeger-agent:
      image: jaegertracing/jaeger-agent
-      command: ["--collector.host-port=jaeger-collector:14267"]
+      command: ["--reporter.tchannel.collector.host-port=jaeger-collector:14267"]


do we want to do this as a breaking change or support the old config options as deprecated

I will try to depreciate in a way that values from old flags would be used and logged a warning.

I don't want to keep old flags just as noop. Then I think it's better to fail at init time to avoid misconfiguration.

yurishkuro · 2018-10-03T19:48:33Z

cmd/agent/main.go

@@ -50,9 +52,19 @@ func main() {

 			builder := &app.Builder{}
 			builder.InitFromViper(v)
+			tchanRep := tchannel.NewBuilder().InitFromViper(v)


what signal would this code use to instantiate grpc instead of tchannel reporter?

Not sure if I follow. Do you mean doing something similar as SPAN_STORAGE_TYPE?

We could do something similar to storage type or add all reporter flags and introduce flag --reporter.type

cmd/agent/app/builder.go

yurishkuro · 2018-10-03T19:54:29Z

cmd/agent/app/builder.go

-func (c HTTPServerConfiguration) GetHTTPServer(svc string, channel *tchannel.Channel, mFactory metrics.Factory) *http.Server {
-	mgr := httpserver.NewCollectorProxy(svc, channel, mFactory)
+func (c HTTPServerConfiguration) getHTTPServer(manager httpserver.ClientConfigManager, mFactory metrics.Factory, mBuilder *jmetrics.Builder) (*http.Server, error) {
+	if manager == nil {


we never do these checks anywhere else afaik

I have added these checks to better understand requirements. manager and reporter is mandatory. We can also change the builder to build(reporters, manager)

yurishkuro · 2018-10-03T19:59:19Z

cmd/agent/main.go

@@ -50,9 +52,19 @@ func main() {

 			builder := &app.Builder{}
 			builder.InitFromViper(v)
+			tchanRep := tchannel.NewBuilder().InitFromViper(v)
+			mFactory, err := builder.GetMetricsFactory()


CreateAgent calls this again

That is cached

yurishkuro · 2018-10-03T20:04:50Z

cmd/agent/main.go

+			if err != nil {
+				logger.Fatal("Could not create metrics", zap.Error(err))
+			}
+			r, err := tchanRep.CreateReporter(mFactory, logger)


I wonder if it would make more sense to encapsulate "tchannel collector proxy" that will either implement or at least expose both Reporter and ClientConfigManager APIs. It would make the code in main() simpler (and code itself can be unit tested), and will be easier to switch for gRPC implementation.

Right now httpserver.NewCollectorProxy() below is leaking tchannel detail outside of tchannel package.

pavolloffay · 2018-10-08T08:48:43Z

@yurishkuro could you please have a look?

yurishkuro · 2018-10-08T18:58:54Z

I'm trying to build out internal versions with this branch.

cmd/agent/app/builder.go

yurishkuro · 2018-10-12T23:50:23Z

I was able to build our internal agent with these changes, but haven't had a change to test it, esp. loading of the old configs.

yurishkuro · 2018-10-12T23:57:10Z

cmd/agent/app/builder_test.go

-    - 127.0.0.1:14269
-
-collectorServiceName: some-collector-service
-minPeers: 4


just to confirm - this is no longer supported? I thought there was a fallback with the options that might make the old config still work.

old flags are supported and deprecated https://github.com/jaegertracing/jaeger/pull/1092/files#diff-1fc7689d387f4303d6e478ca01841286R29. The new flags take precedence over old ones.

Jaeger agent is a daemon program that runs on every host and receives tracing data submitted by Jaeger client libraries. Usage: jaeger-agent [flags] jaeger-agent [command] Available Commands: help Help about any command version Print the version Flags: --collector.host-port string Deprecated; comma-separated string representing host:ports of a static list of collectors to connect to directly (e.g. when not using service discovery) --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 Deprecated; sets the timeout used when establishing new connections (default 250ms) --discovery.min-peers int Deprecated; 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") --log-level string Minimal allowed log Level. For more levels see https://github.com/uber-go/zap (default "info") --metrics-backend string Defines which metrics backend to use for metrics reporting: expvar, prometheus, none (default "prometheus") --metrics-http-route string Defines the route of HTTP endpoint for metrics backends that support scraping (default "/metrics") --processor.jaeger-binary.server-host-port string host:port for the UDP server (default ":6832") --processor.jaeger-binary.server-max-packet-size int max packet size for the UDP server (default 65000) --processor.jaeger-binary.server-queue-size int length of the queue for the UDP server (default 1000) --processor.jaeger-binary.workers int how many workers the processor should run (default 10) --processor.jaeger-compact.server-host-port string host:port for the UDP server (default ":6831") --processor.jaeger-compact.server-max-packet-size int max packet size for the UDP server (default 65000) --processor.jaeger-compact.server-queue-size int length of the queue for the UDP server (default 1000) --processor.jaeger-compact.workers int how many workers the processor should run (default 10) --processor.zipkin-compact.server-host-port string host:port for the UDP server (default ":5775") --processor.zipkin-compact.server-max-packet-size int max packet size for the UDP server (default 65000) --processor.zipkin-compact.server-queue-size int length of the queue for the UDP server (default 1000) --processor.zipkin-compact.workers int how many workers the processor should run (default 10) --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) Use "jaeger-agent [command] --help" for more information about a command.

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

pavolloffay · 2018-10-16T07:04:45Z

@yurishkuro is there anything more what should be done?

yurishkuro · 2018-10-16T15:30:06Z

We're testing it internally to make sure we can still do multi-reporter configs. Will update soon.

cc @isaachier

pavolloffay · 2018-10-29T09:40:53Z

Any news @isaachier or @yurishkuro ?

isaachier

We tested this internally and it looks good.

pavolloffay requested review from black-adder, jpkrohling, vprithvi and yurishkuro as code owners September 28, 2018 08:17

ghost assigned pavolloffay Sep 28, 2018

ghost added the review label Sep 28, 2018

pavolloffay changed the title ~~WIP: Agent flags~~ WIP: Refactor agent configuration Sep 28, 2018

jpkrohling reviewed Sep 28, 2018

View reviewed changes

yurishkuro reviewed Sep 28, 2018

View reviewed changes

cmd/agent/app/builder.go Outdated Show resolved Hide resolved

yurishkuro reviewed Sep 28, 2018

View reviewed changes

pavolloffay force-pushed the agent-flags branch from 66b2d78 to 2a11e96 Compare October 2, 2018 14:30

pavolloffay changed the title ~~WIP: Refactor agent configuration~~ Refactor agent configuration Oct 2, 2018

pavolloffay mentioned this pull request Oct 2, 2018

[agent] Make timeout for reporting configurable #1034

Merged

yurishkuro reviewed Oct 3, 2018

View reviewed changes

yurishkuro reviewed Oct 8, 2018

View reviewed changes

cmd/agent/app/builder.go Show resolved Hide resolved

yurishkuro reviewed Oct 12, 2018

View reviewed changes

pavolloffay added 9 commits October 15, 2018 10:11

Make agent reporters configurable

16c48ef

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

Add nil check

3e3c47a

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

Fix docker compose

4430db5

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

Remove redundant if

ea03533

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

Deprecate flags

5e31a0b

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

Pass metrics explicitly

d166921

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

Collector proxy

9fe3da0

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

interface collector proxy

24b18df

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

Fix all-in-one

f8876dc

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

pavolloffay added 2 commits October 15, 2018 10:11

Remove comments

d238d12

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

Use directly reporters

8af5d3e

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

pavolloffay force-pushed the agent-flags branch from af4f24f to 8af5d3e Compare October 15, 2018 08:12

isaachier approved these changes Oct 29, 2018

View reviewed changes

yurishkuro approved these changes Oct 29, 2018

View reviewed changes

yurishkuro merged commit 3bdb8e6 into jaegertracing:master Oct 29, 2018

ghost removed the review label Oct 29, 2018

yurishkuro mentioned this pull request Oct 29, 2018

[tracking] Release 1.8 #1150

Closed

7 tasks

yurishkuro added this to the 1.8 Release milestone Nov 9, 2018

dstdfx mentioned this pull request Sep 20, 2020

Expose the tuning options via expvar #1333

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor agent configuration #1092

Refactor agent configuration #1092

pavolloffay commented Sep 28, 2018 •

edited

Loading

jpkrohling Sep 28, 2018

yurishkuro Sep 28, 2018

jpkrohling Sep 28, 2018

yurishkuro Sep 28, 2018

codecov bot commented Oct 2, 2018 •

edited

Loading

pavolloffay commented Oct 2, 2018

yurishkuro Oct 3, 2018

yurishkuro Oct 3, 2018

pavolloffay Oct 4, 2018

yurishkuro Oct 3, 2018

pavolloffay Oct 4, 2018

pavolloffay Oct 5, 2018

yurishkuro Oct 3, 2018

pavolloffay Oct 4, 2018

yurishkuro Oct 3, 2018

pavolloffay Oct 4, 2018

yurishkuro Oct 3, 2018

pavolloffay commented Oct 8, 2018

yurishkuro commented Oct 8, 2018

yurishkuro commented Oct 12, 2018

yurishkuro Oct 12, 2018

pavolloffay Oct 15, 2018 •

edited

Loading

pavolloffay Oct 15, 2018

pavolloffay commented Oct 16, 2018

yurishkuro commented Oct 16, 2018

pavolloffay commented Oct 29, 2018

isaachier left a comment

Refactor agent configuration #1092

Refactor agent configuration #1092

Conversation

pavolloffay commented Sep 28, 2018 • edited Loading

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

codecov bot commented Oct 2, 2018 • edited Loading

Codecov Report

pavolloffay commented Oct 2, 2018

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

pavolloffay commented Oct 8, 2018

yurishkuro commented Oct 8, 2018

yurishkuro commented Oct 12, 2018

Choose a reason for hiding this comment

pavolloffay Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Oct 16, 2018

yurishkuro commented Oct 16, 2018

pavolloffay commented Oct 29, 2018

isaachier left a comment

Choose a reason for hiding this comment

pavolloffay commented Sep 28, 2018 •

edited

Loading

codecov bot commented Oct 2, 2018 •

edited

Loading

pavolloffay Oct 15, 2018 •

edited

Loading