Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve the issue of failing to retrieve attachments in dubbo 2.7.6 and 2.7.7 #12982

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Resolve the issue of failing to retrieve attachments in dubbo 2.7.6 and 2.7.7 #12982

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0ac1bc6
Resolve the issue of failing to retrieve attachments in dubbo 2.7.6+
steverao Dec 31, 2024
3cd545e
Fix compile error
steverao Dec 31, 2024
a9e1180
Optimize codes
steverao Jan 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions ...e/src/main/java/io/opentelemetry/instrumentation/apachedubbo/v2_7/DubboHeadersGetter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,31 @@
package io.opentelemetry.instrumentation.apachedubbo.v2_7;

import io.opentelemetry.context.propagation.TextMapGetter;
import java.lang.reflect.Method;
import java.util.Map;
import java.util.Set;
import org.apache.dubbo.rpc.RpcInvocation;

enum DubboHeadersGetter implements TextMapGetter<DubboRequest> {
INSTANCE;

@Override
@SuppressWarnings("deprecation") // deprecation for dubbo 3.2.15
@SuppressWarnings("unchecked") // unchecked for 2.7.6, 2.7.7
public Iterable<String> keys(DubboRequest request) {
return request.invocation().getAttachments().keySet();
RpcInvocation invocation = request.invocation();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @laurit, is it necessary to add unit test here? I don't seem to have found any related test in current codebase, do you have any suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess here it is a wider issue, this method is only used by jaeger and open tracing baggage propagation. We don't really test this method at all in any of the TextMapGetter implementations.
Since creating the unit test isn't probably particularly hard it would be best to add it. You could start with

package io.opentelemetry.instrumentation.apachedubbo.v2_7;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.when;

import java.net.InetSocketAddress;
import java.util.Iterator;
import org.apache.dubbo.common.URL;
import org.apache.dubbo.rpc.RpcContext;
import org.apache.dubbo.rpc.RpcInvocation;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;

@ExtendWith(MockitoExtension.class)
class DubboHeadersGetterTest {

  @Mock RpcContext context;

  @Test
  void keys() {
    when(context.getUrl()).thenReturn(new URL("http", "localhost", 1));
    when(context.getRemoteAddress()).thenReturn(new InetSocketAddress(1));
    when(context.getLocalAddress()).thenReturn(new InetSocketAddress(1));

    RpcInvocation invocation = new RpcInvocation();
    invocation.setAttachment("key", "value");
    DubboRequest request = DubboRequest.create(invocation, context);

    Iterator<String> iterator = DubboHeadersGetter.INSTANCE.keys(request).iterator();
    assertThat(iterator.hasNext()).isTrue();
    assertThat(iterator.next()).isEqualTo("key");
    assertThat(iterator.hasNext()).isFalse();
  }
}

You could change the DubboHeadersGetter. keys implementation to always call getObjectAttachments when it is present. Then your test could use a mock for RpcInvocation and test could verify that the expected method was called based on the latest dep flag. That way you wouldn't need to bother with testing the 2.7.6 and 2.7.7 versions that have the issue.

Map<String, String> attachments = invocation.getAttachments();
Set<String> keys = invocation.getAttachments().keySet();
// In 2.7.6, 2.7.7, the StringToObjectMap implementation does not correctly retrieve the keySet.
if (keys.size() == 0 && "ObjectToStringMap".equals(attachments.getClass().getSimpleName())) {
Method getObjectAttachmentsMethod = null;
try {
getObjectAttachmentsMethod = invocation.getClass().getMethod("getObjectAttachments");
return ((Map<String, Object>) getObjectAttachmentsMethod.invoke(invocation)).keySet();
} catch (Exception e) {
// ignore
}
}
return keys;
}

@Override
Expand Down
Loading