fix: Refine connecitivity metrics to capture RPCs with no response he… (googleapis#4252)

* fix: Refine connecitivity metrics to capture RPCs with no response headers

* test fix

Author: surbhigarg92

google-cloud-spanner/clirr-ignored-differences.xml

@@ -1062,4 +1062,34 @@
     <className>com/google/cloud/spanner/connection/Connection</className>
     <method>java.lang.Object getConnectionPropertyValue(com.google.cloud.spanner.connection.ConnectionProperty)</method>
   </difference>
+  <!-- (Remove since these methods has no usage. CompositeTracer interface is annotated as internal API) -->
+  <difference>
+    <differenceType>7002</differenceType>
+    <className>com/google/cloud/spanner/CompositeTracer</className>
+    <method>void recordAFELatency(java.lang.Long)</method>
+  </difference>
+  <difference>
+    <differenceType>7002</differenceType>
+    <className>com/google/cloud/spanner/CompositeTracer</className>
+    <method>void recordAFELatency(java.lang.Float)</method>
+  </difference>
+  <difference>
+    <differenceType>7002</differenceType>
+    <className>com/google/cloud/spanner/CompositeTracer</className>
+    <method>void recordAfeHeaderMissingCount(java.lang.Long)</method>
+  </difference>
+  <difference>
+    <differenceType>7002</differenceType>
+    <className>com/google/cloud/spanner/CompositeTracer</className>
+    <method>void recordGFELatency(java.lang.Long)</method>
+  </difference>
+  <difference>
+    <differenceType>7002</differenceType>
+    <className>com/google/cloud/spanner/CompositeTracer</className>
+    <method>void recordGFELatency(java.lang.Float)</method>
+  </difference><difference>
+  <differenceType>7002</differenceType>
+  <className>com/google/cloud/spanner/CompositeTracer</className>
+  <method>void recordGfeHeaderMissingCount(java.lang.Long)</method>
+</difference>
 </differences>

google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsRecorder.java

@@ -99,21 +99,23 @@ class BuiltInMetricsRecorder extends OpenTelemetryMetricsRecorder {
   void recordServerTimingHeaderMetrics(
       Float gfeLatency,
       Float afeLatency,
-      Long gfeHeaderMissingCount,
-      Long afeHeaderMissingCount,
-      Map<String, String> attributes) {
+      Map<String, String> attributes,
+      boolean isDirectPathUsed,
+      boolean isAfeEnabled) {
     io.opentelemetry.api.common.Attributes otelAttributes = toOtelAttributes(attributes);
-    if (gfeLatency != null) {
-      gfeLatencyRecorder.record(gfeLatency, otelAttributes);
+    if (!isDirectPathUsed) {
+      if (gfeLatency != null) {
+        gfeLatencyRecorder.record(gfeLatency, otelAttributes);
+      } else {
+        gfeHeaderMissingCountRecorder.add(1, otelAttributes);
+      }
     }
-    if (gfeHeaderMissingCount > 0) {
-      gfeHeaderMissingCountRecorder.add(gfeHeaderMissingCount, otelAttributes);
-    }
-    if (afeLatency != null) {
-      afeLatencyRecorder.record(afeLatency, otelAttributes);
-    }
-    if (afeHeaderMissingCount > 0) {
-      afeHeaderMissingCountRecorder.add(afeHeaderMissingCount, otelAttributes);
+    if (isAfeEnabled) {
+      if (afeLatency != null) {
+        afeLatencyRecorder.record(afeLatency, otelAttributes);
+      } else {
+        afeHeaderMissingCountRecorder.add(1, otelAttributes);
+      }
     }
   }
 

google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsTracer.java

@@ -39,10 +39,10 @@ class BuiltInMetricsTracer extends MetricsTracer implements ApiTracer {
   private final Map<String, String> attributes = new HashMap<>();
   private Float gfeLatency = null;
   private Float afeLatency = null;
-  private TraceWrapper traceWrapper;
-  private long gfeHeaderMissingCount = 0;
-  private long afeHeaderMissingCount = 0;
+  private final TraceWrapper traceWrapper;
   private final ISpan currentSpan;
+  private boolean isDirectPathUsed;
+  private boolean isAfeEnabled;
 
   BuiltInMetricsTracer(
       MethodName methodName,
@@ -66,7 +66,7 @@ public void attemptSucceeded() {
       super.attemptSucceeded();
       attributes.put(STATUS_ATTRIBUTE, StatusCode.Code.OK.toString());
       builtInOpenTelemetryMetricsRecorder.recordServerTimingHeaderMetrics(
-          gfeLatency, afeLatency, gfeHeaderMissingCount, afeHeaderMissingCount, attributes);
+          gfeLatency, afeLatency, attributes, isDirectPathUsed, isAfeEnabled);
     }
   }
 
@@ -80,7 +80,7 @@ public void attemptCancelled() {
       super.attemptCancelled();
       attributes.put(STATUS_ATTRIBUTE, StatusCode.Code.CANCELLED.toString());
       builtInOpenTelemetryMetricsRecorder.recordServerTimingHeaderMetrics(
-          gfeLatency, afeLatency, gfeHeaderMissingCount, afeHeaderMissingCount, attributes);
+          gfeLatency, afeLatency, attributes, isDirectPathUsed, isAfeEnabled);
     }
   }
 
@@ -98,7 +98,7 @@ public void attemptFailedDuration(Throwable error, java.time.Duration delay) {
       super.attemptFailedDuration(error, delay);
       attributes.put(STATUS_ATTRIBUTE, extractStatus(error));
       builtInOpenTelemetryMetricsRecorder.recordServerTimingHeaderMetrics(
-          gfeLatency, afeLatency, gfeHeaderMissingCount, afeHeaderMissingCount, attributes);
+          gfeLatency, afeLatency, attributes, isDirectPathUsed, isAfeEnabled);
     }
   }
 
