Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BREAKING CHANGE] Remove deprecated Jaeger tchannel receiver #636

Merged
merged 4 commits into from
Mar 31, 2020
Merged

[BREAKING CHANGE] Remove deprecated Jaeger tchannel receiver #636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f8b974
Remove deprecated Jaeger tchannel receiver
objectiser Mar 16, 2020
dd261b6
Fix minor issues
objectiser Mar 17, 2020
eb674a3
Rebase
objectiser Mar 31, 2020
32be07b
Fix test
objectiser Mar 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 0 additions & 4 deletions examples/k8s.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ spec:
ports:
- containerPort: 55678 # Default Opencensus receiver port.
- containerPort: 55679 # ZPages endpoint.
# - containerPort: 14267 # Default Jaeger TChannel port.
# - containerPort: 14268 # Default Jaeger HTTP receiver port.
# - containerPort: 9411 # Default Zipkin receiver port.
volumeMounts:
Expand Down Expand Up @@ -138,8 +137,6 @@ spec:
port: 55678
protocol: TCP
targetPort: 55678
- name: jaeger-tchannel # Default endpoint for Jaeger TChannel receiver.
port: 14267
- name: jaeger-thrift-http # Default endpoint for Jaeger HTTP receiver.
port: 14268
- name: zipkin # Default endpoint for Zipkin receiver.
Expand Down Expand Up @@ -190,7 +187,6 @@ spec:
memory: 400Mi
ports:
- containerPort: 55678 # Default endpoint for Opencensus receiver.
- containerPort: 14267 # Default endpoint for Jaeger TChannel receiver.
- containerPort: 14268 # Default endpoint for Jaeger HTTP receiver.
- containerPort: 9411 # Default endpoint for Zipkin receiver.
- containerPort: 8888 # Default endpoint for querying metrics.
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1122,6 +1122,8 @@ github.com/uber/jaeger-lib v2.2.0+incompatible h1:MxZXOiR2JuoANZ3J6DE/U0kSFv/eJ/
github.com/uber/jaeger-lib v2.2.0+incompatible/go.mod h1:ComeNDZlWwrWnDv8aPp0Ba6+uUTzImX/AauajbLI56U=
github.com/uber/tchannel-go v1.10.0 h1:YOihLHuvkwT3nzvpgqFtexFW+pb5vD1Tz7h/bIWApgE=
github.com/uber/tchannel-go v1.10.0/go.mod h1:Rrgz1eL8kMjW/nEzZos0t+Heq0O4LhnUJVA32OvWKHo=
github.com/uber/tchannel-go v1.17.0 h1:ZqTJgBdVPzE2AzQKfIE6KJrndWaF7IW9eSOXhPJG/KY=
pjanotti marked this conversation as resolved.
Show resolved Hide resolved
github.com/uber/tchannel-go v1.17.0/go.mod h1:Rrgz1eL8kMjW/nEzZos0t+Heq0O4LhnUJVA32OvWKHo=
github.com/ugorji/go v1.1.4/go.mod h1:uQMGLiO92mf5W77hV/PUCpI3pbzQx3CRekS0kk+RGrc=
github.com/ugorji/go/codec v0.0.0-20181204163529-d75b2dcb6bc8/go.mod h1:VFNgLljTbGfSG7qAOspJ7OScBnGdDN/yBr0sguwnwf0=
github.com/ultraware/funlen v0.0.2 h1:Av96YVBwwNSe4MLR7iI/BIa3VyI7/djnto/pK3Uxbdo=
Expand Down
15 changes: 0 additions & 15 deletions receiver/jaegerreceiver/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,6 @@ func TestLoadConfig(t *testing.T) {
Endpoint: ":3456",
},
},
"thrift_tchannel": {
ReceiverSettings: configmodels.ReceiverSettings{
Endpoint: "0.0.0.0:123",
},
},
"thrift_compact": {
ReceiverSettings: configmodels.ReceiverSettings{
Endpoint: "0.0.0.0:456",
Expand Down Expand Up @@ -96,11 +91,6 @@ func TestLoadConfig(t *testing.T) {
Endpoint: defaultHTTPBindEndpoint,
},
},
"thrift_tchannel": {
ReceiverSettings: configmodels.ReceiverSettings{
Endpoint: defaultTChannelBindEndpoint,
},
},
"thrift_compact": {
ReceiverSettings: configmodels.ReceiverSettings{
Endpoint: defaultThriftCompactBindEndpoint,
Expand Down Expand Up @@ -154,11 +144,6 @@ func TestLoadConfig(t *testing.T) {
Endpoint: ":3456",
},
},
"thrift_tchannel": {
ReceiverSettings: configmodels.ReceiverSettings{
Endpoint: "0.0.0.0:123",
},
},
},
})
}
Expand Down
21 changes: 6 additions & 15 deletions receiver/jaegerreceiver/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,16 +40,14 @@ const (
// Protocol values.
protoGRPC = "grpc"
protoThriftHTTP = "thrift_http"
// TODO https://github.com/open-telemetry/opentelemetry-collector/issues/267
// Remove ThriftTChannel support.
// Deprecated, see https://github.com/open-telemetry/opentelemetry-collector/issues/267
protoThriftTChannel = "thrift_tchannel"
protoThriftBinary = "thrift_binary"
protoThriftCompact = "thrift_compact"

// Default endpoints to bind to.
defaultGRPCBindEndpoint = "localhost:14250"
defaultHTTPBindEndpoint = "localhost:14268"
defaultTChannelBindEndpoint = "localhost:14267"
defaultGRPCBindEndpoint = "localhost:14250"
defaultHTTPBindEndpoint = "localhost:14268"

defaultThriftCompactBindEndpoint = "localhost:6831"
defaultThriftBinaryBindEndpoint = "localhost:6832"
Expand Down Expand Up @@ -160,11 +158,7 @@ func (f *Factory) CreateTraceReceiver(
}

if protoTChannel != nil && protoTChannel.IsEnabled() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if someone has this in the config file? Would it silently be ignored? Perhaps there could be a warning stating that this has been removed and that gRPC should be used instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would currently complain - so fail fast. It is the same issue with the jaeger thrift exporter being removed, and the existing one renamed from jaeger_grpc to jaeger.

As there is already going to be a breaking change for the next release, my preference would be to just get this one removed as well - so cleaned up before getting to 1.x.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

related nit: could you ensure that the message says something like unknown or not supported just to be clear that it may be a known protocol but not supported (no need to distinguish what is the exact case).

var err error
config.CollectorThriftPort, err = extractPortFromEndpoint(protoTChannel.Endpoint)
if err != nil {
return nil, err
}
logger.Warn("Protocol unknown or not supported", zap.String("protocol", protoThriftTChannel))
}

