[BREAKING CHANGE] Remove deprecated Jaeger tchannel receiver #636
Conversation
objectiser
requested review from
bogdandrutu,
flands,
owais,
pjanotti,
rghetia,
songy23 and
tigrannajaryan
as code owners
| @@ -158,14 +153,6 @@ func (f *Factory) CreateTraceReceiver( | |||
| } | |||
| } | |||
|
|
|||
| if protoTChannel != nil && protoTChannel.IsEnabled() { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
| cfg := factory.CreateDefaultConfig() | ||
| rCfg := cfg.(*Config) | ||
|
|
||
| rCfg.Protocols[protoThriftTChannel], _ = defaultsForProtocol(protoThriftTChannel) |
There was a problem hiding this comment.
One of the tests should be kept, describing what happens when this old option is being used.
There was a problem hiding this comment.
There is already a test for unknown protocols this should include a test implicit against the one being removed.
objectiser
changed the title
There was a problem hiding this comment.
Minor suggestions only @objectiser. Requesting changes to hold it off until we have the same receiver on Contrib. Unfortunately, in this case, it will need a different name in the config. Perhaps jaeger_legacy on contrib repo? My thought is that we can deprecate more Jaeger protocols later.
| @@ -158,14 +153,6 @@ func (f *Factory) CreateTraceReceiver( | |||
| } | |||
| } | |||
|
|
|||
| if protoTChannel != nil && protoTChannel.IsEnabled() { | |||
There was a problem hiding this comment.
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).
| cfg := factory.CreateDefaultConfig() | ||
| rCfg := cfg.(*Config) | ||
|
|
||
| rCfg.Protocols[protoThriftTChannel], _ = defaultsForProtocol(protoThriftTChannel) |
There was a problem hiding this comment.
There is already a test for unknown protocols this should include a test implicit against the one being removed.
@objectiser I think it will be easier if I tag a version of core repo now so you can then move the code as it is (minus protocols on core) to contrib. |
Codecov Report
@@ Coverage Diff @@
## master #636 +/- ##
==========================================
+ Coverage 74.47% 75.98% +1.51%
==========================================
Files 145 145
Lines 10161 10613 +452
==========================================
+ Hits 7567 8064 +497
+ Misses 2228 2167 -61
- Partials 366 382 +16
Continue to review full report at Codecov.
|
|
@objectiser contrib was updated, the receiver can be moved to contrib without, or with very minimal, changes. |
|
@pjanotti Thanks, I'll try to get that done in the next couple of days. |
Support was removed in #636 and the error will still be returned because we return an error for every unknown protocol.
Support was removed in open-telemetry#636 and the error will still be returned because we return an error for every unknown protocol.
Description:
Remove the deprecated tchannel support from the Jaeger receiver.
Link to tracking Issue:
Resolves #267
Testing:
Relevant tests were removed.
Documentation:
Removed references to tchannel from the documentation.