HBASE-26521 Name RPC spans as `$package.$service/$method` by ndimiduk · Pull Request #4024 · apache/hbase

ndimiduk · 2022-01-12T21:06:04Z

Bring us into basic conformance with https://github.com/open-telemetry/opentelemetry-specification/blob/3e380e249f60c3a5f68746f5e84d10195ba41a79/specification/trace/semantic_conventions/rpc.md

Apache9 · 2022-01-20T02:12:56Z

+ attributes.put(RPC_METHOD, method);
+ }
+
+ private static String getRpcPackageAndService(final Descriptors.MethodDescriptor md) {


We could move this method to a util class and also let the IpcServerSpanBuilder to call this method too? So in the future if we want to change the implementation, we could fix them both at the same time.

I'm generally against "util" classes. They indicate to me a lack of proper object/interface design and separation of concerns. I also prefer that test code reuse as little of the implementation code as is necessary -- and no more. The reason being that test code usually needs to test output behaviors. When test code and implementation share the same code for computing those behaviors, it nullifies the point.
Let me see what makes sense for these methods.

I follow this strategy in the latest patch. IpcServerSpanBuilder has the details of how these tracing variables are extracted from the MethodDescriptor instance. The test uses constant values rather than looking them up from some instance. I think this makes for a more robust test.

But IpcServerSpanBuilder is not for testing only? It is what we need to use at server side? I can not follow...

I misunderstood you. The latest commit consolidates the implementation for both IpcClientSpanBuilder and IpcServerSpanBuilder into IpcClientSpanBuilder.

Apache9 · 2022-01-20T02:13:02Z

+ return md.getService().getFullName();
+ }
+
+ private static String getRpcName(final Descriptors.MethodDescriptor md) {


Apache9 · 2022-01-20T02:16:05Z

+ return new FeatureMatcher<SpanData, T>(matcher, "SpanData having duration that ", "duration") {
+ @Override
+ @SuppressWarnings("unchecked")
+ protected T featureValueOf(SpanData item) {


Looking at the implementation of java.time.Duration, it seems that it can only be compared with another Duration, so do we need to introduce a generic type here? Just use Duration directly?

Sure, good point. I guess the T extends Comparable<T> & TemporalAmount stuff is overly broad.

ndimiduk · 2022-01-24T20:43:44Z

Hmm, backup/restore test failures.

ndimiduk · 2022-01-26T00:17:09Z

TestResult fails for me locally on branch-2.5 when running on JDK11 + Hadoop3. Not introduced by this change.

Apache9 · 2022-01-26T05:15:08Z

The failure of TestResult is because of HBASE-26688.

Let me see how to address it.

Apache9 · 2022-02-02T06:02:29Z

+ attributes.put(RPC_METHOD, method);
+ }
+
+ private static String getRpcPackageAndService(final Descriptors.MethodDescriptor md) {


But IpcServerSpanBuilder is not for testing only? It is what we need to use at server side? I can not follow...

Apache9 · 2022-02-02T06:09:05Z

+
+ public IpcServerSpanBuilder(final RpcCall rpcCall) {
+ this.rpcCall = rpcCall;
+ // it happens that `getFullName` returns a string in the $package.$service format required by


What I mean is that, we have this logic in both IpcServerSpanBuilder and IpcClientSpanBuilder, as what you said in the comment, we may have to parse the value in the future then we need to modify both places. So I suggest that we have a common method to generate the span name from the ServiceDescriptor and MethodDescriptor.

Simplify `SpanDataMatchers#hasDuration` to work explicitly with `Duration` instances.

Further consolidate implementation extracting method information from protobuf objects.

remove unused import

Apache-HBase · 2022-02-08T15:00:23Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 59s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
		_ master Compile Tests _
+0 🆗	mvndep	0m 18s	Maven dependency ordering for branch
+1 💚	mvninstall	4m 20s	master passed
+1 💚	compile	9m 27s	master passed
+1 💚	checkstyle	2m 19s	master passed
+1 💚	spotbugs	15m 28s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 18s	Maven dependency ordering for patch
+1 💚	mvninstall	4m 16s	the patch passed
+1 💚	compile	9m 26s	the patch passed
+1 💚	javac	9m 26s	the patch passed
+1 💚	checkstyle	2m 20s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	xml	0m 3s	The patch has no ill-formed XML file.
+1 💚	hadoopcheck	22m 56s	Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1.
+1 💚	spotbugs	18m 2s	the patch passed
		_ Other Tests _
+1 💚	asflicense	0m 50s	The patch does not generate ASF License warnings.
		102m 41s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4024/6/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#4024
Optional Tests	dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile xml
uname	Linux 2f033cef5fa0 4.15.0-163-generic #171-Ubuntu SMP Fri Nov 5 11:55:11 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `36d8654`
Default Java	AdoptOpenJDK-1.8.0_282-b08
Max. process+thread count	126 (vs. ulimit of 30000)
modules	C: hbase-common hbase-client hbase-server . U: .
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4024/6/console
versions	git=2.17.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2022-02-08T18:38:26Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 0s	Docker mode activated.
-0 ⚠️	yetus	0m 3s	Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
		_ Prechecks _
		_ master Compile Tests _
+0 🆗	mvndep	0m 34s	Maven dependency ordering for branch
+1 💚	mvninstall	5m 10s	master passed
+1 💚	compile	3m 38s	master passed
+1 💚	shadedjars	9m 15s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	4m 47s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 19s	Maven dependency ordering for patch
+1 💚	mvninstall	5m 3s	the patch passed
+1 💚	compile	3m 36s	the patch passed
+1 💚	javac	3m 36s	the patch passed
+1 💚	shadedjars	9m 8s	patch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	4m 41s	the patch passed
		_ Other Tests _
+1 💚	unit	270m 46s	root in the patch passed.
		320m 47s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4024/6/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR	#4024
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux 115a8e68a1b6 4.15.0-162-generic #170-Ubuntu SMP Mon Oct 18 11:38:05 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `36d8654`
Default Java	AdoptOpenJDK-11.0.10+9
Test Results	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4024/6/testReport/
Max. process+thread count	4403 (vs. ulimit of 30000)
modules	C: hbase-common hbase-client hbase-server . U: .
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4024/6/console
versions	git=2.17.1 maven=3.6.3
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2022-02-08T20:19:33Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 26s	Docker mode activated.
-0 ⚠️	yetus	0m 3s	Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
		_ Prechecks _
		_ master Compile Tests _
+0 🆗	mvndep	0m 22s	Maven dependency ordering for branch
+1 💚	mvninstall	3m 49s	master passed
+1 💚	compile	2m 44s	master passed
+1 💚	shadedjars	8m 17s	branch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	3m 36s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 22s	Maven dependency ordering for patch
+1 💚	mvninstall	3m 48s	the patch passed
+1 💚	compile	2m 50s	the patch passed
+1 💚	javac	2m 50s	the patch passed
+1 💚	shadedjars	8m 14s	patch has no errors when building our shaded downstream artifacts.
+1 💚	javadoc	3m 35s	the patch passed
		_ Other Tests _
-1 ❌	unit	380m 29s	root in the patch failed.
		421m 50s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4024/6/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR	#4024
Optional Tests	javac javadoc unit shadedjars compile
uname	Linux a9294f2cd344 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `36d8654`
Default Java	AdoptOpenJDK-1.8.0_282-b08
unit	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4024/6/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-root.txt
Test Results	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4024/6/testReport/
Max. process+thread count	4981 (vs. ulimit of 30000)
modules	C: hbase-common hbase-client hbase-server . U: .
Console output	https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4024/6/console
versions	git=2.17.1 maven=3.6.3
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

Apache9

+1

Thanks Nick for the hard work!

ndimiduk · 2022-02-09T14:19:46Z

And thank you @Apache9 for sticking with me through the reviews!

Signed-off-by: Duo Zhang <zhangduo@apache.org>

ndimiduk requested review from Apache9, joshelser and taklwu January 12, 2022 21:06