tracetest fails to compile in C++

[isaachier]

tracetest.thrift declares a recursive member in its struct. I have found the generated code fails to compile in C++. It seems Thrift does not support any sort of recursive types as far as I can tell. What is the best way to fix/address this?

[yurishkuro]

I haven't used C++ for a while, but isn't there a way to pre-declare the type so that you can declare a pointer to it? what does the code look like?

[isaachier]

I saw something called cpp_type for Thrift which could help here. Usually the solution would be to use a pointer to the type instead of the actual type. Thrift doesn't seem to directly allow that.

Example:

struct Downstream {
  //...
  Downstream* downstream;
};

[isaachier]

Actually, looking at the file it is clear to me that Downstream can probably be wrapped in a list without the last member. Not sure how to do this without breaking backwards compatibility with other clients.

[isaachier]

Original issue in Apache Thrift: apache/thrift#84.

[isaachier]

One last thought. The reason this only fails in C++ and not any other language relates to the way optional is implemented in C++. Unlike Go, optional does not generate a pointer member in C++. Instead, the generated code looks something like this:

Thrift:

struct Foo {
    2: optional Bar bar
}

C++:

typedef struct _Foo__isset {
    bool bar :1;
} _Foo_isset;

class Foo {
    Bar bar;
    _Foo__isset __isset;

    void __set_bar(const Bar& b) { bar = b; __isset.bar = true; }
};

[isaachier]

@yurishkuro this might block C++ client crossdock testing if it cannot be changed.

[yurishkuro]

can we fudge the generated files? Changing tracetest API is not impossible, but rather involved, we'd need to do it across all clients, unless the IDL change can be made compatible with the existing format.

[isaachier]

Ya that is fine with me. Might be worth noting as a "TODO." Newer versions of Thrift fix this, but not 0.9.2.

[yurishkuro]

why do you need 0.9.2? I believe you can use 0.9.3 or 0.10, they are compatible in the actual payloads, just the language APIs are different, which is not a problem for you.

[isaachier]

In that case why are we using it across the board when using jaeger-idl?

…

On Oct 16, 2017 5:42 PM, "Yuri Shkuro" ***@***.***> wrote: why do you need 0.9.2? I believe you can use 0.9.3 or 0.10, they are compatible in the actual payloads, just the language APIs are different, which is not a problem for you. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#35 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACdXeaSDl_rPNUUozDxIyg9B4KikK8O5ks5ss82zgaJpZM4P5mqF> .

[yurishkuro]

IIRC we only use 0.9.2 in Java, for legacy reasons. Go uses 0.9.3, JS/Python don't use Thrift at all (they use thriftrw).

[isaachier]

I see. Lol if I had known this before I wouldn't have bothered with downgrading to 0.9.2 and adding it to the package manager ( hunter-packages/thrift#3). I think this might make it easier on those relying on older versions of Thrift to integrate with Jaeger but I might be wrong.

…

On Oct 16, 2017 6:00 PM, "Yuri Shkuro" ***@***.***> wrote: IIRC we only use 0.9.2 in Java, for legacy reasons. Go uses 0.9.3, JS/Python don't use Thrift at all (they use thriftrw). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#35 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACdXed7yNGSYO2RFf8pGIORU4miw6zTjks5ss9IQgaJpZM4P5mqF> .

[yurishkuro]

is there a way to shadow/vendor thrift code in C++ so that it doesn't conflict with people's dependencies?

[isaachier]

Although that sounds simple, it is pretty difficult to do properly. The package manager I have been using (Hunter) is able to do this, but I'm not sure if most users will want to use it.

…

On Oct 16, 2017 6:52 PM, "Yuri Shkuro" ***@***.***> wrote: is there a way to shadow/vendor thrift code in C++ so that it doesn't conflict with people's dependencies? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#35 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACdXeaC0dabJIaeh1nJFMIyH-gHtCLUAks5ss94SgaJpZM4P5mqF> .

[isaachier]

I'm wondering how this is done in any compiled language. When linking together an executable, the linker doesn't care where the symbols come from, as long as they are resolved. Even if I make a library that included the Thrift library sources, wouldn't the linker ultimately discard the symbols that it doesn't need?