@@ -115,7 +115,7 @@ public void attemptFailedRetriesExhausted(Throwable error) {
       super.attemptFailedRetriesExhausted(error);
       attributes.put(STATUS_ATTRIBUTE, extractStatus(error));
       builtInOpenTelemetryMetricsRecorder.recordServerTimingHeaderMetrics(
-          gfeLatency, afeLatency, gfeHeaderMissingCount, afeHeaderMissingCount, attributes);
+          gfeLatency, afeLatency, attributes, isDirectPathUsed, isAfeEnabled);
     }
   }
 
@@ -132,24 +132,16 @@ public void attemptPermanentFailure(Throwable error) {
       super.attemptPermanentFailure(error);
       attributes.put(STATUS_ATTRIBUTE, extractStatus(error));
       builtInOpenTelemetryMetricsRecorder.recordServerTimingHeaderMetrics(
-          gfeLatency, afeLatency, gfeHeaderMissingCount, afeHeaderMissingCount, attributes);
+          gfeLatency, afeLatency, attributes, isDirectPathUsed, isAfeEnabled);
     }
   }
 
-  void recordGFELatency(Float gfeLatency) {
+  public void recordServerTimingHeaderMetrics(
+      Float gfeLatency, Float afeLatency, boolean isDirectPathUsed, boolean isAfeEnabled) {
     this.gfeLatency = gfeLatency;
-  }
-
-  void recordAFELatency(Float afeLatency) {
+    this.isDirectPathUsed = isDirectPathUsed;
     this.afeLatency = afeLatency;
-  }
-
-  void recordGfeHeaderMissingCount(Long value) {
-    this.gfeHeaderMissingCount = value;
-  }
-
-  void recordAfeHeaderMissingCount(Long value) {
-    this.afeHeaderMissingCount = value;
+    this.isAfeEnabled = isAfeEnabled;
   }
 
   @Override

google-cloud-spanner/src/main/java/com/google/cloud/spanner/CompositeTracer.java

@@ -191,50 +191,13 @@ public void addAttributes(Map<String, String> attributes) {
     }
   }
 
-  public void recordGFELatency(Long gfeLatency) {
+  public void recordServerTimingHeaderMetrics(
+      Float gfeLatency, Float afeLatency, boolean isDirectPathUsed, boolean isAfeEnabled) {
     for (ApiTracer child : children) {
       if (child instanceof BuiltInMetricsTracer) {
-        ((BuiltInMetricsTracer) child).recordGFELatency(Float.valueOf(gfeLatency));
-      }
-    }
-  }
-
-  public void recordGfeHeaderMissingCount(Long value) {
-    for (ApiTracer child : children) {
-      if (child instanceof BuiltInMetricsTracer) {
-        ((BuiltInMetricsTracer) child).recordGfeHeaderMissingCount(value);
-      }
-    }
-  }
-
-  public void recordAFELatency(Long afeLatency) {
-    for (ApiTracer child : children) {
-      if (child instanceof BuiltInMetricsTracer) {
-        ((BuiltInMetricsTracer) child).recordAFELatency(Float.valueOf(afeLatency));
-      }
-    }
-  }
-
-  public void recordAfeHeaderMissingCount(Long value) {
-    for (ApiTracer child : children) {
-      if (child instanceof BuiltInMetricsTracer) {
-        ((BuiltInMetricsTracer) child).recordAfeHeaderMissingCount(value);
-      }
-    }
-  }
-
-  public void recordGFELatency(Float gfeLatency) {
-    for (ApiTracer child : children) {
-      if (child instanceof BuiltInMetricsTracer) {
-        ((BuiltInMetricsTracer) child).recordGFELatency(gfeLatency);
-      }
-    }
-  }
-
-  public void recordAFELatency(Float afeLatency) {
-    for (ApiTracer child : children) {
-      if (child instanceof BuiltInMetricsTracer) {
-        ((BuiltInMetricsTracer) child).recordAFELatency(afeLatency);
+        ((BuiltInMetricsTracer) child)
+            .recordServerTimingHeaderMetrics(
+                gfeLatency, afeLatency, isDirectPathUsed, isAfeEnabled);
       }
     }
   }