Thrift IDL not in main repository, no build support

[ringerc]

In the jaeger cpp-client source dir:

$ find . -name \*.thrift
$

and a git grep emitZipkinBatch finds only generated output from Thrift + references in UDPClient.h, UDPClientTest.cpp, and MockAgent.h.

Where are the Thrift sources to re-generate what's in thrift-gen ?

It's necessary for #38 to work since the local Thrift may not be the same version as the Thrift compiler used to check in the sources. Plus it's not great to carry generated sources in git when you can avoid it.

[isaachier]

OK so the Thrift schema files are shared among many language bindings. Hence, they are actually in a submodule idl. I'm assuming git grep is skipping submodules. You can regenerate using the Makefile in that directory. I agree I am not a fan of the use of version 0.9.2, but this has more to do with Uber's internal use than anything else.

[ringerc]

@isaachier Aah, that's git's lovely default of silently ignoring submodules biting me. Thanks for the pointer. I'll look. Guess I'll need to glue that into the CMake build.

[ringerc]

For other readers that means

git submodule init
git submodule update

then you'll find that idl/ is populated with origin https://github.com/uber/jaeger-idl.git.

Surprising it's in /uber/, but whatever.

Will look at plugging it into the cmake build later, out of time now.

[isaachier]

I don't think you need to plug in to build at all (I didn't for a reason). Just regenerate a move to your thrift-gen directory.

[ringerc]

On 29 January 2018 at 02:47, isaachier ***@***.***> wrote: I don't think you need to plug in to build at all (I didn't for a reason). Just regenerate a move to your thrift-gen directory. That's fine for an ad-hoc local build. But for packaging and deployment it

should be done the same way every time, in the CMakeFile. Ideally the generated sources shouldn't be in git at all, just the IDL to produce them, but that's not so practical when the IDL is in another module. It's also not user friendly to have to manually fetch some IDL, generate some sources, move them across, etc when you do a build. Especially if undocumented. The more of these sorts of corner cases there are the harder it is to get someone to use a tool, contribute to it, etc. Consider a typical workflow like this: - Fetch commit from git, with any submodules - "make dist" a tarball and gpg sign it, annotating it with the commit id or tag - (offline) generate srpm with .spec file and sources - (offline) build the srpm on target platforms, using local repositories of signed RPMs for dependencies - (offline) sign the rpms - Push resulting .rpm and debuginfo into repository for testing - Run smoketests in CI - Push resulting rpms to public repository for use by other builds/tools/users "make dist" needs to know how to generate the IDL. For users to be able to compile the software the way they'd usually expect to, so does "make". A CMake check for Thrift >= 0.9.2 would ensure there's a useful error, at least. And while I'm prototyping I can do all this stuff manually. But for deployment in core infrastructure in organisations that have regulatory requirements to satisfy or aren't ready to jump fully on the container bandwagon yet, that's not going to work out. Per my comment on the other open issue, I guess it's a target audience / dev style thing. What's your target audience, and do you want to deal with users who can't chase the bleeding edge and do everything online? I'd argue you should, but I obviously have a vested interest. But also, don't you want to be able to see full request lifecycles including things like where they spent time waiting on locks in the DB, what request latencies were like, etc? 'cos those projects work a bit more conservatively.

…

-- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services

[isaachier]

Regarding the Thrift sources, the idl directory should never have to be used. The fact that we use Thrift 0.9.2 does force users of more recent versions to manually generate the idl files. If they had the "right" version, they wouldn't need to know about it.

The target audience is a good question and followed up there

[ringerc]

I just tried to build with the latest Thrift in Rawhide

$ thrift --version
Thrift version 0.10.0

and it fails too:

