Move all tchannel packages to a single top level package #2112

pavolloffay · 2020-03-03T14:51:55Z

Related to #2105
Supersedes #2109

The top level package is ./tchannel. I have kept the original package structure. e.g. ./tchannel/agent/app/reporter/tchannel.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

vprithvi · 2020-03-03T14:58:12Z

cmd/agent/app/reporter/flags.go

+}
+
+// AddFlagsWithCustomTypes adds types to use message.
+func AddFlagsWithCustomTypes(types []string) func(*flag.FlagSet) {


Why do we need types?

Also - if GRPC is the only supported reporter, we can deprecate the flag so that we can remove it later

I made it backwars compatible, so in Uber's custom build the type can be added back.

Also - if GRPC is the only supported reporter, we can deprecate the flag so that we can remove it later

I would leave it for now as it is. It gives us more flexibility to extend.

I am not sure what the intent of this function, can you elaborate?

I have added a better description to the signature

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

codecov · 2020-03-03T15:50:44Z

Codecov Report

Merging #2112 into master will decrease coverage by 0.16%.
The diff coverage is 31.57%.

@@            Coverage Diff             @@
##           master    #2112      +/-   ##
==========================================
- Coverage   96.29%   96.12%   -0.17%     
==========================================
  Files         214      217       +3     
  Lines       10535    10541       +6     
==========================================
- Hits        10145    10133      -12     
- Misses        331      353      +22     
+ Partials       59       55       -4

Impacted Files	Coverage Δ
tchannel/agent/app/reporter/tchannel/flags.go	`100% <ø> (ø)`
tchannel/agent/app/reporter/tchannel/builder.go	`100% <ø> (ø)`
cmd/collector/app/builder_flags.go	`100% <ø> (ø)`	⬆️
tchannel/agent/app/reporter/tchannel/reporter.go	`100% <ø> (ø)`
...hannel/agent/app/configmanager/tchannel/manager.go	`100% <ø> (ø)`
...nel/agent/app/reporter/tchannel/collector_proxy.go	`100% <ø> (ø)`
cmd/collector/app/collector.go	`69.11% <0%> (-3.04%)`	⬇️
tchannel/collector/app/collector.go	`0% <0%> (ø)`
tchannel/collector/app/server/thrift.go	`0% <0%> (ø)`
tchannel/collector/app/flags.go	`0% <0%> (ø)`
... and 8 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 bf48066...0ec96ee. Read the comment docs.

pavolloffay · 2020-03-03T15:58:52Z

The coverage is failing bc of the new files in tchannel package I will not cover those as It will be eventually removed.

yurishkuro · 2020-03-03T18:50:27Z

cmd/agent/app/builder.go

 }
+
+// ProxyBuilder is a func which builds CollectorProxy.
+type ProxyBuilder func(map[string]string, metrics.Factory, *zap.Logger) (CollectorProxy, error)


nit: I'd prefer passing a ProxyBuilderParams struct

cmd/agent/app/reporter/grpc/builder.go

yurishkuro · 2020-03-03T18:57:24Z

cmd/agent/app/reporter/flags.go

