zydronium/google-nomulus
1
0
Fork 0
mirror of https://github.com/google/nomulus.git synced 2025-05-13 16:07:15 +02:00
Code Issues Projects Releases Packages Wiki Activity

Fix proxy metrics instrumentation bug

Browse source 
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=197209531
This commit is contained in:
jianglai 2018-05-18 15:27:52 -07:00
parent 68b24f0a54
commit 0cb303ed7f
4 changed files with 27 additions and 18 deletions
Download patch file Download diff file Expand all files Collapse all files

6
java/google/registry/proxy/handler/BackendMetricsHandler.java
View file

@ -113,6 +113,10 @@ public class BackendMetricsHandler extends ChannelDuplexHandler {
} }
FullHttpRequest request = (FullHttpRequest) msg; FullHttpRequest request = (FullHttpRequest) msg;
// Record request size now because the content would have read by the time the listener is
// called and the readable bytes would be zero by then.
int bytes = request.content().readableBytes();
// Record sent time before write finishes allows us to take network latency into account. // Record sent time before write finishes allows us to take network latency into account.
DateTime sentTime = clock.nowUtc(); DateTime sentTime = clock.nowUtc();
ChannelFuture unusedFuture = ChannelFuture unusedFuture =
@ -121,7 +125,7 @@ public class BackendMetricsHandler extends ChannelDuplexHandler {
future -> { future -> {
if (future.isSuccess()) { if (future.isSuccess()) {
// Only instrument request metrics when the request is actually sent to GAE. // Only instrument request metrics when the request is actually sent to GAE.
metrics.requestSent(relayedProtocolName, clientCertHash, request); metrics.requestSent(relayedProtocolName, clientCertHash, bytes);
requestSentTimeQueue.add(sentTime); requestSentTimeQueue.add(sentTime);
} }
}); });

5
java/google/registry/proxy/metric/BackendMetrics.java
View file

@ -23,7 +23,6 @@ import com.google.monitoring.metrics.IncrementableMetric;
import com.google.monitoring.metrics.LabelDescriptor; import com.google.monitoring.metrics.LabelDescriptor;
import com.google.monitoring.metrics.MetricRegistryImpl; import com.google.monitoring.metrics.MetricRegistryImpl;
import google.registry.util.NonFinalForTesting; import google.registry.util.NonFinalForTesting;
import io.netty.handler.codec.http.FullHttpRequest;
import io.netty.handler.codec.http.FullHttpResponse; import io.netty.handler.codec.http.FullHttpResponse;
import javax.inject.Inject; import javax.inject.Inject;
import javax.inject.Singleton; import javax.inject.Singleton;
@ -112,9 +111,9 @@ public class BackendMetrics {
} }
@NonFinalForTesting @NonFinalForTesting
public void requestSent(String protocol, String certHash, FullHttpRequest request) { public void requestSent(String protocol, String certHash, int bytes) {
requestsCounter.increment(protocol, certHash); requestsCounter.increment(protocol, certHash);
requestBytes.record(request.content().readableBytes(), protocol, certHash); requestBytes.record(bytes, protocol, certHash);
} }
@NonFinalForTesting @NonFinalForTesting

26
javatests/google/registry/proxy/handler/BackendMetricsHandlerTest.java
View file