/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/Agent.cpp: In member function ‘uint32_t jaegertracing::agent::thrift::Agent_emitZipkinBatch_args::write(apache::thrift::protocol::TProtocol*) const’:
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/Agent.cpp:70:10: error: ‘class apache::thrift::protocol::TProtocol’ has no member named ‘incrementRecursionDepth’; did you mean ‘incrementInputRecursionDepth’?
   oprot->incrementRecursionDepth();
          ^~~~~~~~~~~~~~~~~~~~~~~

since it looks like the Thrift folks renamed the member.

So it appears that jaeger cpp-client probably currently works with thrift [0.9.2,0.9.3] only.

I strongly argue that committing generated files from IDL is bad practice, especially where the result of code-generation from the IDL doesn't follow a strong specification and isn't guaranteed to be compatible across versions. It doesn't look to me like Thrift generated code is meant to be used with a different library version.

It'd be better to generate the code from the IDL, per something like https://github.com/snikulov/cmake-modules/blob/master/FindThrift.cmake / https://github.com/apache/parquet-cpp/blob/master/cmake_modules/FindThrift.cmake where a THRIFT_COMPILER is exposed.

[ringerc]

Ok, Thrift installed with

git clone //git-wip-us.apache.org/repos/asf/thrift.git
cd thrift
git checkout 0.9.2
./bootstrap.sh
./configure --with-cpp --with-java=no --with-python=no --with-lua=no --with-perl=no --enable-shared=yes --enable-static=no --enable-tutorial=no --with-qt4=no
make -s

[ringerc]

@isaachier I did a manual rebuild of the IDL with the existing Thrift 0.9.2, to see if I could reproduce the current sources. I'm then going to go on to make sure it works with any suitable Thrift.

But the generated sources do not match what's in the Jaeger cpp-client tree. It looks like the tracetest_types and zipkincore_constants sources have produced different sources. I have no way to tell if these are local changes made manually to the generated code or if they're a generation issue since they're in the first commit.

The diff can be seen in ringerc@819eff4 ; I pushed a branch with it for convenient viewing.

Were these changes made locally by hand?

I'm using the same Thrift version as the IDL scripts use, and the Makefile in idl/ doesn't pass extra arguments. Looking at the diff I'm inclined to think "local changes" ?

[ringerc]

Causes build failures:

$ make
[  2%] Built target testutils
[  2%] Building CXX object CMakeFiles/jaegertracing.dir/src/jaegertracing/thrift-gen/TracedService.cpp.o
In file included from /home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/TracedService.h:11:0,
                 from /home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/TracedService.cpp:7:
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/tracetest_types.h:64:14: error: field ‘downstream’ has incomplete type ‘jaegertracing::crossdock::thrift::Downstream’
   Downstream downstream;
              ^~~~~~~~~~
