diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/metrics/MetricsHttpEventHandler.java b/service/src/main/java/org/whispersystems/textsecuregcm/metrics/MetricsHttpEventHandler.java index 9ca1046fd..019a48572 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/metrics/MetricsHttpEventHandler.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/metrics/MetricsHttpEventHandler.java @@ -148,13 +148,18 @@ public class MetricsHttpEventHandler extends EventsHandler { tags.add(Tag.of(TRAFFIC_SOURCE_TAG, TrafficSource.HTTP.name().toLowerCase())); tags.addAll(UserAgentTagUtil.getLibsignalAndPlatformTags(requestInfo.userAgent)); + final Optional maybeClientVersionTag = + UserAgentTagUtil.getClientVersionTag(requestInfo.userAgent, clientReleaseManager); + + maybeClientVersionTag.ifPresent(tags::add); + meterRegistry.counter(REQUEST_COUNTER_NAME, tags).increment(); meterRegistry.counter(RESPONSE_BYTES_COUNTER_NAME, tags).increment(requestInfo.responseBytes); meterRegistry.counter(REQUEST_BYTES_COUNTER_NAME, tags).increment(requestInfo.requestBytes); - UserAgentTagUtil.getClientVersionTag(requestInfo.userAgent, clientReleaseManager).ifPresent( - clientVersionTag -> meterRegistry.counter(REQUESTS_BY_VERSION_COUNTER_NAME, - Tags.of(clientVersionTag, UserAgentTagUtil.getPlatformTag(requestInfo.userAgent))).increment()); + maybeClientVersionTag.ifPresent(clientVersionTag -> meterRegistry.counter(REQUESTS_BY_VERSION_COUNTER_NAME, + Tags.of(clientVersionTag, UserAgentTagUtil.getPlatformTag(requestInfo.userAgent))) + .increment()); } @Override diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/metrics/MetricsRequestEventListener.java b/service/src/main/java/org/whispersystems/textsecuregcm/metrics/MetricsRequestEventListener.java index 2d3c9b6cb..798c43601 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/metrics/MetricsRequestEventListener.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/metrics/MetricsRequestEventListener.java @@ -98,11 +98,16 @@ public class MetricsRequestEventListener implements RequestEventListener { @Nullable final String userAgent; { final List userAgentValues = event.getContainerRequest().getRequestHeader(HttpHeaders.USER_AGENT); - userAgent = userAgentValues != null && !userAgentValues.isEmpty() ? userAgentValues.get(0) : null; + userAgent = userAgentValues != null && !userAgentValues.isEmpty() ? userAgentValues.getFirst() : null; } tags.addAll(UserAgentTagUtil.getLibsignalAndPlatformTags(userAgent)); + final Optional maybeClientVersionTag = + UserAgentTagUtil.getClientVersionTag(userAgent, clientReleaseManager); + + maybeClientVersionTag.ifPresent(tags::add); + meterRegistry.counter(REQUEST_COUNTER_NAME, tags).increment(); Optional.ofNullable(event.getContainerRequest().getProperty(WebSocketResourceProvider.REQUEST_LENGTH_PROPERTY)) @@ -117,10 +122,9 @@ public class MetricsRequestEventListener implements RequestEventListener { .filter(bytes -> bytes >= 0) .ifPresent(bytes -> meterRegistry.counter(RESPONSE_BYTES_COUNTER_NAME, tags).increment(bytes)); - UserAgentTagUtil.getClientVersionTag(userAgent, clientReleaseManager) - .ifPresent(clientVersionTag -> meterRegistry.counter(REQUESTS_BY_VERSION_COUNTER_NAME, - Tags.of(clientVersionTag, UserAgentTagUtil.getPlatformTag(userAgent))) - .increment()); + maybeClientVersionTag.ifPresent(clientVersionTag -> meterRegistry.counter(REQUESTS_BY_VERSION_COUNTER_NAME, + Tags.of(clientVersionTag, UserAgentTagUtil.getPlatformTag(userAgent))) + .increment()); } } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/grpc/BackupsGrpcServiceTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/grpc/BackupsGrpcServiceTest.java index 107f249f6..0ad64ebd1 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/grpc/BackupsGrpcServiceTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/grpc/BackupsGrpcServiceTest.java @@ -30,7 +30,6 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; -import org.junitpioneer.jupiter.cartesian.CartesianTest; import org.mockito.Mock; import org.signal.chat.backup.BackupsGrpc; import org.signal.chat.backup.GetBackupAuthCredentialsRequest; diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/metrics/MetricsHttpEventHandlerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/metrics/MetricsHttpEventHandlerTest.java index b2a0479aa..8eff2cc18 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/metrics/MetricsHttpEventHandlerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/metrics/MetricsHttpEventHandlerTest.java @@ -36,6 +36,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; +import org.junitpioneer.jupiter.cartesian.CartesianTest; import org.mockito.ArgumentCaptor; import org.whispersystems.textsecuregcm.storage.ClientReleaseManager; @@ -75,10 +76,11 @@ class MetricsHttpEventHandlerTest { listener = new MetricsHttpEventHandler(null, meterRegistry, clientReleaseManager, Set.of("/test")); } - @ParameterizedTest - @ValueSource(booleans = {true, false}) + @CartesianTest @SuppressWarnings("unchecked") - void testRequests(boolean pathFromFilter) { + void testRequests(@CartesianTest.Values(booleans = {true, false}) final boolean pathFromFilter, + @CartesianTest.Values(booleans = {true, false}) final boolean versionActive) { + final String path = "/test"; final String method = "GET"; final int statusCode = 200; @@ -105,10 +107,11 @@ class MetricsHttpEventHandlerTest { }); } - final Response response = mock(Response.class); when(response.getStatus()).thenReturn(statusCode); + when(clientReleaseManager.isVersionActive(any(), any())).thenReturn(versionActive); + final ArgumentCaptor> tagCaptor = ArgumentCaptor.forClass(Iterable.class); listener.onRequestRead(request, Content.Chunk.from(ByteBuffer.allocate(512), true)); @@ -127,7 +130,7 @@ class MetricsHttpEventHandlerTest { tags.add(tag); } - assertEquals(6, tags.size()); + assertEquals(versionActive ? 7 : 6, tags.size()); assertTrue(tags.contains(Tag.of(MetricsHttpEventHandler.PATH_TAG, path))); assertTrue(tags.contains(Tag.of(MetricsHttpEventHandler.METHOD_TAG, method))); assertTrue(tags.contains(Tag.of(MetricsHttpEventHandler.STATUS_CODE_TAG, String.valueOf(statusCode)))); @@ -135,6 +138,7 @@ class MetricsHttpEventHandlerTest { tags.contains(Tag.of(MetricsHttpEventHandler.TRAFFIC_SOURCE_TAG, TrafficSource.HTTP.name().toLowerCase()))); assertTrue(tags.contains(Tag.of(UserAgentTagUtil.PLATFORM_TAG, "android"))); assertTrue(tags.contains(Tag.of(UserAgentTagUtil.LIBSIGNAL_TAG, "false"))); + assertEquals(versionActive, tags.contains(Tag.of(UserAgentTagUtil.VERSION_TAG, "6.53.7"))); } @ParameterizedTest diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/metrics/MetricsRequestEventListenerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/metrics/MetricsRequestEventListenerTest.java index 28b92584c..2784224ac 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/metrics/MetricsRequestEventListenerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/metrics/MetricsRequestEventListenerTest.java @@ -11,6 +11,8 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -65,6 +67,8 @@ class MetricsRequestEventListenerTest { private Counter counter; private Counter responseBytesCounter; private Counter requestBytesCounter; + private Counter requestsByVersionCounter; + private ClientReleaseManager clientReleaseManager; private MetricsRequestEventListener listener; private static final TrafficSource TRAFFIC_SOURCE = TrafficSource.HTTP; @@ -75,20 +79,22 @@ class MetricsRequestEventListenerTest { counter = mock(Counter.class); responseBytesCounter = mock(Counter.class); requestBytesCounter = mock(Counter.class); - - final ClientReleaseManager clientReleaseManager = mock(ClientReleaseManager.class); - when(clientReleaseManager.isVersionActive(any(), any())).thenReturn(false); + requestsByVersionCounter = mock(Counter.class); + clientReleaseManager = mock(ClientReleaseManager.class); listener = new MetricsRequestEventListener(TRAFFIC_SOURCE, meterRegistry, clientReleaseManager); } - @Test + @ParameterizedTest + @ValueSource(booleans = {true, false}) @SuppressWarnings("unchecked") - void testOnEvent() { + void testOnEvent(final boolean versionActive) { final String path = "/test"; final String method = "GET"; final int statusCode = 200; + when(clientReleaseManager.isVersionActive(any(), any())).thenReturn(versionActive); + final ExtendedUriInfo uriInfo = mock(ExtendedUriInfo.class); when(uriInfo.getMatchedTemplates()).thenReturn(Collections.singletonList(new UriTemplate(path))); @@ -115,6 +121,8 @@ class MetricsRequestEventListenerTest { .thenReturn(responseBytesCounter); when(meterRegistry.counter(eq(MetricsRequestEventListener.REQUEST_BYTES_COUNTER_NAME), any(Iterable.class))) .thenReturn(requestBytesCounter); + when(meterRegistry.counter(eq(MetricsRequestEventListener.REQUESTS_BY_VERSION_COUNTER_NAME), any(Iterable.class))) + .thenReturn(requestsByVersionCounter); listener.onEvent(event); @@ -122,6 +130,7 @@ class MetricsRequestEventListenerTest { verify(counter).increment(); verify(responseBytesCounter).increment(1024L); verify(requestBytesCounter).increment(512L); + verify(requestsByVersionCounter, versionActive ? times(1) : never()).increment(); final Iterable tagIterable = tagCaptor.getValue(); final Set tags = new HashSet<>(); @@ -130,7 +139,7 @@ class MetricsRequestEventListenerTest { tags.add(tag); } - assertEquals(7, tags.size()); + assertEquals(versionActive ? 8 : 7, tags.size()); assertTrue(tags.contains(Tag.of(MetricsRequestEventListener.PATH_TAG, path))); assertTrue(tags.contains(Tag.of(MetricsRequestEventListener.METHOD_TAG, method))); assertTrue(tags.contains(Tag.of(MetricsRequestEventListener.STATUS_CODE_TAG, String.valueOf(statusCode)))); @@ -138,6 +147,7 @@ class MetricsRequestEventListenerTest { assertTrue(tags.contains(Tag.of(MetricsRequestEventListener.AUTHENTICATED_TAG, "false"))); assertTrue(tags.contains(Tag.of(UserAgentTagUtil.PLATFORM_TAG, "android"))); assertTrue(tags.contains(Tag.of(UserAgentTagUtil.LIBSIGNAL_TAG, "true"))); + assertEquals(versionActive, tags.contains(Tag.of(UserAgentTagUtil.VERSION_TAG, "7.6.2"))); } @Test