Skip to content

Commit 2df6379

Browse files
ndimidukndimiduk
committed
PR Feedback
1 parent 1e7f931 commit 2df6379

File tree

3 files changed

+53
-66
lines changed

3 files changed

+53
-66
lines changed

‎hbase-client/src/main/java/org/apache/hadoop/hbase/client/trace/IpcClientSpanBuilder.java‎

Lines changed: 14 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,7 @@
3030
import java.util.HashMap;
3131
import java.util.Map;
3232
import java.util.function.Supplier;
33-
import org.apache.hadoop.hbase.client.AsyncConnectionImpl;
3433
import org.apache.hadoop.hbase.net.Address;
35-
import org.apache.hadoop.hbase.security.User;
3634
import org.apache.hadoop.hbase.trace.HBaseSemanticAttributes.RpcSystem;
3735
import org.apache.hadoop.hbase.trace.TraceUtil;
3836
import org.apache.yetus.audience.InterfaceAudience;
@@ -47,27 +45,14 @@ public class IpcClientSpanBuilder implements Supplier<Span> {
4745
private String name;
4846
private final Map<AttributeKey<?>, Object> attributes = new HashMap<>();
4947

50-
public IpcClientSpanBuilder(
51-
final Supplier<String> connectionStringSupplier,
52-
final Supplier<User> userSupplier
53-
) {
54-
// TODO: this constructor is a hack used by AbstractRpcClient because it does not have access
55-
// to the AsyncConnectionImpl within which it resides. Use this for now, until we come back
56-
// and plumb through the instance.
57-
ConnectionSpanBuilder.populateConnectionAttributes(attributes, connectionStringSupplier,
58-
userSupplier);
59-
}
60-
6148
@Override
6249
public Span get() {
6350
return build();
6451
}
6552

6653
public IpcClientSpanBuilder setMethodDescriptor(final Descriptors.MethodDescriptor md) {
67-
// it happens that `getFullName` returns a string in the $package.$service format required by
68-
// the otel RPC specification. Use it for now; might have to parse the value in the future.
69-
final String packageAndService = md.getService().getFullName();
70-
final String method = md.getName();
54+
final String packageAndService = getRpcPackageAndService(md);
55+
final String method = getRpcName(md);
7156
this.name = packageAndService + "/" + method;
7257
populateMethodDescriptorAttributes(attributes, md);
7358
return this;
@@ -99,12 +84,20 @@ static void populateMethodDescriptorAttributes(
9984
final Map<AttributeKey<?>, Object> attributes,
10085
final Descriptors.MethodDescriptor md
10186
) {
102-
// it happens that `getFullName` returns a string in the $package.$service format required by
103-
// the otel RPC specification. Use it for now; might have to parse the value in the future.
104-
final String packageAndService = md.getService().getFullName();
105-
final String method = md.getName();
87+
final String packageAndService = getRpcPackageAndService(md);
88+
final String method = getRpcName(md);
10689
attributes.put(RPC_SYSTEM, RpcSystem.HBASE_RPC.name());
10790
attributes.put(RPC_SERVICE, packageAndService);
10891
attributes.put(RPC_METHOD, method);
10992
}
93+
94+
private static String getRpcPackageAndService(final Descriptors.MethodDescriptor md) {
95+
// it happens that `getFullName` returns a string in the $package.$service format required by
96+
// the otel RPC specification. Use it for now; might have to parse the value in the future.
97+
return md.getService().getFullName();
98+
}
99+
100+
private static String getRpcName(final Descriptors.MethodDescriptor md) {
101+
return md.getName();
102+
}
110103
}

‎hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -393,18 +393,10 @@ private void onCallFinished(Call call, HBaseRpcController hrc, Address addr,
393393
}
394394
}
395395

