Fix dubbo trace/span cross-process propagation

[zmapleshine]

It is inappropriate to use the Invocation for cross-process propagation. When the Dubbo Client makes an RPC call, No matter how to set up the invocation's attachement in Filter, AbstractInvoker will eventually get the attachement from the RPCContext and backfill it to the invocation.

OpenTelemetryFilter

if (kind.equals(CLIENT)) {
      context = tracer.startClientSpan(interfaceName, methodName);
      tracer.inject(context, (RpcInvocation) invocation, DubboInjectAdapter.SETTER);
    } else {
      context = tracer.startServerSpan(interfaceName, methodName, (RpcInvocation) invocation);
    }

DubboInjectAdapter

@Override
  public void set(RpcInvocation rpcInvocation, String key, String value) {
    rpcInvocation.setAttachment(key, value);
  }

AbstractInvoker

RpcInvocation invocation = (RpcInvocation) inv;
        invocation.setInvoker(this);
        if (attachment != null && attachment.size() > 0) {
            invocation.addAttachmentsIfAbsent(attachment);
        }
        Map<String, String> contextAttachments = RpcContext.getContext().getAttachments();
        if (contextAttachments != null && contextAttachments.size() != 0) {
            /**
             * invocation.addAttachmentsIfAbsent(context){@link RpcInvocation#addAttachmentsIfAbsent(Map)}should not be used here,
             * because the {@link RpcContext#setAttachment(String, String)} is passed in the Filter when the call is triggered
             * by the built-in retry mechanism of the Dubbo. The attachment to update RpcContext will no longer work, which is
             * a mistake in most cases (for example, through Filter to RpcContext output traceId and spanId and other information).
             */
            invocation.addAttachments(contextAttachments);
        }

As a Dubbo server, the attachements of the RpcContext are set in the ContextFilter, which is derived from the invocation. ,and this Filter is a high priority.

ContextFilter

RpcContext.getContext()
                .setInvoker(invoker)
                .setInvocation(invocation)
//                .setAttachments(attachments)  // merged from dubbox
                .setLocalAddress(invoker.getUrl().getHost(),
                        invoker.getUrl().getPort());

        // merged from dubbox
        // we may already added some attachments into RpcContext before this filter (e.g. in rest protocol)
        if (attachments != null) {
            if (RpcContext.getContext().getAttachments() != null) {
                RpcContext.getContext().getAttachments().putAll(attachments);
            } else {
                RpcContext.getContext().setAttachments(attachments);
            }
        }

About this issue see #3427

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ zmapleshine (f0837af, dfb5bed)

[mateuszrzeszutek]

Would it be possible to change the tests so that they verify that passing context via RpcInvocation is broken?

[tydhot]

Yes, it should choose RpcContext here, thanks!

[zmapleshine]

Would it be possible to change the tests so that they verify that passing context via RpcInvocation is broken?

Well,I think this problem is difficult to verify in general tests because it occurs inside the dubbo invoke and there is no reference value to prove that the trace is not correct when dubbo server side extract trace context.

It seems that the problem can only be discovered by looking at it manually.

[mateuszrzeszutek]

It seems that the problem can only be discovered by looking at it manually.

Too bad -- I hoped that it'd be possible to add a one-liner that does something with the RpcContext to reproduce that.

Still, thanks for fixing that! 👍

[zmapleshine]

Too bad -- I hoped that it'd be possible to add a one-liner that does something with the RpcContext to reproduce that.

I will pay attention to this issue, and if there is a better way to test it in the future, I will start a issue to discuss it.