@ -114,7 +114,8 @@ public class BackendMetricsHandlerTest {
// outbound message passed to the next handler. // outbound message passed to the next handler.
assertThat(channel.writeOutbound(request)).isTrue(); assertThat(channel.writeOutbound(request)).isTrue();
assertHttpRequestEquivalent(request, channel.readOutbound()); assertHttpRequestEquivalent(request, channel.readOutbound());
verify(metrics).requestSent(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, request); verify(metrics)
.requestSent(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, request.content().readableBytes());
verifyNoMoreInteractions(metrics); verifyNoMoreInteractions(metrics);
} }
@ -130,7 +131,8 @@ public class BackendMetricsHandlerTest {
assertThat(channel.writeInbound(response)).isTrue(); assertThat(channel.writeInbound(response)).isTrue();
assertHttpResponseEquivalent(response, channel.readInbound()); assertHttpResponseEquivalent(response, channel.readInbound());
verify(metrics).requestSent(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, request); verify(metrics)
.requestSent(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, request.content().readableBytes());
verify(metrics).responseReceived(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, response, 1); verify(metrics).responseReceived(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, response, 1);
verifyNoMoreInteractions(metrics); verifyNoMoreInteractions(metrics);
} }
@ -150,7 +152,8 @@ public class BackendMetricsHandlerTest {
assertThat(channel.writeInbound(response)).isTrue(); assertThat(channel.writeInbound(response)).isTrue();
assertHttpResponseEquivalent(response, channel.readInbound()); assertHttpResponseEquivalent(response, channel.readInbound());
verify(metrics).requestSent(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, request); verify(metrics)
.requestSent(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, request.content().readableBytes());
verify(metrics).responseReceived(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, response, 1); verify(metrics).responseReceived(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, response, 1);
verifyNoMoreInteractions(metrics); verifyNoMoreInteractions(metrics);
} }
@ -167,10 +170,10 @@ public class BackendMetricsHandlerTest {
public void testSuccess_pipelinedResponses() { public void testSuccess_pipelinedResponses() {
FullHttpRequest request1 = makeHttpPostRequest("request 1", HOST, "/"); FullHttpRequest request1 = makeHttpPostRequest("request 1", HOST, "/");
FullHttpResponse response1 = makeHttpResponse("response 1", HttpResponseStatus.OK); FullHttpResponse response1 = makeHttpResponse("response 1", HttpResponseStatus.OK);
FullHttpRequest request2 = makeHttpPostRequest("request 2", HOST, "/"); FullHttpRequest request2 = makeHttpPostRequest("request 22", HOST, "/");
FullHttpResponse response2 = makeHttpResponse("response 2", HttpResponseStatus.OK); FullHttpResponse response2 = makeHttpResponse("response 22", HttpResponseStatus.OK);
FullHttpRequest request3 = makeHttpPostRequest("request 3", HOST, "/"); FullHttpRequest request3 = makeHttpPostRequest("request 333", HOST, "/");
FullHttpResponse response3 = makeHttpResponse("response 3", HttpResponseStatus.OK); FullHttpResponse response3 = makeHttpResponse("response 333", HttpResponseStatus.OK);
// First request, time = 0 // First request, time = 0
assertThat(channel.writeOutbound(request1)).isTrue(); assertThat(channel.writeOutbound(request1)).isTrue();
@ -216,9 +219,12 @@ public class BackendMetricsHandlerTest {
long latency2 = new Duration(sentTime2, receivedTime2).getMillis(); long latency2 = new Duration(sentTime2, receivedTime2).getMillis();
long latency3 = new Duration(sentTime3, receivedTime3).getMillis(); long latency3 = new Duration(sentTime3, receivedTime3).getMillis();
verify(metrics).requestSent(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, request1); verify(metrics)
verify(metrics).requestSent(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, request2); .requestSent(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, request1.content().readableBytes());
verify(metrics).requestSent(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, request3); verify(metrics)
.requestSent(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, request2.content().readableBytes());
verify(metrics)
.requestSent(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, request3.content().readableBytes());
verify(metrics).responseReceived(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, response1, latency1); verify(metrics).responseReceived(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, response1, latency1);
verify(metrics).responseReceived(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, response2, latency2); verify(metrics).responseReceived(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, response2, latency2);
verify(metrics).responseReceived(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, response3, latency3); verify(metrics).responseReceived(RELAYED_PROTOCOL_NAME, CLIENT_CERT_HASH, response3, latency3);

8
javatests/google/registry/proxy/metric/BackendMetricsTest.java
View file

@ -47,7 +47,7 @@ public class BackendMetricsTest {
public void testSuccess_oneRequest() { public void testSuccess_oneRequest() {
String content = "some content"; String content = "some content";
FullHttpRequest request = makeHttpPostRequest(content, host, "/"); FullHttpRequest request = makeHttpPostRequest(content, host, "/");
metrics.requestSent(protocol, certHash, request); metrics.requestSent(protocol, certHash, request.content().readableBytes());
assertThat(BackendMetrics.requestsCounter) assertThat(BackendMetrics.requestsCounter)
.hasValueForLabels(1, protocol, certHash) .hasValueForLabels(1, protocol, certHash)
@ -68,8 +68,8 @@ public class BackendMetricsTest {
String content2 = "some other content"; String content2 = "some other content";
FullHttpRequest request1 = makeHttpPostRequest(content1, host, "/"); FullHttpRequest request1 = makeHttpPostRequest(content1, host, "/");
FullHttpRequest request2 = makeHttpPostRequest(content2, host, "/"); FullHttpRequest request2 = makeHttpPostRequest(content2, host, "/");
metrics.requestSent(protocol, certHash, request1); metrics.requestSent(protocol, certHash, request1.content().readableBytes());
metrics.requestSent(protocol, certHash, request2); metrics.requestSent(protocol, certHash, request2.content().readableBytes());
assertThat(BackendMetrics.requestsCounter) assertThat(BackendMetrics.requestsCounter)
.hasValueForLabels(2, protocol, certHash) .hasValueForLabels(2, protocol, certHash)
@ -146,7 +146,7 @@ public class BackendMetricsTest {
String responseContent = "the only response"; String responseContent = "the only response";
FullHttpRequest request = makeHttpPostRequest(requestContent, host, "/"); FullHttpRequest request = makeHttpPostRequest(requestContent, host, "/");
FullHttpResponse response = makeHttpResponse(responseContent, HttpResponseStatus.OK); FullHttpResponse response = makeHttpResponse(responseContent, HttpResponseStatus.OK);
metrics.requestSent(protocol, certHash, request); metrics.requestSent(protocol, certHash, request.content().readableBytes());
metrics.responseReceived(protocol, certHash, response, 10); metrics.responseReceived(protocol, certHash, response, 10);
assertThat(BackendMetrics.requestsCounter) assertThat(BackendMetrics.requestsCounter)