396-
private User resolveUser() {
397-
try {
398-
return userProvider.getCurrent();
399-
} catch (IOException e) {
400-
return null;
401-
}
402-
}
403-
404396
private Call callMethod(final Descriptors.MethodDescriptor md, final HBaseRpcController hrc,
405397
final Message param, Message returnType, final User ticket, final Address addr,
406398
final RpcCallback<Message> callback) {
407-
Span span = new IpcClientSpanBuilder(() -> null, this::resolveUser)
399+
Span span = new IpcClientSpanBuilder()
408400
.setMethodDescriptor(md)
409401
.setRemoteAddress(addr)
410402
.build();

‎hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/AbstractTestIPC.java‎

Lines changed: 38 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -17,16 +17,16 @@
1717
*/
1818
package org.apache.hadoop.hbase.ipc;
1919

20-
import static org.apache.hadoop.hbase.ipc.TestProtobufRpcServiceImpl.SERVICE;
21-
import static org.apache.hadoop.hbase.ipc.TestProtobufRpcServiceImpl.newBlockingStub;
22-
import static org.apache.hadoop.hbase.ipc.TestProtobufRpcServiceImpl.newStub;
2320
import static org.apache.hadoop.hbase.client.trace.hamcrest.AttributesMatchers.containsEntry;
2421
import static org.apache.hadoop.hbase.client.trace.hamcrest.SpanDataMatchers.hasAttributes;
2522
import static org.apache.hadoop.hbase.client.trace.hamcrest.SpanDataMatchers.hasDuration;
2623
import static org.apache.hadoop.hbase.client.trace.hamcrest.SpanDataMatchers.hasKind;
2724
import static org.apache.hadoop.hbase.client.trace.hamcrest.SpanDataMatchers.hasName;
2825
import static org.apache.hadoop.hbase.client.trace.hamcrest.SpanDataMatchers.hasStatusWithCode;
2926
import static org.apache.hadoop.hbase.client.trace.hamcrest.SpanDataMatchers.hasTraceId;
27+
import static org.apache.hadoop.hbase.ipc.TestProtobufRpcServiceImpl.SERVICE;
28+
import static org.apache.hadoop.hbase.ipc.TestProtobufRpcServiceImpl.newBlockingStub;
29+
import static org.apache.hadoop.hbase.ipc.TestProtobufRpcServiceImpl.newStub;
3030
import static org.hamcrest.MatcherAssert.assertThat;
3131
import static org.hamcrest.Matchers.allOf;
3232
import static org.hamcrest.Matchers.everyItem;
@@ -43,7 +43,6 @@
4343
import static org.mockito.Mockito.spy;
4444
import static org.mockito.Mockito.verify;
4545
import static org.mockito.internal.verification.VerificationModeFactory.times;
46-
4746
import io.opentelemetry.api.common.AttributeKey;
4847
import io.opentelemetry.api.trace.SpanKind;
4948
import io.opentelemetry.api.trace.StatusCode;
@@ -68,17 +67,14 @@
6867
import org.apache.hadoop.hbase.util.Bytes;
6968
import org.apache.hadoop.io.compress.GzipCodec;
7069
import org.apache.hadoop.util.StringUtils;
71-
import org.apache.hbase.thirdparty.com.google.protobuf.Descriptors;
7270
import org.hamcrest.Matcher;
7371
import org.junit.Rule;
7472
import org.junit.Test;
7573
import org.slf4j.Logger;
7674
import org.slf4j.LoggerFactory;
77-
7875
import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableList;
7976
import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
8077
import org.apache.hbase.thirdparty.com.google.protobuf.ServiceException;
81-
8278
import org.apache.hadoop.hbase.shaded.ipc.protobuf.generated.TestProtos.EchoRequestProto;
8379
import org.apache.hadoop.hbase.shaded.ipc.protobuf.generated.TestProtos.EchoResponseProto;
8480
import org.apache.hadoop.hbase.shaded.ipc.protobuf.generated.TestProtos.EmptyRequestProto;
@@ -465,49 +461,51 @@ private SpanData waitSpan(Matcher<SpanData> matcher) {
465461
.orElseThrow(AssertionError::new);
466462
}
467463

468-
private static String buildIpcSpanName(Descriptors.MethodDescriptor md) {
469-
final String packageAndService = md.getService().getFullName();
470-
final String method = md.getName();
471-
return packageAndService + "/" + method;
464+
private static String buildIpcSpanName(final String packageAndService, final String methodName) {
465+
return packageAndService + "/" + methodName;
472466
}
473467

474-
private static Matcher<SpanData> buildIpcClientSpanMatcher(Descriptors.MethodDescriptor md) {
468+
private static Matcher<SpanData> buildIpcClientSpanMatcher(
469+
final String packageAndService,
470+
final String methodName
471+
) {
475472
return allOf(
476-
hasName(buildIpcSpanName(md)),
473+
hasName(buildIpcSpanName(packageAndService, methodName)),
477474
hasKind(SpanKind.CLIENT)
478475
);
479476
}
480477

481-
private static Matcher<SpanData> buildIpcServerSpanMatcher(Descriptors.MethodDescriptor md) {
478+
private static Matcher<SpanData> buildIpcServerSpanMatcher(
479+
final String packageAndService,
480+
final String methodName
481+
) {
482482
return allOf(
483-
hasName(buildIpcSpanName(md)),
483+
hasName(buildIpcSpanName(packageAndService, methodName)),
484484
hasKind(SpanKind.SERVER)
485485
);
486486
}
487487

488488
private static Matcher<SpanData> buildIpcClientSpanAttributesMatcher(
489-
Descriptors.MethodDescriptor md,
490-
InetSocketAddress isa
489+
final String packageAndService,
490+
final String methodName,
491+
final InetSocketAddress isa
491492
) {
492-
final String packageAndService = md.getService().getFullName();
493-
final String method = md.getName();
494493
return hasAttributes(allOf(
495494
containsEntry("rpc.system", "HBASE_RPC"),
496495
containsEntry("rpc.service", packageAndService),
497-
containsEntry("rpc.method", method),
496+
containsEntry("rpc.method", methodName),
498497
containsEntry("net.peer.name", isa.getHostName()),
499498
containsEntry(AttributeKey.longKey("net.peer.port"), (long) isa.getPort())));
500499
}
501500

502501
private static Matcher<SpanData> buildIpcServerSpanAttributesMatcher(
503-
Descriptors.MethodDescriptor md
502+
final String packageAndService,
503+
final String methodName
504504
) {
505-
final String packageAndService = md.getService().getFullName();
506-
final String method = md.getName();
507505
return hasAttributes(allOf(
508506
containsEntry("rpc.system", "HBASE_RPC"),
509507
containsEntry("rpc.service", packageAndService),
510-
containsEntry("rpc.method", method)));
508+
containsEntry("rpc.method", methodName)));
511509
}
512510

513511
private void assertRemoteSpan() {
@@ -528,12 +526,14 @@ public void testTracingSuccessIpc() throws IOException, ServiceException {
528526
stub.pause(null, PauseRequestProto.newBuilder().setMs(100).build());
529527
// use the ISA from the running server so that we can get the port selected.
530528
final InetSocketAddress isa = rpcServer.getListenerAddress();
531-
final Descriptors.MethodDescriptor pauseMd =
532-
SERVICE.getDescriptorForType().findMethodByName("pause");
533-
final SpanData pauseClientSpan = waitSpan(buildIpcClientSpanMatcher(pauseMd));
534-
assertThat(pauseClientSpan, buildIpcClientSpanAttributesMatcher(pauseMd, isa));
535-
final SpanData pauseServerSpan = waitSpan(buildIpcServerSpanMatcher(pauseMd));
536-
assertThat(pauseServerSpan, buildIpcServerSpanAttributesMatcher(pauseMd));
529+
final SpanData pauseClientSpan = waitSpan(buildIpcClientSpanMatcher(
530+
"hbase.test.pb.TestProtobufRpcProto", "pause"));
531+
assertThat(pauseClientSpan, buildIpcClientSpanAttributesMatcher(
532+
"hbase.test.pb.TestProtobufRpcProto", "pause", isa));
533+
final SpanData pauseServerSpan = waitSpan(buildIpcServerSpanMatcher(
534+
"hbase.test.pb.TestProtobufRpcProto", "pause"));
535+
assertThat(pauseServerSpan, buildIpcServerSpanAttributesMatcher(
536+
"hbase.test.pb.TestProtobufRpcProto", "pause"));
537537
assertRemoteSpan();
538538
assertFalse("no spans provided", traceRule.getSpans().isEmpty());
539539
assertThat(traceRule.getSpans(), everyItem(allOf(
@@ -556,12 +556,14 @@ public void testTracingErrorIpc() throws IOException {
556556
assertThrows(ServiceException.class,
557557
() -> stub.error(null, EmptyRequestProto.getDefaultInstance()));
558558
final InetSocketAddress isa = rpcServer.getListenerAddress();
559-
final Descriptors.MethodDescriptor errorMd =
560-
SERVICE.getDescriptorForType().findMethodByName("error");
561-
final SpanData errorClientSpan = waitSpan(buildIpcClientSpanMatcher(errorMd));
562-
assertThat(errorClientSpan, buildIpcClientSpanAttributesMatcher(errorMd, isa));
563-
final SpanData errorServerSpan = waitSpan(buildIpcServerSpanMatcher(errorMd));
564-
assertThat(errorServerSpan, buildIpcServerSpanAttributesMatcher(errorMd));
559+
final SpanData errorClientSpan = waitSpan(buildIpcClientSpanMatcher(
560+
"hbase.test.pb.TestProtobufRpcProto", "error"));
561+
assertThat(errorClientSpan, buildIpcClientSpanAttributesMatcher(
562+
"hbase.test.pb.TestProtobufRpcProto", "error", isa));
563+
final SpanData errorServerSpan = waitSpan(buildIpcServerSpanMatcher(
564+
"hbase.test.pb.TestProtobufRpcProto", "error"));
565+
assertThat(errorServerSpan, buildIpcServerSpanAttributesMatcher(
566+
"hbase.test.pb.TestProtobufRpcProto", "error"));
565567
assertRemoteSpan();
566568
assertFalse("no spans provided", traceRule.getSpans().isEmpty());
567569
assertThat(traceRule.getSpans(), everyItem(allOf(

0 commit comments

Comments
 (0)