Example:
g++ -o myapp -L/path/to/thrift-0.9.2 -ljaegertracing -L/path/to/jaeger -ljaegertracing -L/path/to/thrift-0.10.0

The above case will probably drop the symbols in Thrift 0.10.0 in favor of the existing symbols in Thrift 0.9.2.

[yurishkuro]

In Java this is achieved via "shading" which rewrites the package name of the included module, e.g. instead of com.guava you would have io.jaeger.shade.com.guava in the final JAR.

In Go I've seen people simply copying the vendored code into a sub-dir, which also results in a separate package name.

Of course this approach only works if the shaded packages are not exposed via your module's public API.

[isaachier]

OK I see. Java and Go both actually define symbols with the path as a prefix/namespace. In C++ this all has to be done by hand. Unfortunately, libraries rarely do anything like that. An example of how that could be accomplished.

namespace v2 {
namespace apache {
namespace thrift {
// ...
}
}
}

More often they are actually included in a separate directory and linked with a different library.

[isaachier]

Still a problem, but fixed temporarily with this: isaachier/jaeger-client-cpp@10719ae.

[isaachier]

Just going to resolve this for now.

[ringerc]

This is different to the Thrift pull req in apache/thrift#84, since that didn't address optional self-membership.

I've opened a new Thrift bug https://issues.apache.org/jira/browse/THRIFT-4484 .

This blocks code-generation for the c++ library. It has to apply a manual patch to the generated code right now, making it hard to use other Thrift versions, automate builds more, etc.

What would the consequences of changing how this is modeled in the IDL be?

[yurishkuro]

tracetest defines a schema for cross-language integration tests of the instrumentation libraries. Not impossible to change the schema, but the usual caveats of upgrading across 5 projects

[ringerc]

@yurishkuro OK, good to know. Unfortunately it looks like zipkincore.thrift is affected too; unsure if working around that with an IDL change introduces a compatibility issue with zipkin.

Ideally Thrift would just generate sane C++...

[ringerc]

The fix to the IDL is trivial. I don't know Thrift well enough (or at all) so can't say if it will affect the wire format, but I'm guessing not?

More of a concern is whether it affects the other bindings' generated code. See ringerc#1 .

[isaachier]

@yurishkuro reminded me a while back that the Thrift wire protocol is unlikely to change at all between minor versions of Thrift. The only changes will be the headers used etc. I only use 0.9.2 for an internal Uber project. Otherwise, if a later version supports recursive types, feel free to use that version instead. As I have pointed out before, please ignore Zipkin entirely if possible. I understand that there is a method using a Thrift type as an argument, but the argument is unused and should not be needed for compilation.

[ringerc]

Good to know. The latest Thrift also chokes on this IDL and produces bad C++, though. It looks like using a by-reference member isn't enough, Thrift generates differently wacky C++ where it tries to access members of `__isset` e.g. `this->downstream->__isset.serviceName`. Looking into it.

[ringerc]

It looks like the problem IDL is confined to use by Crossdock, not the main protocol and project.

So Jaeger should probably just patch it to something Thrift doesn't choke on.

[ringerc]

This is a blocker for making the cpp-client work on a Thrift other than 0.9.2. Since Thrift 0.9.2 doesn't support openssl 1.1, it's a pretty major source of pain.

The bad IDL only appears to affect Zipkin support and Crossdock support, so the impact of fixing it even if it makes a wire protocol change will only affect users of those features.

Please?

In the mean time I've ripped out Zipkin and Crossdock support from jaeger cpp-client so I can build it on Thrift 0.11.0 and OpenSSL 1.1; see jaegertracing/jaeger-client-cpp#61 for the working branch. This is not mergeable.

[ringerc]

Ping @yurishkuro for practicality of removing the faulty optional self-referential usage from the IDL.

[isaachier]

I don't see the need to change the type. If anything, I heard more recent versions of Thrift support the recursive references.

[ringerc]

I tried it. They support the recursive references, but they generate bad code when a recursive reference is declared 'optional'.​ See above-linked Thrift bug. I guess I could try to fix Thrift, but then we'd be stuck waiting for a release, and I'd probably just go further down the rabbit hole.

[ringerc]

When I tested Thrift 0.11.0 it produced differently wrong results. I'll check. Yeah, it does, per your comments on #94.