Skip to content

fix: change server timing duration attribute to float as per w3c #3851

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

Merged
merged 4 commits into from
May 9, 2025
Merged

fix: change server timing duration attribute to float as per w3c #3851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
448061e
fix: change server timing duration attribute to float as per w3c
surbhigarg92 May 8, 2025
1b80ffe
fix clirr
rahul2393 May 8, 2025
a82f14e
remove deprecated warning
rahul2393 May 8, 2025
fa01e13
Merge branch 'main' into server_timing_float
rahul2393 May 9, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsRecorder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@ class BuiltInMetricsRecorder extends OpenTelemetryMetricsRecorder {
* @param attributes Map of the attributes to store
*/
void recordServerTimingHeaderMetrics(
Long gfeLatency,
Long afeLatency,
Float gfeLatency,
Float afeLatency,
Long gfeHeaderMissingCount,
Long afeHeaderMissingCount,
Map<String, String> attributes) {
Expand Down
8 changes: 4 additions & 4 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsTracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ class BuiltInMetricsTracer extends MetricsTracer implements ApiTracer {
private final BuiltInMetricsRecorder builtInOpenTelemetryMetricsRecorder;
// These are RPC specific attributes and pertain to a specific API Trace
private final Map<String, String> attributes = new HashMap<>();
private Long gfeLatency = null;
private Long afeLatency = null;
private Float gfeLatency = null;
private Float afeLatency = null;
private long gfeHeaderMissingCount = 0;
private long afeHeaderMissingCount = 0;

Expand Down Expand Up @@ -119,11 +119,11 @@ public void attemptPermanentFailure(Throwable error) {
gfeLatency, afeLatency, gfeHeaderMissingCount, afeHeaderMissingCount, attributes);
}

void recordGFELatency(Long gfeLatency) {
void recordGFELatency(Float gfeLatency) {
this.gfeLatency = gfeLatency;
}

void recordAFELatency(Long afeLatency) {
void recordAFELatency(Float afeLatency) {
this.afeLatency = afeLatency;
}

Expand Down
20 changes: 18 additions & 2 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/CompositeTracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ public void addAttributes(Map<String, String> attributes) {
public void recordGFELatency(Long gfeLatency) {
for (ApiTracer child : children) {
if (child instanceof BuiltInMetricsTracer) {
((BuiltInMetricsTracer) child).recordGFELatency(gfeLatency);
((BuiltInMetricsTracer) child).recordGFELatency(Float.valueOf(gfeLatency));
}
}
}
Expand All @@ -210,7 +210,7 @@ public void recordGfeHeaderMissingCount(Long value) {
public void recordAFELatency(Long afeLatency) {
for (ApiTracer child : children) {
if (child instanceof BuiltInMetricsTracer) {
((BuiltInMetricsTracer) child).recordAFELatency(afeLatency);
((BuiltInMetricsTracer) child).recordAFELatency(Float.valueOf(afeLatency));
}
}
}
Expand All @@ -222,4 +222,20 @@ public void recordAfeHeaderMissingCount(Long 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);
}
}
}
}
18 changes: 9 additions & 9 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/HeaderInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ class HeaderInterceptor implements ClientInterceptor {
private static final Metadata.Key<String> GOOGLE_CLOUD_RESOURCE_PREFIX_KEY =
Metadata.Key.of("google-cloud-resource-prefix", Metadata.ASCII_STRING_MARSHALLER);
private static final Pattern SERVER_TIMING_PATTERN =
Pattern.compile("(?<metricName>[a-zA-Z0-9_-]+);\\s*dur=(?<duration>\\d+)");
Pattern.compile("(?<metricName>[a-zA-Z0-9_-]+);\\s*dur=(?<duration>\\d+(\\.\\d+)?)");
private static final Pattern GOOGLE_CLOUD_RESOURCE_PREFIX_PATTERN =
Pattern.compile(
".*projects/(?<project>\\p{ASCII}[^/]*)(/instances/(?<instance>\\p{ASCII}[^/]*))?(/databases/(?<database>\\p{ASCII}[^/]*))?");
Expand Down Expand Up @@ -162,15 +162,15 @@ private void processHeader(
// would fail to parse it correctly. To make the parsing more robust, the logic has been
// updated to handle multiple metrics gracefully.

Map<String, Long> serverTimingMetrics = parseServerTimingHeader(serverTiming);
Map<String, Float> serverTimingMetrics = parseServerTimingHeader(serverTiming);
if (serverTimingMetrics.containsKey(GFE_TIMING_HEADER)) {
long gfeLatency = serverTimingMetrics.get(GFE_TIMING_HEADER);
float gfeLatency = serverTimingMetrics.get(GFE_TIMING_HEADER);

measureMap.put(SPANNER_GFE_LATENCY, gfeLatency);
measureMap.put(SPANNER_GFE_LATENCY, (long) gfeLatency);
measureMap.put(SPANNER_GFE_HEADER_MISSING_COUNT, 0L);
measureMap.record(tagContext);

spannerRpcMetrics.recordGfeLatency(gfeLatency, attributes);
spannerRpcMetrics.recordGfeLatency((long) gfeLatency, attributes);
spannerRpcMetrics.recordGfeHeaderMissingCount(0L, attributes);
if (compositeTracer != null) {
compositeTracer.recordGFELatency(gfeLatency);
Expand All @@ -189,7 +189,7 @@ private void processHeader(
// Record AFE metrics
if (compositeTracer != null && GapicSpannerRpc.isEnableAFEServerTiming()) {
if (serverTimingMetrics.containsKey(AFE_TIMING_HEADER)) {
long afeLatency = serverTimingMetrics.get(AFE_TIMING_HEADER);
float afeLatency = serverTimingMetrics.get(AFE_TIMING_HEADER);
compositeTracer.recordAFELatency(afeLatency);
} else {
compositeTracer.recordAfeHeaderMissingCount(1L);
Expand All @@ -200,16 +200,16 @@ private void processHeader(
}
}

private Map<String, Long> parseServerTimingHeader(String serverTiming) {
Map<String, Long> serverTimingMetrics = new HashMap<>();
private Map<String, Float> parseServerTimingHeader(String serverTiming) {
Map<String, Float> serverTimingMetrics = new HashMap<>();
if (serverTiming != null) {
Matcher matcher = SERVER_TIMING_PATTERN.matcher(serverTiming);
while (matcher.find()) {
String metricName = matcher.group("metricName");
String durationStr = matcher.group("duration");

if (metricName != null && durationStr != null) {
serverTimingMetrics.put(metricName, Long.valueOf(durationStr));
serverTimingMetrics.put(metricName, Float.valueOf(durationStr));
}
}
}
Expand Down
13 changes: 6 additions & 7 deletions google-cloud-spanner/src/test/java/com/google/cloud/spanner/AbstractNettyMockServerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
import java.util.Random;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;
import org.junit.After;
import org.junit.AfterClass;
import org.junit.Before;
Expand All @@ -44,11 +44,10 @@ abstract class AbstractNettyMockServerTest {
protected static InetSocketAddress address;
static ExecutorService executor;
protected static LocalChannelProvider channelProvider;
protected static AtomicInteger fakeServerTiming =
new AtomicInteger(new Random().nextInt(1000) + 1);

protected static AtomicInteger fakeAFEServerTiming =
new AtomicInteger(new Random().nextInt(500) + 1);
protected static final AtomicReference<Float> fakeServerTiming =
new AtomicReference<>((float) (new Random().nextDouble() * 1000) + 1);
protected static final AtomicReference<Float> fakeAFEServerTiming =
new AtomicReference<>((float) new Random().nextInt(500) + 1);

protected Spanner spanner;

Expand Down Expand Up @@ -76,7 +75,7 @@ public void sendHeaders(Metadata headers) {
headers.put(
Metadata.Key.of("server-timing", Metadata.ASCII_STRING_MARSHALLER),
String.format(
"afe; dur=%d, gfet4t7; dur=%d",
"afe; dur=%f, gfet4t7; dur=%f",
fakeAFEServerTiming.get(), fakeServerTiming.get()));
super.sendHeaders(headers);
}
Expand Down
29 changes: 15 additions & 14 deletions ...spanner/src/test/java/com/google/cloud/spanner/OpenTelemetryBuiltInMetricsTracerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public class OpenTelemetryBuiltInMetricsTracerTest extends AbstractNettyMockServ
Attributes.builder().put(BuiltInMetricsConstant.DIRECT_PATH_USED_KEY, "false").build();
;

private static final long MIN_LATENCY = 0;
private static final double MIN_LATENCY = 0;

private DatabaseClient client;

Expand Down Expand Up @@ -159,7 +159,7 @@ public void testMetricsSingleUseQuery() {
assertFalse(resultSet.next());
}

long elapsed = stopwatch.elapsed(TimeUnit.MILLISECONDS);
double elapsed = stopwatch.elapsed(TimeUnit.MILLISECONDS);
Attributes expectedAttributes =
expectedCommonBaseAttributes.toBuilder()
.putAll(expectedCommonRequestAttributes)
Expand All @@ -170,13 +170,14 @@ public void testMetricsSingleUseQuery() {
MetricData operationLatencyMetricData =
getMetricData(metricReader, BuiltInMetricsConstant.OPERATION_LATENCIES_NAME);
assertNotNull(operationLatencyMetricData);
long operationLatencyValue = getAggregatedValue(operationLatencyMetricData, expectedAttributes);
double operationLatencyValue =
getAggregatedValue(operationLatencyMetricData, expectedAttributes);
assertThat(operationLatencyValue).isIn(Range.closed(MIN_LATENCY, elapsed));

MetricData attemptLatencyMetricData =
getMetricData(metricReader, BuiltInMetricsConstant.ATTEMPT_LATENCIES_NAME);
assertNotNull(attemptLatencyMetricData);
long attemptLatencyValue = getAggregatedValue(attemptLatencyMetricData, expectedAttributes);
double attemptLatencyValue = getAggregatedValue(attemptLatencyMetricData, expectedAttributes);
assertThat(attemptLatencyValue).isIn(Range.closed(MIN_LATENCY, elapsed));

MetricData operationCountMetricData =
Expand All @@ -191,7 +192,7 @@ public void testMetricsSingleUseQuery() {

MetricData gfeLatencyMetricData =
getMetricData(metricReader, BuiltInMetricsConstant.GFE_LATENCIES_NAME);
long gfeLatencyValue = getAggregatedValue(gfeLatencyMetricData, expectedAttributes);
double gfeLatencyValue = getAggregatedValue(gfeLatencyMetricData, expectedAttributes);
assertEquals(fakeServerTiming.get(), gfeLatencyValue, 0);

assertFalse(
Expand Down Expand Up @@ -229,7 +230,7 @@ public void testMetricsSingleUseQueryWithAfeEnabled() throws Exception {
assertFalse(resultSet.next());
}

long elapsed = stopwatch.elapsed(TimeUnit.MILLISECONDS);
double elapsed = stopwatch.elapsed(TimeUnit.MILLISECONDS);
Attributes expectedAttributes =
expectedCommonBaseAttributes.toBuilder()
.putAll(expectedCommonRequestAttributes)
Expand All @@ -240,14 +241,14 @@ public void testMetricsSingleUseQueryWithAfeEnabled() throws Exception {
MetricData operationLatencyMetricData =
getMetricData(metricReader, BuiltInMetricsConstant.OPERATION_LATENCIES_NAME);
assertNotNull(operationLatencyMetricData);
long operationLatencyValue =
double operationLatencyValue =
getAggregatedValue(operationLatencyMetricData, expectedAttributes);
assertThat(operationLatencyValue).isIn(Range.closed(MIN_LATENCY, elapsed));

MetricData attemptLatencyMetricData =
getMetricData(metricReader, BuiltInMetricsConstant.ATTEMPT_LATENCIES_NAME);
assertNotNull(attemptLatencyMetricData);
long attemptLatencyValue = getAggregatedValue(attemptLatencyMetricData, expectedAttributes);
double attemptLatencyValue = getAggregatedValue(attemptLatencyMetricData, expectedAttributes);
assertThat(attemptLatencyValue).isIn(Range.closed(MIN_LATENCY, elapsed));

MetricData operationCountMetricData =
Expand All @@ -262,15 +263,15 @@ public void testMetricsSingleUseQueryWithAfeEnabled() throws Exception {

MetricData gfeLatencyMetricData =
getMetricData(metricReader, BuiltInMetricsConstant.GFE_LATENCIES_NAME);
long gfeLatencyValue = getAggregatedValue(gfeLatencyMetricData, expectedAttributes);
double gfeLatencyValue = getAggregatedValue(gfeLatencyMetricData, expectedAttributes);
assertEquals(fakeServerTiming.get(), gfeLatencyValue, 0);

assertFalse(
checkIfMetricExists(metricReader, BuiltInMetricsConstant.GFE_CONNECTIVITY_ERROR_NAME));

MetricData afeLatencyMetricData =
getMetricData(metricReader, BuiltInMetricsConstant.AFE_LATENCIES_NAME);
long afeLatencyValue = getAggregatedValue(afeLatencyMetricData, expectedAttributes);
double afeLatencyValue = getAggregatedValue(afeLatencyMetricData, expectedAttributes);
assertEquals(fakeAFEServerTiming.get(), afeLatencyValue, 0);
assertFalse(
checkIfMetricExists(metricReader, BuiltInMetricsConstant.AFE_CONNECTIVITY_ERROR_NAME));
Expand Down Expand Up @@ -402,7 +403,7 @@ public void testNoNetworkConnection() {

// Attempt count should have a failed metric point for CreateSession.
assertEquals(
1, getAggregatedValue(attemptCountMetricData, expectedAttributesCreateSessionFailed));
1, getAggregatedValue(attemptCountMetricData, expectedAttributesCreateSessionFailed), 0);
}

@Test
Expand Down Expand Up @@ -509,14 +510,14 @@ private boolean checkIfMetricExists(InMemoryMetricReader reader, String metricNa
return false;
}

private long getAggregatedValue(MetricData metricData, Attributes attributes) {
private float getAggregatedValue(MetricData metricData, Attributes attributes) {
switch (metricData.getType()) {
case HISTOGRAM:
return metricData.getHistogramData().getPoints().stream()
.filter(pd -> pd.getAttributes().equals(attributes))
.map(data -> (long) data.getSum() / data.getCount())
.map(data -> (float) data.getSum() / data.getCount())
.findFirst()
.orElse(0L);
.orElse(0F);
case LONG_SUM:
return metricData.getLongSumData().getPoints().stream()
.filter(pd -> pd.getAttributes().equals(attributes))
Expand Down
Loading