Add non-const overload to Ice::Request::getCurrent

[alnr]

This enables C++ implementations of Ice::DispatchInterceptor to modify the incoming request before passing it to the servant. Due to the constness of Ice::Request::getCurrent, this was impossible before.

This brings C++ in line with other language implementations, where such a limitation does not exist, as far as I can tell.

Please let me know if this is something you guys are interested in merging, then I'll send the signed contributor agreement.

[zcabot]

Contributor requirements have been met.

[bentoi]

This limitation doesn't exist in other languages but we didn't really intend to allow updates of this object data members. It's just that other languages lack the concept of const objects like in C++. What's your use case for modifying the current object?

[alnr]

The use case is instrumentation of our application with OpenTracing. Incoming Ice calls have a SpanContext in their Ice::Context, which the DispatchInterceptor would deserialize. It would then start a new span (named after Ice::Current::operation), serialize the new SpanContext into the current Ice::Context and dispatch to the servant. Once that call finishes, the DispatchInterceptor finishes the Span.

Inside of the servant, the implementation could optionally decide to deserialze the current SpanContext from it's Ice::Current::ctx and start ChildSpan or pass the context to other invocations. The point is that each Ice invocation would automatically be instrumented.

With regards to your comment about not allowing the DispatchInterceptor to modify the request:
the signature of the virtual function to be overridden when implementing a DispatchInterceptor (bool dispatch(::Ice::Request& req) override) suggests otherwise (non-const reference). It's just that the only functionality Ice::Request provides is access to it's Ice::Current, which is const.

Is there a problem with allowing mutable access to the Ice::Current? I believe my change would be completely backwards-compatible.

Also: you own examples in cpp/test/Ice/interceptor/AMDInterceptorI.cpp explicitly modify the Ice::Current in the DispatchInterceptor (using const_cast) 😉

[bentoi]

Our tests are definitely not good examples 😄

We'll discuss some more your proposal... but did you consider using per-thread implicit contexts instead of modifying the dispatch context?

Your dispatch interceptor could read the SpanContext context, create a new span with the operation name and add it to the communicator's implicit per-thread context.

This way, invocations from the servant dispatch would automatically use this context instead of having to provide the context from Ice::Current and the servant implementation could retrieve it using communicator->getImplicitContext()->getContext().

[alnr]

I looked at per-thread contexts briefly, but the description threw me off somewhat:

This implicit context is sent with all invocations made via proxies created by that communicator, provided that you do not supply an explicit context with the call.

What if I get back a proxy from an Ice invocation, and then invoke a method on that proxy? Which context will be sent?

I would need to experiment to find out if this would work.

Also, implicit contexts are only sent if there is no explicit context. We occasionally use explicit contexts in our software, meaning those invocations could not be traced.

[bentoi]

The documentation should probably say "associated with" instead of "created by" that communicator. If you receive a proxy form an invocation, the implicit context associated with the proxy's communicator will be used if no explicit context is provided.

The code using explicit context will need to be fixed to initialize the explicit context with the implicit context:

Ice::Context ctx = proxy->ice_getCommunicator()->getImplicitContext()->getContext();
ctx["key"] = "value";
proxy->ice_ping(ctx);

[alnr]

I spent some more time on this. Per-thread implicit contexts + DispatchInterceptors are actually a good solution to instrument an Ice app. Thanks for that hint!

However, the change in this PR should still be merged. Reason being (beside aligning C++ with the other languages) as follows:

The dispatch interceptor reads the Tracing context from the Ice::Request::getCurrent()::ctx, starts a new tracing span, serializes the tracing context from that new span into the communicators per-thread implicit context, and finally dispatches the call to the servant. The confusion starts here: the servant might now have two competing contexts: the implicit context containing the new (correct) tracing info (available via current.adapter->getCommunicator()->getImplicitContext()) PLUS the original context (via current.ctx), which contains the outer/parent tracing info deserialized by the dispatch interceptor before.

Merging this change would allow a dispatch interceptor to remove the parent trace context from the Ice::Context before dispatching, removing that confusion.

[bentoi]

Why does your servant need to access the tracing info form the context? Can't it just retrieve the ActiveSpan setup by the dispatcher? Wouldn't it be best if your servants didn't have to deal at all with the tracing info from the Ice::Current's context or the implicit context (avoiding deserializing it again)?

[alnr]

The ActiveSpan concept does not appear in the OpenTracing C++ SDK as far as I can tell.

In all C++ OpenTracing instrumentation I have seen, spans are put as unique_ptrs on the stack, binding their lifetime to the scope (which is exactly what you want usually). There is no concept of a global/thread-local span storage.

Maybe there should be (something like a thread-local stack with the most recent (innermost) span on top of the stack), but that's not how the OpenTracing currently works.

[bentoi]

Ok I see, there's a bug about this: opentracing/opentracing-cpp#97. You could indeed implement something similar with thread specific storage but probably not that trivial.

[bernardnormier]

Note that OpenTracing is getting replaced by OpenTelemetry.

[bernardnormier]

beside aligning C++ with the other languages

Current in other languages should be read-only when read-only is supported by the language, not the other way around. The user-code should not modify Current.

Merging this change would allow a dispatch interceptor to remove the parent trace context from the Ice::Context before dispatching, removing that confusion.

I don't see a compelling argument here. Your servant code just uses the implicit per-thread context and ignores the current context - sounds pretty simple.

Also note that you have to be careful with async calls: AMI calls where the result is retrieved from a different thread and AMD dispatches where the thread supplying the result is not your dispatch thread. Having a modified Current context is not particularly helpful for these async callbacks.

Overall I suggest:

• not to change anything for Ice 3.7
• look into providing an elegant Ice - OpenTelemetry integration for Ice 4.0 (new issue/enhancement)

See also #417.