In file included from /home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/TracedService.h:11:0,
                 from /home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/TracedService.cpp:7:
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/tracetest_types.h:47:7: note: definition of ‘class jaegertracing::crossdock::thrift::Downstream’ is not complete until the closing brace
 class Downstream {
       ^~~~~~~~~~
In file included from /home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/TracedService.h:11:0,
                 from /home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/TracedService.cpp:7:
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/tracetest_types.h:280:17: error: field ‘downstream’ has incomplete type ‘jaegertracing::crossdock::thrift::TraceResponse’
   TraceResponse downstream;
                 ^~~~~~~~~~
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/tracetest_types.h:267:7: note: definition of ‘class jaegertracing::crossdock::thrift::TraceResponse’ is not complete until the closing brace
 class TraceResponse {
       ^~~~~~~~~~~~~
CMakeFiles/jaegertracing.dir/build.make:1694: recipe for target 'CMakeFiles/jaegertracing.dir/src/jaegertracing/thrift-gen/TracedService.cpp.o' failed

which looks to be down to

@@ -56,7 +61,9 @@ class Downstream {
   std::string host;
   std::string port;
   Transport::type transport;
-  boost::shared_ptr<Downstream> downstream;
+  Downstream downstream;

If I try building with Thrift 0.9.1, thrift generation fails with

[ERROR:/home/craig/projects/2Q/opentracing-jaeger-cpp-client/idl/thrift/crossdock/tracetest.thrift:28] (last token was 'Downstream')
Type "Downstream" has not been defined.

so it looks like Thrift is having issues with a self-reference of some sort.

If the IDL cannot be regenerated it'll be hard to keep up with changes to the protocol.

BTW, shouldn't the IDL be part of the opentracing project? Or am I missing how all this works?

[ringerc]

Same issue if I use Thrift 0.10.0

In file included from /home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/TracedService.h:12:0,
                 from /home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/TracedService.cpp:7:
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/tracetest_types.h:62:14: error: field ‘downstream’ has incomplete type ‘jaegertracing::crossdock::thrift::Downstream’
   Downstream downstream;
              ^~~~~~~~~~
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/tracetest_types.h:48:7: note: definition of ‘class jaegertracing::crossdock::thrift::Downstream’ is not complete until the closing brace
 class Downstream : public virtual ::apache::thrift::TBase {
       ^~~~~~~~~~
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/tracetest_types.h:290:17: error: field ‘downstream’ has incomplete type ‘jaegertracing::crossdock::thrift::TraceResponse’
   TraceResponse downstream;
                 ^~~~~~~~~~
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/tracetest_types.h:280:7: note: definition of ‘class jaegertracing::crossdock::thrift::TraceResponse’ is not complete until the closing brace
 class TraceResponse : public virtual ::apache::thrift::TBase {
       ^~~~~~~~~~~~~
CMakeFiles/jaegertracing.dir/build.make:1694: recipe for target 'CMakeFiles/jaegertracing.dir/src/jaegertracing/thrift-gen/TracedService.cpp.o' failed

[isaachier]

Zipkin isn't used in C++ client. The tracetest changes are indeed a real issue and one reason I cannot foresee making the Thrift generation part of the standard build. See this issue and ensuing discussion: jaegertracing/jaeger-idl#35. I had to painstakingly fix the generated code by hand to avoid a serious issue in the schema definition.

[ringerc]

@isaachier Thanks for the reference, I never would've found that.

Seems like a horrible decision in Thrift about how to translate the IDL to C++. __isset? Ew.

std::optional in C++17 would help but .. well, c++17. Pity they didn't just import one of the older implementations of that concept as a thrift::optional instead of their separate member __isset hack.

The tests they added in apache/thrift#84 cover various self-referential forms other than optional self-inclusion.

Looking for a Thrift bug.

Seems like it'd be hard to fix this without changing the opentracing IDL. But maybe that's what should happen.

[ringerc]

Didn't find a Thrift bug so created https://issues.apache.org/jira/browse/THRIFT-4484

[ringerc]

"one of" the reasons? Other known issues?

[ringerc]

@isaachier I think I found a fix for the IDL issue; ringerc/jaeger-idl#1 .

[ringerc]

I'm abandoning making it work on different Thrift versions for now. Thrift 0.11.0 and newwer build with std::shared_ptr instead of boost::shared_ptr unless you define -DFORCE_BOOST_SMART_PTR. So we'd have to detect if it's a newer Thrift and (if even possible) how it was built, and switch to std::shared_ptr using a namespace hack like Thrift does in its lib/cpp/src/thrift/stdcxx.h.

This goes in the too-hard basket at this point, given that the IDL is also busted per jaegertracing/jaeger-idl#35 and even Thrift 0.11.0 generates bad code for the fix using by-reference inclusion. I'll push a branch with my WIP, but unless someone has more time/care than me, it looks like the dependency on 0.9.2-0.9.3 stays.

[isaachier]

Resolved in latest release with #94.

[ringerc]

This still requires a patch file to be applied by the build on top of the generated Thrift IDL right? A patch that's probably specific to the Thrift version used?

Not a complaint in any way, just for clarity in terms of how it's resolved.