Remove Thrift headers from Jaeger public headers

[BeDeBaMu]

• Adresses Remove Thrift headers from Jaeger public headers #121
  so that thrift is a private dependency of jaeger.

• Moved the minimal set of code from .h to .cpp to avoid thrift-gen headers to be exposed as part of the API.

Signed-off-by: Benoit De Backer [email protected]

[AppVeyorBot]

✅ Build jaeger-client-cpp 35 completed (commit 83c834d476 by @BeDeBaMu)

[yurishkuro]

@mdouaihy could you review this?

[BeDeBaMu]

Hi,

It’s a minimal fix that should solve the primary concern.
However, to reduce the number of exported headers (nearly 100) I’d rather add some new changes:

1. Adapt some code to avoid exposing implementation details
   a. Samplers
   b. Metrics
   c. [optional] utils/YAML.h
2. change the code layout a little bit [To be discussed, there may be constraints I am not aware of]
   a. Move the public headers under one directory include/ (keep private ones under src)
3. Modify CMakeLists in order to
   a. export only public headers,
   b. use PRIVATE for lib when applicable
   c. [bonus] improve project visualization in IDEs
4. Abstract thrift and remove it from Span, Tag, LogRecord, etc.

Benefits expected:

• from 1. & 3.a much less header exports
• from 2. easier CMakeLists code to export public headers, easier public include visibility
• from 3.b easier linkage with jaeger-client-cpp
• from 4 easier switch to another serializer (protobuf)

Do you have any particular concerns regarding these proposals?
I will insert issues later so that we can discuss them.

[mdouaihy]

@BeDeBaMu, with your change, we no longer have the thrift includes in the public headers.

However, you had to forward declare some classes as the thrift transformation is accessible through methods of the public classes.

@yurishkuro I think that the change is ok but we need to continue the work to remove any reference to thrift completely from any public object (header, classed) as suggested.

[mdouaihy]

Hi @yurishkuro, I just want you to know that @BeDeBaMu and I are from the same organisation. Before I validate, I want to check that it's not a problem for you.

[BeDeBaMu]

@yurishkuro I think that the change is ok but we need to continue the work to remove any reference to thrift completely from any public object (header, classed) as suggested.

@mdouaihy this is the purpose of my 4th item

[yurishkuro]

Any way to see how this change impacts https://github.com/envoyproxy/envoy/tree/master/examples/jaeger-native-tracing ?

[mdouaihy]

I just tested manually and it's working.

However; I noticed that the build-plugin script needs to be updated to build a version linking statically it's dependencies. I ll add an issue for that.

[yurishkuro]

does this resolve #121 ?