Move remaining t-channel code from core to tchannel package #2306

pavolloffay · 2020-06-25T10:12:31Z

Related to #2129

This PR removes tchannel-go references from the code components. The only remaining references are in the hotrod app which will be removed in a follow-up PR.

git grep tchannel-go/
cmd/opentelemetry/go.sum:1233:github.com/uber/tchannel-go v1.10.0 h1:YOihLHuvkwT3nzvpgqFtexFW+pb5vD1Tz7h/bIWApgE=
cmd/opentelemetry/go.sum:1234:github.com/uber/tchannel-go v1.10.0/go.mod h1:Rrgz1eL8kMjW/nEzZos0t+Heq0O4LhnUJVA32OvWKHo=
cmd/opentelemetry/go.sum:1235:github.com/uber/tchannel-go v1.16.0 h1:B7dirDs15/vJJYDeoHpv3xaEUjuRZ38Rvt1qq9g7pSo=
cmd/opentelemetry/go.sum:1236:github.com/uber/tchannel-go v1.16.0/go.mod h1:Rrgz1eL8kMjW/nEzZos0t+Heq0O4LhnUJVA32OvWKHo=
examples/hotrod/services/driver/client.go:23:   "github.com/uber/tchannel-go"
examples/hotrod/services/driver/client.go:24:   "github.com/uber/tchannel-go/thrift"
examples/hotrod/services/driver/server.go:21:   "github.com/uber/tchannel-go"
examples/hotrod/services/driver/server.go:22:   "github.com/uber/tchannel-go/thrift"
examples/hotrod/services/driver/thrift-gen/driver/tchan-driver.go:10:   "github.com/uber/tchannel-go/thrift"
go.mod:75:      github.com/uber/tchannel-go v1.16.0
go.sum:436:github.com/uber/tchannel-go v1.16.0 h1:B7dirDs15/vJJYDeoHpv3xaEUjuRZ38Rvt1qq9g7pSo=
go.sum:437:github.com/uber/tchannel-go v1.16.0/go.mod h1:Rrgz1eL8kMjW/nEzZos0t+Heq0O4LhnUJVA32OvWKHo=
tchannel/agent/app/configmanager/tchannel/manager.go:21:        "github.com/uber/tchannel-go"
tchannel/agent/app/configmanager/tchannel/manager.go:22:        "github.com/uber/tchannel-go/thrift"
tchannel/agent/app/configmanager/tchannel/manager_test.go:24:   "github.com/uber/tchannel-go/thrift"
tchannel/agent/app/reporter/tchannel/builder.go:23:     tchannel "github.com/uber/tchannel-go"
tchannel/agent/app/reporter/tchannel/builder_test.go:24:        "github.com/uber/tchannel-go"
tchannel/agent/app/reporter/tchannel/reporter.go:21:    "github.com/uber/tchannel-go"
tchannel/agent/app/reporter/tchannel/reporter.go:22:    "github.com/uber/tchannel-go/thrift"
tchannel/agent/app/testutils/mock_collector.go:22:      "github.com/uber/tchannel-go"
tchannel/agent/app/testutils/mock_collector.go:23:      "github.com/uber/tchannel-go/thrift"
tchannel/agent/app/testutils/mock_collector_test.go:24: "github.com/uber/tchannel-go/thrift"
tchannel/collector/app/collector.go:18: "github.com/uber/tchannel-go"
tchannel/collector/app/handler/tchannel_handler.go:18:  "github.com/uber/tchannel-go/thrift"
tchannel/collector/app/sampling/handler.go:18:  "github.com/uber/tchannel-go/thrift"
tchannel/collector/app/server/thrift.go:21:     "github.com/uber/tchannel-go"
tchannel/collector/app/server/thrift.go:22:     "github.com/uber/tchannel-go/thrift"
tchannel/pkg/discovery/peerlistmgr/peer_list_mgr.go:25: "github.com/uber/tchannel-go"
tchannel/pkg/discovery/peerlistmgr/peer_list_mgr_test.go:27:    tchannel "github.com/uber/tchannel-go"
tchannel/pkg/discovery/peerlistmgr/peer_list_mgr_test.go:28:    "github.com/uber/tchannel-go/raw"
tchannel/pkg/discovery/peerlistmgr/peer_list_mgr_test.go:29:    "github.com/uber/tchannel-go/testutils"
thrift-gen/baggage/tchan-baggage.go:10: "github.com/uber/tchannel-go/thrift"
thrift-gen/jaeger/tchan-jaeger.go:10:   "github.com/uber/tchannel-go/thrift"
thrift-gen/sampling/tchan-sampling.go:10:       "github.com/uber/tchannel-go/thrift"
thrift-gen/zipkincore/tchan-zipkincore.go:10:   "github.com/uber/tchannel-go/thrift"

git grep tchannel/
tchannel/agent/app/configmanager/tchannel/manager_test.go:26:   "github.com/jaegertracing/jaeger/tchannel/agent/app/testutils"
tchannel/agent/app/reporter/tchannel/builder.go:27:     "github.com/jaegertracing/jaeger/tchannel/pkg/discovery/peerlistmgr"
tchannel/agent/app/reporter/tchannel/collector_proxy.go:23:     "github.com/jaegertracing/jaeger/tchannel/agent/app/configmanager/tchannel"
tchannel/agent/app/reporter/tchannel/collector_proxy_test.go:28:        "github.com/jaegertracing/jaeger/tchannel/agent/app/configmanager/tchannel"
tchannel/agent/app/reporter/tchannel/reporter.go:25:    "github.com/jaegertracing/jaeger/tchannel/pkg/discovery/peerlistmgr"
tchannel/agent/app/reporter/tchannel/reporter_test.go:27:       "github.com/jaegertracing/jaeger/tchannel/agent/app/testutils"
tchannel/collector/app/collector.go:23: "github.com/jaegertracing/jaeger/tchannel/collector/app/server"
tchannel/collector/app/server/thrift.go:27:     tHandler "github.com/jaegertracing/jaeger/tchannel/collector/app/handler"
tchannel/collector/app/server/thrift.go:28:     "github.com/jaegertracing/jaeger/tchannel/collector/app/sampling"

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

