HBASE-26648 Improve fidelity of RegionLocator spans

Signed-off-by: Duo Zhang <[email protected]>

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

@@ -67,8 +67,7 @@ public <T> ConnectionSpanBuilder addAttribute(final AttributeKey<T> key, T value
   public Span build() {
     final SpanBuilder builder = TraceUtil.getGlobalTracer()
       .spanBuilder(name)
-      // TODO: what about clients embedded in Master/RegionServer/Gateways/&c?
-      .setSpanKind(SpanKind.CLIENT);
+      .setSpanKind(SpanKind.INTERNAL);
     attributes.forEach((k, v) -> builder.setAttribute((AttributeKey<? super Object>) k, v));
     return builder.startSpan();
   }

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

@@ -64,8 +64,7 @@ public TableSpanBuilder setTableName(final TableName tableName) {
   public Span build() {
     final SpanBuilder builder = TraceUtil.getGlobalTracer()
       .spanBuilder(name)
-      // TODO: what about clients embedded in Master/RegionServer/Gateways/&c?
-      .setSpanKind(SpanKind.CLIENT);
+      .setSpanKind(SpanKind.INTERNAL);
     attributes.forEach((k, v) -> builder.setAttribute((AttributeKey<? super Object>) k, v));
     return builder.startSpan();
   }

hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestAsyncRegionLocatorTracing.java

@@ -132,7 +132,7 @@ public void testClearCache() {
     SpanData span = waitSpan("AsyncRegionLocator.clearCache");
     assertThat(span, allOf(
       hasStatusWithCode(StatusCode.OK),
-      hasKind(SpanKind.CLIENT),
+      hasKind(SpanKind.INTERNAL),
       buildConnectionAttributesMatcher(conn)));
   }
 
@@ -144,7 +144,7 @@ public void testClearCacheServerName() {
     SpanData span = waitSpan("AsyncRegionLocator.clearCache");
     assertThat(span, allOf(
       hasStatusWithCode(StatusCode.OK),
-      hasKind(SpanKind.CLIENT),
+      hasKind(SpanKind.INTERNAL),
       buildConnectionAttributesMatcher(conn),
       hasAttributes(containsEntry("db.hbase.server.name", sn.getServerName()))));
   }
@@ -155,7 +155,7 @@ public void testClearCacheTableName() {
     SpanData span = waitSpan("AsyncRegionLocator.clearCache");
     assertThat(span, allOf(
       hasStatusWithCode(StatusCode.OK),
-      hasKind(SpanKind.CLIENT),
+      hasKind(SpanKind.INTERNAL),
       buildConnectionAttributesMatcher(conn),
       buildTableAttributesMatcher(TableName.META_TABLE_NAME)));
   }
@@ -167,7 +167,7 @@ public void testGetRegionLocation() {
     SpanData span = waitSpan("AsyncRegionLocator.getRegionLocation");
     assertThat(span, allOf(
       hasStatusWithCode(StatusCode.OK),
-      hasKind(SpanKind.CLIENT),
+      hasKind(SpanKind.INTERNAL),
       buildConnectionAttributesMatcher(conn),
       buildTableAttributesMatcher(TableName.META_TABLE_NAME),
       hasAttributes(
@@ -186,7 +186,7 @@ public void testGetRegionLocations() {
       .toArray(String[]::new);
     assertThat(span, allOf(
       hasStatusWithCode(StatusCode.OK),
-      hasKind(SpanKind.CLIENT),
+      hasKind(SpanKind.INTERNAL),
       buildConnectionAttributesMatcher(conn),
       buildTableAttributesMatcher(TableName.META_TABLE_NAME),
       hasAttributes(

hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtil.java

@@ -49,6 +49,7 @@
 import java.util.Random;
 import java.util.Set;
 import java.util.TreeSet;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ThreadLocalRandom;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
@@ -61,6 +62,7 @@
 import org.apache.hadoop.hbase.Waiter.ExplainingPredicate;
 import org.apache.hadoop.hbase.Waiter.Predicate;
 import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.AsyncAdmin;
 import org.apache.hadoop.hbase.client.AsyncClusterConnection;
 import org.apache.hadoop.hbase.client.BufferedMutator;
 import org.apache.hadoop.hbase.client.ClusterConnectionFactory;
@@ -1534,6 +1536,16 @@ public static void setReplicas(Admin admin, TableName table, int replicaCount)
     admin.modifyTable(desc);
   }
 
+  /**
+   * Set the number of Region replicas.
+   */
+  public static void setReplicas(AsyncAdmin admin, TableName table, int replicaCount)
+    throws ExecutionException, IOException, InterruptedException {
+    TableDescriptor desc = TableDescriptorBuilder.newBuilder(admin.getDescriptor(table).get())
+      .setRegionReplication(replicaCount).build();
+    admin.modifyTable(desc).get();
+  }
+
   /**
    * Drop an existing table
    * @param tableName existing table

hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncMetaRegionLocator.java

@@ -18,68 +18,153 @@
 package org.apache.hadoop.hbase.client;
 
 import static org.apache.hadoop.hbase.client.RegionReplicaTestHelper.testLocator;
-
+import static org.apache.hadoop.hbase.client.trace.hamcrest.SpanDataMatchers.hasEnded;
+import static org.apache.hadoop.hbase.client.trace.hamcrest.SpanDataMatchers.hasKind;
+import static org.apache.hadoop.hbase.client.trace.hamcrest.SpanDataMatchers.hasName;
+import static org.apache.hadoop.hbase.client.trace.hamcrest.SpanDataMatchers.hasParentSpanId;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.allOf;
+import static org.hamcrest.Matchers.endsWith;
+import static org.hamcrest.Matchers.hasItem;
+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.sdk.trace.data.SpanData;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.ConnectionRule;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
 import org.apache.hadoop.hbase.HBaseTestingUtil;
 import org.apache.hadoop.hbase.HRegionLocation;
+import org.apache.hadoop.hbase.MatcherPredicate;
+import org.apache.hadoop.hbase.MiniClusterRule;
 import org.apache.hadoop.hbase.RegionLocations;
+import org.apache.hadoop.hbase.StartTestingClusterOption;
 import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.Waiter;
 import org.apache.hadoop.hbase.client.RegionReplicaTestHelper.Locator;
+import org.apache.hadoop.hbase.client.trace.StringTraceRenderer;
 import org.apache.hadoop.hbase.testclassification.ClientTests;
 import org.apache.hadoop.hbase.testclassification.MediumTests;
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
+import org.apache.hadoop.hbase.trace.OpenTelemetryClassRule;
+import org.apache.hadoop.hbase.trace.OpenTelemetryTestRule;
+import org.apache.hadoop.hbase.trace.TraceUtil;
+import org.hamcrest.Matcher;
 import org.junit.ClassRule;
+import org.junit.Rule;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
-
-import org.apache.hbase.thirdparty.com.google.common.io.Closeables;
+import org.junit.rules.ExternalResource;
+import org.junit.rules.RuleChain;
+import org.junit.rules.TestRule;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 @Category({ MediumTests.class, ClientTests.class })
 public class TestAsyncMetaRegionLocator {
+  private static final Logger logger = LoggerFactory.getLogger(TestAsyncMetaRegionLocator.class);
 
   @ClassRule
   public static final HBaseClassTestRule CLASS_RULE =
     HBaseClassTestRule.forClass(TestAsyncMetaRegionLocator.class);
 
-  private static final HBaseTestingUtil TEST_UTIL = new HBaseTestingUtil();
+  private static final OpenTelemetryClassRule otelClassRule = OpenTelemetryClassRule.create();
+  private static final MiniClusterRule miniClusterRule = MiniClusterRule.newBuilder()
+    .setMiniClusterOption(StartTestingClusterOption.builder()
+      .numWorkers(3)
+      .build())
+    .build();
+  private static final ConnectionRule connectionRule =
+    ConnectionRule.createAsyncConnectionRule(miniClusterRule::createAsyncConnection);
 
-  private static ConnectionRegistry REGISTRY;
+  private static final class Setup extends ExternalResource {
+    private ConnectionRegistry registry;
 
-  private static AsyncMetaRegionLocator LOCATOR;
+    @Override
+    protected void before() throws Throwable {
+      final AsyncAdmin admin = connectionRule.getAsyncConnection().getAdmin();
+      TEST_UTIL = miniClusterRule.getTestingUtility();
+      HBaseTestingUtil.setReplicas(admin, TableName.META_TABLE_NAME, 3);
+      TEST_UTIL.waitUntilNoRegionsInTransition();
+      registry = ConnectionRegistryFactory.getRegistry(TEST_UTIL.getConfiguration());
+      RegionReplicaTestHelper.waitUntilAllMetaReplicasAreReady(TEST_UTIL, registry);
+      admin.balancerSwitch(false).get();
+      LOCATOR = new AsyncMetaRegionLocator(registry);
+    }
 
-  @BeforeClass
-  public static void setUp() throws Exception {
-    TEST_UTIL.startMiniCluster(3);
-    HBaseTestingUtil.setReplicas(TEST_UTIL.getAdmin(), TableName.META_TABLE_NAME, 3);
-    TEST_UTIL.waitUntilNoRegionsInTransition();
-    REGISTRY = ConnectionRegistryFactory.getRegistry(TEST_UTIL.getConfiguration());
-    RegionReplicaTestHelper.waitUntilAllMetaReplicasAreReady(TEST_UTIL, REGISTRY);
-    TEST_UTIL.getAdmin().balancerSwitch(false, true);
-    LOCATOR = new AsyncMetaRegionLocator(REGISTRY);
+    @Override
+    protected void after() {
+      registry.close();
+    }
   }
 
-  @AfterClass
-  public static void tearDown() throws Exception {
-    Closeables.close(REGISTRY, true);
-    TEST_UTIL.shutdownMiniCluster();
-  }
+  @ClassRule
+  public static final TestRule classRule = RuleChain.outerRule(otelClassRule)
+    .around(miniClusterRule)
+    .around(connectionRule)
+    .around(new Setup());
+
+  private static HBaseTestingUtil TEST_UTIL;
+  private static AsyncMetaRegionLocator LOCATOR;
+
+  @Rule
+  public final OpenTelemetryTestRule otelTestRule = new OpenTelemetryTestRule(otelClassRule);
 
   @Test
   public void test() throws Exception {
-    testLocator(TEST_UTIL, TableName.META_TABLE_NAME, new Locator() {
+    TraceUtil.trace(() -> {
+      try {
+        testLocator(miniClusterRule.getTestingUtility(), TableName.META_TABLE_NAME, new Locator() {
+          @Override
+          public void updateCachedLocationOnError(HRegionLocation loc, Throwable error) {
+            LOCATOR.updateCachedLocationOnError(loc, error);
+          }
 
-      @Override
-      public void updateCachedLocationOnError(HRegionLocation loc, Throwable error)
-          throws Exception {
-        LOCATOR.updateCachedLocationOnError(loc, error);
+          @Override
+          public RegionLocations getRegionLocations(
+            TableName tableName,
+            int replicaId,
+            boolean reload
+          ) throws Exception {
+            return LOCATOR.getRegionLocations(replicaId, reload).get();
+          }
+        });
+      } catch (Exception e) {
+        throw new RuntimeException(e);
       }
+    }, "test");
 
-      @Override
-      public RegionLocations getRegionLocations(TableName tableName, int replicaId, boolean reload)
-          throws Exception {
-        return LOCATOR.getRegionLocations(replicaId, reload).get();
-      }
-    });
+    final Configuration conf = TEST_UTIL.getConfiguration();
+    final Matcher<SpanData> parentSpanMatcher = allOf(hasName("test"), hasEnded());
+    Waiter.waitFor(conf, TimeUnit.SECONDS.toMillis(5), new MatcherPredicate<>(
+      otelClassRule::getSpans, hasItem(parentSpanMatcher)));
+    final List<SpanData> spans = otelClassRule.getSpans();
+    if (logger.isDebugEnabled()) {
+      StringTraceRenderer renderer = new StringTraceRenderer(spans);
+      renderer.render(logger::debug);
+    }
+
+    assertThat(spans, hasItem(parentSpanMatcher));
+    final SpanData parentSpan = spans.stream()
+      .filter(parentSpanMatcher::matches)
+      .findAny()
+      .orElseThrow(AssertionError::new);
+
+    final Matcher<SpanData> registryGetMetaRegionLocationsMatcher = allOf(
+      hasName(endsWith("ConnectionRegistry.getMetaRegionLocations")),
+      hasParentSpanId(parentSpan),
+      hasKind(SpanKind.INTERNAL),
+      hasEnded());
+    assertThat(spans, hasItem(registryGetMetaRegionLocationsMatcher));
+    final SpanData registry_getMetaRegionLocationsSpan = spans.stream()
+      .filter(registryGetMetaRegionLocationsMatcher::matches)
+      .findAny()
+      .orElseThrow(AssertionError::new);
+
+    final Matcher<SpanData> clientGetMetaRegionLocationsMatcher = allOf(
+      hasName(endsWith("ClientMetaService/GetMetaRegionLocations")),
+      hasParentSpanId(registry_getMetaRegionLocationsSpan),
+      hasKind(SpanKind.CLIENT),
+      hasEnded());
+    assertThat(spans, hasItem(clientGetMetaRegionLocationsMatcher));
   }
 }