if protoThriftBinary != nil && protoThriftBinary.IsEnabled() {
Expand Down Expand Up @@ -206,12 +200,11 @@ func (f *Factory) CreateTraceReceiver(
}
}

if (protoGRPC == nil && protoHTTP == nil && protoTChannel == nil && protoThriftBinary == nil && protoThriftCompact == nil) ||
if (protoGRPC == nil && protoHTTP == nil && protoThriftBinary == nil && protoThriftCompact == nil) ||
(config.CollectorGRPCPort == 0 && config.CollectorHTTPPort == 0 && config.CollectorThriftPort == 0 && config.AgentBinaryThriftPort == 0 && config.AgentCompactThriftPort == 0) {
err := fmt.Errorf("either %v, %v, %v, %v, or %v protocol endpoint with non-zero port must be enabled for %s receiver",
err := fmt.Errorf("either %v, %v, %v, or %v protocol endpoint with non-zero port must be enabled for %s receiver",
protoGRPC,
protoThriftHTTP,
protoThriftTChannel,
protoThriftCompact,
protoThriftBinary,
typeStr,
Expand Down Expand Up @@ -258,8 +251,6 @@ func defaultsForProtocol(proto string) (*receiver.SecureReceiverSettings, error)
defaultEndpoint = defaultGRPCBindEndpoint
case protoThriftHTTP:
defaultEndpoint = defaultHTTPBindEndpoint
case protoThriftTChannel:
defaultEndpoint = defaultTChannelBindEndpoint
case protoThriftBinary:
defaultEndpoint = defaultThriftBinaryBindEndpoint
case protoThriftCompact:
Expand Down
14 changes: 1 addition & 13 deletions receiver/jaegerreceiver/factory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,18 +100,6 @@ func TestCreateInvalidHTTPEndpoint(t *testing.T) {
assert.Equal(t, 14268, r.(*jReceiver).config.CollectorHTTPPort, "http port should be default")
}

func TestCreateInvalidTChannelEndpoint(t *testing.T) {
factory := Factory{}
cfg := factory.CreateDefaultConfig()
rCfg := cfg.(*Config)

rCfg.Protocols[protoThriftTChannel], _ = defaultsForProtocol(protoThriftTChannel)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the tests should be kept, describing what happens when this old option is being used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a test for unknown protocols this should include a test implicit against the one being removed.

r, err := factory.CreateTraceReceiver(context.Background(), zap.NewNop(), cfg, nil)

assert.NoError(t, err, "unexpected error creating receiver")
assert.Equal(t, 14267, r.(*jReceiver).config.CollectorThriftPort, "thrift port should be default")
}

func TestCreateInvalidThriftBinaryEndpoint(t *testing.T) {
factory := Factory{}
cfg := factory.CreateDefaultConfig()
Expand Down Expand Up @@ -172,7 +160,7 @@ func TestCreateLargePort(t *testing.T) {
cfg := factory.CreateDefaultConfig()
rCfg := cfg.(*Config)

rCfg.Protocols[protoThriftTChannel] = &receiver.SecureReceiverSettings{
rCfg.Protocols[protoThriftHTTP] = &receiver.SecureReceiverSettings{
ReceiverSettings: configmodels.ReceiverSettings{
Endpoint: "localhost:65536",
},
Expand Down
7 changes: 0 additions & 7 deletions receiver/jaegerreceiver/testdata/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ receivers:
endpoint: "localhost:9876"
thrift_http:
endpoint: ":3456"
thrift_tchannel:
endpoint: "0.0.0.0:123"
thrift_compact:
endpoint: "0.0.0.0:456"
thrift_binary:
Expand All @@ -24,7 +22,6 @@ receivers:
protocols:
grpc:
thrift_http:
thrift_tchannel:
thrift_compact:
thrift_binary:
# The following demonstrates only enabling certain protocols with defaults/overrides.
Expand All @@ -41,8 +38,6 @@ receivers:
disabled: true
thrift_http:
disabled: true
thrift_tchannel:
disabled: true
thrift_compact:
disabled: true
thrift_binary:
Expand All @@ -60,8 +55,6 @@ receivers:
endpoint: "localhost:9876"
thrift_http:
endpoint: ":3456"
thrift_tchannel:
endpoint: "0.0.0.0:123"

processors:
exampleprocessor:
Expand Down
77 changes: 7 additions & 70 deletions receiver/jaegerreceiver/trace_receiver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ import (
"github.com/jaegertracing/jaeger/thrift-gen/sampling"
"github.com/jaegertracing/jaeger/thrift-gen/zipkincore"
"github.com/uber/jaeger-lib/metrics"
"github.com/uber/tchannel-go"
"github.com/uber/tchannel-go/thrift"
"go.uber.org/zap"
"google.golang.org/grpc"

Expand Down Expand Up @@ -89,7 +87,6 @@ type jReceiver struct {
config *Configuration

grpc *grpc.Server
tchanServer *jTchannelReceiver
collectorServer *http.Server

agentSamplingManager *jSamplingConfig.SamplingManager
Expand All @@ -100,27 +97,18 @@ type jReceiver struct {
logger *zap.Logger
}

type jTchannelReceiver struct {
nextConsumer consumer.TraceConsumerOld
instanceName string

tchannel *tchannel.Channel
}

const (
defaultAgentQueueSize = 1000
defaultAgentMaxPacketSize = 65000
defaultAgentServerWorkers = 10

// Legacy metrics receiver name tag values
collectorReceiverTagValue = "jaeger-collector"
tchannelCollectorReceiverTagValue = "jaeger-tchannel-collector"
agentReceiverTagValue = "jaeger-agent"
collectorReceiverTagValue = "jaeger-collector"
agentReceiverTagValue = "jaeger-agent"

agentTransport = "agent" // This is not 100% precise since it can be either compact or binary.
collectorHTTPTransport = "collector_http"
collectorTChannelTransport = "collector_tchannel"
grpcTransport = "grpc"
agentTransport = "agent" // This is not 100% precise since it can be either compact or binary.
collectorHTTPTransport = "collector_http"
grpcTransport = "grpc"

thriftFormat = "thrift"
protobufFormat = "protobuf"
Expand All @@ -140,11 +128,7 @@ func New(
context.Background(), instanceName, agentTransport, agentReceiverTagValue),
nextConsumer: nextConsumer,
instanceName: instanceName,
tchanServer: &jTchannelReceiver{
nextConsumer: nextConsumer,
instanceName: instanceName,
},
logger: logger,
logger: logger,
}, nil
}

Expand Down Expand Up @@ -208,20 +192,6 @@ func (jr *jReceiver) collectorHTTPEnabled() bool {
return jr.config != nil && jr.config.CollectorHTTPPort > 0
}

// TODO https://github.com/open-telemetry/opentelemetry-collector/issues/267
// Remove ThriftTChannel support.
func (jr *jReceiver) collectorThriftAddr() string {
var port int
if jr.config != nil {
port = jr.config.CollectorThriftPort
}
return fmt.Sprintf(":%d", port)
}

func (jr *jReceiver) collectorThriftEnabled() bool {
return jr.config != nil && jr.config.CollectorThriftPort > 0
}

func (jr *jReceiver) Start(host component.Host) error {
jr.mu.Lock()
defer jr.mu.Unlock()
Expand Down Expand Up @@ -271,10 +241,6 @@ func (jr *jReceiver) stopTraceReceptionLocked() error {
}
jr.collectorServer = nil
}
if jr.tchanServer.tchannel != nil {
jr.tchanServer.tchannel.Close()
jr.tchanServer.tchannel = nil
}
if jr.grpc != nil {
jr.grpc.Stop()
jr.grpc = nil
Expand Down Expand Up @@ -336,17 +302,6 @@ func (jr *jReceiver) SubmitBatches(batches []*jaeger.Batch, options handler.Subm
return jbsr, err
}

func (jtr *jTchannelReceiver) SubmitBatches(thriftCtx thrift.Context, batches []*jaeger.Batch) ([]*jaeger.BatchSubmitResponse, error) {
ctx := obsreport.ReceiverContext(
thriftCtx, jtr.instanceName, collectorTChannelTransport, tchannelCollectorReceiverTagValue)
ctx = obsreport.StartTraceDataReceiveOp(ctx, jtr.instanceName, collectorTChannelTransport)

jbsr, numSpans, err := consumeTraceData(ctx, batches, jtr.nextConsumer)
obsreport.EndTraceDataReceiveOp(ctx, thriftFormat, numSpans, err)

return jbsr, err
}

var _ reporter.Reporter = (*jReceiver)(nil)
var _ api_v2.CollectorServiceServer = (*jReceiver)(nil)
var _ configmanager.ClientConfigManager = (*jReceiver)(nil)
Expand Down Expand Up @@ -476,28 +431,10 @@ func (jr *jReceiver) buildProcessor(address string, factory apacheThrift.TProtoc
}

func (jr *jReceiver) startCollector(host component.Host) error {
if !jr.collectorGRPCEnabled() && !jr.collectorHTTPEnabled() && !jr.collectorThriftEnabled() {
if !jr.collectorGRPCEnabled() && !jr.collectorHTTPEnabled() {
return nil
}

if jr.collectorThriftEnabled() {
tch, terr := tchannel.NewChannel("jaeger-collector", new(tchannel.ChannelOptions))
if terr != nil {
return fmt.Errorf("failed to create NewTChannel: %v", terr)
}

server := thrift.NewServer(tch)
server.Register(jaeger.NewTChanCollectorServer(jr.tchanServer))

taddr := jr.collectorThriftAddr()
tln, terr := net.Listen("tcp", taddr)
if terr != nil {
return fmt.Errorf("failed to bind to TChannel address %q: %v", taddr, terr)
}
tch.Serve(tln)
jr.tchanServer.tchannel = tch
}

if jr.collectorHTTPEnabled() {
// Now the collector that runs over HTTP
caddr := jr.collectorHTTPAddr()
Expand Down