objectiser · 2020-06-25T10:19:05Z

cmd/agent/app/builder_test.go

 		grpc.AddFlags(flags)
 		reporter.AddFlags(flags)
-		app.AddFlags(flags)


Why are these flags being removed?

leftovers, not needed anymore

objectiser · 2020-06-25T10:21:38Z

cmd/agent/app/processors/thrift_processor_test.go

 	sizeF := func() int {
-		return len(collector.GetZipkinSpans())
+		return len(collector.GetJaegerBatches())


Why have these changed to Jaeger batches when in a zipkin processor test? Is zipkin format collection via the agent relevant now? If not, then we should probably change the name of the test.

Jaeger agent can ingest zipkin thrift spans https://www.jaegertracing.io/docs/1.18/deployment/#agent. However the reporter in the agent convert these spans to jaeger batches.

codecov · 2020-06-25T12:21:46Z

Codecov Report

Merging #2306 into master will decrease coverage by 0.30%.
The diff coverage is 34.69%.

@@            Coverage Diff             @@
##           master    #2306      +/-   ##
==========================================
- Coverage   96.23%   95.92%   -0.31%     
==========================================
  Files         218      219       +1     
  Lines       10692    10734      +42     
==========================================
+ Hits        10289    10297       +8     
- Misses        347      381      +34     
  Partials       56       56

Impacted Files	Coverage Δ
cmd/agent/app/testutils/mock_grpc_collector.go	`0.00% <0.00%> (ø)`
tchannel/agent/app/reporter/tchannel/builder.go	`100.00% <ø> (ø)`
tchannel/agent/app/reporter/tchannel/reporter.go	`100.00% <ø> (ø)`
tchannel/agent/app/testutils/fixture.go	`100.00% <ø> (ø)`
.../app/testutils/mock_baggage_restriction_manager.go	`100.00% <ø> (ø)`
tchannel/agent/app/testutils/mock_collector.go	`100.00% <ø> (ø)`
...annel/agent/app/testutils/mock_sampling_manager.go	`100.00% <ø> (ø)`
tchannel/collector/app/sampling/handler.go	`100.00% <ø> (ø)`
tchannel/collector/app/server/thrift.go	`0.00% <ø> (ø)`
tchannel/pkg/discovery/peerlistmgr/options.go	`100.00% <ø> (ø)`
... and 7 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 1f70cab...164a5a4. Read the comment docs.

yurishkuro · 2020-06-25T14:13:40Z

cmd/collector/app/span_processor.go

@@ -164,7 +164,7 @@ func (sp *spanProcessor) ProcessSpans(mSpans []*model.Span, options processor.Sp
 	for i, mSpan := range mSpans {
 		ok := sp.enqueueSpan(mSpan, options.SpanFormat, options.InboundTransport)
 		if !ok && sp.reportBusy {
-			return nil, tchannel.ErrServerBusy
+			return nil, fmt.Errorf("server busy")


nit: this should be a dedicated error (similar to TraceNotFound in the query service) and the gRPC handler should translate it it to gRPC-specific status code. But it's not critical as there's no special processing in the agent for this.

Actually, might be still critical for us because "busy" is understood by the service mesh

pavolloffay added 3 commits June 25, 2020 11:58

Move remaining t-channel code from core to tchannel package

27dde24

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

Fix lint

a0a289d

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

remove references to internal /tchannel/ package

164a5a4

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

pavolloffay requested a review from objectiser June 25, 2020 10:12

pavolloffay requested a review from a team as a code owner June 25, 2020 10:12

objectiser reviewed Jun 25, 2020

View reviewed changes

objectiser approved these changes Jun 25, 2020

View reviewed changes

pavolloffay merged commit ed17b1d into jaegertracing:master Jun 25, 2020

yurishkuro reviewed Jun 25, 2020

View reviewed changes

pavolloffay mentioned this pull request Jun 26, 2020

Define busy error type in processor #2314

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move remaining t-channel code from core to tchannel package #2306

Move remaining t-channel code from core to tchannel package #2306

pavolloffay commented Jun 25, 2020

objectiser Jun 25, 2020

pavolloffay Jun 25, 2020

objectiser Jun 25, 2020

pavolloffay Jun 25, 2020 •

edited

Loading

objectiser Jun 25, 2020

codecov bot commented Jun 25, 2020

yurishkuro Jun 25, 2020

yurishkuro Jun 25, 2020

pavolloffay Jun 26, 2020

Move remaining t-channel code from core to tchannel package #2306

Move remaining t-channel code from core to tchannel package #2306

Conversation

pavolloffay commented Jun 25, 2020

objectiser Jun 25, 2020

Choose a reason for hiding this comment

pavolloffay Jun 25, 2020

Choose a reason for hiding this comment

objectiser Jun 25, 2020

Choose a reason for hiding this comment

pavolloffay Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

objectiser Jun 25, 2020

Choose a reason for hiding this comment

codecov bot commented Jun 25, 2020

Codecov Report

yurishkuro Jun 25, 2020

Choose a reason for hiding this comment

yurishkuro Jun 25, 2020

Choose a reason for hiding this comment

pavolloffay Jun 26, 2020

Choose a reason for hiding this comment

pavolloffay Jun 25, 2020 •

edited

Loading