Use official Apache Thrift instead of Jaeger's fork #2861
Conversation
|
Looks like more changes are required to resolve the lint checks? |
| @@ -91,7 +90,7 @@ func testHTTPHandler(t *testing.T, basePath string) { | |||
| err = resp.Body.Close() | |||
| require.NoError(t, err) | |||
| if endpoint == "/" { | |||
| objResp := &tSampling092.SamplingStrategyResponse{} | |||
| objResp := &sampling.SamplingStrategyResponse{} | |||
There was a problem hiding this comment.
What is the motivation for this change? We use v092 for backwards compatibility with some old clients that used to expect numeric enums in the JSON.
There was a problem hiding this comment.
That explains it. The 0.9.2 client code base is failing to compile because of the Thrift incompatibility issues, and I was wondering why this was using that old client.
Not sure how to move forward here: we can't use 0.9.2, as it won't compile with the Thrift we have now as dependency (0.14.1).
There was a problem hiding this comment.
What exactly is the dependency? We have a fork of 0.9.2 embedded in the repo, it wasn't supposed to interact with the main version of Thrift.
There was a problem hiding this comment.
The context changes, like here:
In the latest Thrift, this function requires a context. Should I just manually change those methods then to comply with the new signature?
There was a problem hiding this comment.
Ah, I see, so this package is just our old generated types, not the full thrift/lib. Then I'd say we drop this support, it's been several years since the clients were upgraded. It was introduced back before agent was even oss, in #61.
Would be good to get confirmation from @vprithvi that it's not being used. There used to be a metric counting requests to this old endpoint https://github.com/jaegertracing/jaeger/pull/61/files#diff-2f12a4e2fcded1e774b911087f15091aead73a569c7d67caaa8be009d521dca2R63, but I am not seeing it in the recent code.
Closes #2781 Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #2861 +/- ##
==========================================
+ Coverage 95.91% 95.93% +0.02%
==========================================
Files 223 223
Lines 9695 9697 +2
==========================================
+ Hits 9299 9303 +4
+ Misses 326 325 -1
+ Partials 70 69 -1
Continue to review full report at Codecov.
|
|
|
Closes jaegertracing#2781 Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Closes #2781
Signed-off-by: Juraci Paixão Kröhling [email protected]