+}
+
+// AddFlagsWithCustomTypes adds types to use message.
+func AddFlagsWithCustomTypes(types []string) func(*flag.FlagSet) {


I am not sure what the intent of this function, can you elaborate?

yurishkuro · 2020-03-03T18:58:42Z

cmd/agent/app/reporter/flags.go

+		flags.String(reporterType, string(GRPC), fmt.Sprintf("Reporter type to use e.g. %s", types))
+		if !setupcontext.IsAllInOne() {
+			flags.String(agentTagsDeprecated, "", "(deprecated) see --"+agentTags)
+			flags.String(agentTags, "", "One or more tags to be added to the Process tags of all spans passing through this agent. Ex: key1=value1,key2=${envVar:defaultValue}")


if the intent is to do something with different reporters, why do agent tags need to be here, and not in the main function?

cmd/agent/app/reporter/flags.go

yurishkuro · 2020-03-03T19:06:56Z

cmd/agent/main.go

-			cp, err := app.CreateCollectorProxy(rOpts, tchanBuilder, grpcBuilder, logger, mFactory)
+			builders := map[reporter.Type]app.ProxyBuilder{}
+			builders[reporter.GRPC] = func(m map[string]string, factory metrics.Factory, logger *zap.Logger) (app.CollectorProxy, error) {
+				return grpc.NewCollectorProxy(grpcBuilder, rOpts.AgentTags, mFactory, logger)


I would prefer to have a function in each package that matches the ProxyBuilder signature, rather than using lambdas here.

I have created proxy_builders.go in agent/app package. They cannot live in the reporter's package bc it creates cyclic imports

# github.com/jaegertracing/jaeger/cmd/agent/app/processors FAIL github.com/jaegertracing/jaeger/cmd/agent/app/processors [setup failed] import cycle not allowed in test FAIL package github.com/jaegertracing/jaeger/cmd/agent/app/processors (test) imports github.com/jaegertracing/jaeger/tchannel/agent/app/reporter/tchannel imports github.com/jaegertracing/jaeger/cmd/agent/app imports github.com/jaegertracing/jaeger/cmd/agent/app/processors GOROOT=/home/ploffay/bin/go #gosetup GOPATH=/home/ploffay/projects/golang #gosetup /home/ploffay/bin/go/bin/go test -c -o /tmp/___go_test_github_com_jaegertracing_jaeger_cmd_agent_app github.com/jaegertracing/jaeger/cmd/agent/app #gosetup FAIL github.com/jaegertracing/jaeger/cmd/agent/app [setup failed] # github.com/jaegertracing/jaeger/cmd/agent/app FAIL import cycle not allowed in test package github.com/jaegertracing/jaeger/cmd/agent/app (test) imports github.com/jaegertracing/jaeger/tchannel/agent/app/reporter imports github.com/jaegertracing/jaeger/cmd/agent/app

I wasn't suggesting importing the function type, just having functions in tch/grpc packages that match it on the signature.

There was also a suggestion to pass pass Options instead of listing the parameters. That creates cycle too.

oh, but the struct, I see...

cmd/agent/main.go

yurishkuro · 2020-03-03T19:11:41Z

tchannel/collector/app/flags.go

+const (
+	// CollectorTChannel is the default port for TChannel server for sending spans
+	CollectorTChannel = 14267
+	collectorPort     = "collector.port"


It looks like we're going back to the deprecated flag name? Maybe we can remove the old one, as it's been quite a while since we deprecated it.

The intent of this PR is to just move tchannel code to a separate package. The component itself will be removed in the follow-up PR along with flags bindings.

nvm, I though we had another option similar to

--collector.grpc-port int The gRPC port for the collector service (default 14250)

cmd/agent/app/builder.go

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

yurishkuro · 2020-03-04T13:35:52Z

cmd/agent/app/reporter/flags.go

-	flags.String(reporterType, string(GRPC), fmt.Sprintf("Reporter type to use e.g. %s, %s[%s]", string(GRPC), string(TCHANNEL), "NOTE: Deprecated since 1.16"))
+func (f Flags) AddFlags(flags *flag.FlagSet) {
+	reps := append([]string{string(GRPC)}, f.Reporters...)
+	flags.String(reporterType, string(GRPC), fmt.Sprintf("Reporter type to use e.g. %s", reps))


if all this complication just to include tchannel in the possible reporter types in the help string, I would say it's not worth it.

Yes just for the help message, I am removing it

yurishkuro · 2020-03-04T13:37:33Z

cmd/agent/app/reporter/grpc/collector_proxy.go

@@ -31,7 +31,7 @@ type ProxyBuilder struct {
 	conn     *grpc.ClientConn
 }

-// NewCollectorProxy creates ProxyBuilder
+// NewCollectorProxy creates CollectorProxyBuilder


should revert this

cmd/agent/main.go

yurishkuro · 2020-03-04T13:44:59Z

tchannel/collector/app/flags.go

+const (
+	// CollectorTChannel is the default port for TChannel server for sending spans
+	CollectorTChannel = 14267
+	collectorPort     = "collector.port"


nvm, I though we had another option similar to

--collector.grpc-port int The gRPC port for the collector service (default 14250)

cmd/all-in-one/main.go

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay · 2020-03-04T14:34:14Z

@yurishkuro comments addressed and PR updated

yurishkuro · 2020-03-04T18:15:14Z

close #2109 ?

I am waiting on our RPC team to resolve the conflict with grpc 1.26, then we can pull in this change to test, and then switch to the internal copy of the new tchannel package.

pavolloffay added 2 commits March 3, 2020 14:48

Move tchannel code to a single top level package

b5c2089

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Refactor agent proxy builder

9368885

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay requested a review from a team as a code owner March 3, 2020 14:51

pavolloffay requested a review from vprithvi March 3, 2020 14:51

pavolloffay mentioned this pull request Mar 3, 2020

Remove tchannel reporter from agent #2105

Closed

3 tasks

vprithvi reviewed Mar 3, 2020

View reviewed changes

pavolloffay added 2 commits March 3, 2020 16:14

Fix format and lint

eb5bb38

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Add empty tests

dc55d12

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

yurishkuro reviewed Mar 3, 2020

View reviewed changes

pavolloffay added 3 commits March 4, 2020 12:33

Review comments

66fa862

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Better handler custom reporter flags

dd1f02b

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Improve comments

1d8f5ea

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

yurishkuro reviewed Mar 4, 2020

View reviewed changes

Fix review comments

0ec96ee

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

yurishkuro approved these changes Mar 4, 2020

View reviewed changes

pavolloffay merged commit cc283fb into jaegertracing:master Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move all tchannel packages to a single top level package #2112

Move all tchannel packages to a single top level package #2112

pavolloffay commented Mar 3, 2020 •

edited

Loading

vprithvi Mar 3, 2020

pavolloffay Mar 3, 2020

yurishkuro Mar 3, 2020

pavolloffay Mar 4, 2020

codecov bot commented Mar 3, 2020 •

edited

Loading

pavolloffay commented Mar 3, 2020

yurishkuro Mar 3, 2020

yurishkuro Mar 3, 2020

yurishkuro Mar 3, 2020

yurishkuro Mar 3, 2020

pavolloffay Mar 4, 2020

yurishkuro Mar 4, 2020

pavolloffay Mar 4, 2020

yurishkuro Mar 4, 2020

yurishkuro Mar 3, 2020

pavolloffay Mar 4, 2020

yurishkuro Mar 4, 2020

yurishkuro Mar 4, 2020

pavolloffay Mar 4, 2020

yurishkuro Mar 4, 2020

yurishkuro Mar 4, 2020

pavolloffay commented Mar 4, 2020

yurishkuro commented Mar 4, 2020

Move all tchannel packages to a single top level package #2112

Move all tchannel packages to a single top level package #2112

Conversation

pavolloffay commented Mar 3, 2020 • 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 Mar 3, 2020 • edited Loading

Codecov Report

pavolloffay commented Mar 3, 2020

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

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 Mar 4, 2020

yurishkuro commented Mar 4, 2020

pavolloffay commented Mar 3, 2020 •

edited

Loading

codecov bot commented Mar 3, 2020 •

edited

Loading