-
Notifications
You must be signed in to change notification settings - Fork 78
feat: start sending impact metrics in payload #341
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,9 +2,6 @@ | |
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| import com.google.gson.Gson; | ||
| import com.google.gson.GsonBuilder; | ||
| import io.getunleash.util.HistogramBucketSerializer; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
@@ -303,60 +300,4 @@ private NumericMetricSample sample(long value) { | |
| private NumericMetricSample sample(Map<String, String> labels, long value) { | ||
| return new NumericMetricSample(labels, value); | ||
| } | ||
|
|
||
| @Test | ||
| public void should_serialize_histogram_with_positive_infinity_as_plus_inf_string() { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving this to http level, so we can catch also if serializer was not mounted. |
||
| InMemoryMetricRegistry registry = new InMemoryMetricRegistry(); | ||
| Histogram histogram = | ||
| registry.histogram( | ||
| new BucketMetricOptions( | ||
| "serialization_test", "test serialization", List.of(1.0, 5.0))); | ||
|
|
||
| histogram.observe(10.0, Map.of("env", "prod")); | ||
|
|
||
| List<CollectedMetric> metrics = registry.collect(); | ||
| assertThat(metrics).hasSize(1); | ||
|
|
||
| Gson gson = | ||
| new GsonBuilder() | ||
| .registerTypeAdapter(HistogramBucket.class, new HistogramBucketSerializer()) | ||
| .create(); | ||
|
|
||
| String json = gson.toJson(metrics.get(0)); | ||
|
|
||
| assertThat(json).contains("\"le\":\"+Inf\""); | ||
| assertThat(json).contains("\"le\":1.0"); | ||
| assertThat(json).contains("\"le\":5.0"); | ||
| } | ||
|
|
||
| @Test | ||
| public void should_serialize_histogram_buckets_correctly() { | ||
| InMemoryMetricRegistry registry = new InMemoryMetricRegistry(); | ||
| Histogram histogram = | ||
| registry.histogram( | ||
| new BucketMetricOptions( | ||
| "bucket_serialization", "test buckets", List.of(0.5, 2.0))); | ||
|
|
||
| histogram.observe(0.3); | ||
| histogram.observe(1.5); | ||
| histogram.observe(10.0); | ||
|
|
||
| List<CollectedMetric> metrics = registry.collect(); | ||
| assertThat(metrics).hasSize(1); | ||
|
|
||
| BucketMetricSample sample = (BucketMetricSample) metrics.get(0).getSamples().get(0); | ||
| assertThat(sample.getBuckets()).hasSize(3); | ||
|
|
||
| Gson gson = | ||
| new GsonBuilder() | ||
| .registerTypeAdapter(HistogramBucket.class, new HistogramBucketSerializer()) | ||
| .create(); | ||
|
|
||
| String json = gson.toJson(sample); | ||
|
|
||
| assertThat(json).contains("\"le\":0.5"); | ||
| assertThat(json).contains("\"le\":2.0"); | ||
| assertThat(json).contains("\"le\":\"+Inf\""); | ||
| assertThat(json).contains("\"count\":"); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,12 +14,17 @@ | |
|
|
||
| import com.github.tomakehurst.wiremock.junit5.WireMockExtension; | ||
| import io.getunleash.engine.MetricsBucket; | ||
| import io.getunleash.impactmetrics.BucketMetricOptions; | ||
| import io.getunleash.impactmetrics.CollectedMetric; | ||
| import io.getunleash.impactmetrics.Histogram; | ||
| import io.getunleash.impactmetrics.InMemoryMetricRegistry; | ||
| import io.getunleash.util.UnleashConfig; | ||
| import java.net.URI; | ||
| import java.net.URISyntaxException; | ||
| import java.time.Instant; | ||
| import java.time.LocalDateTime; | ||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.RegisterExtension; | ||
|
|
||
|
|
@@ -108,4 +113,44 @@ public void should_handle_service_failure_when_sending_metrics() throws URISynta | |
| .withHeader(UNLEASH_INTERVAL, matching(String.valueOf(metricsInterval))) | ||
| .withHeader("UNLEASH-APPNAME", matching("test-app"))); | ||
| } | ||
|
|
||
| @Test | ||
| public void should_send_impact_metrics_with_histogram_and_plus_inf_bucket() | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following the java test style, we verify that |
||
| throws URISyntaxException { | ||
| stubFor( | ||
| post(urlEqualTo("/client/metrics")) | ||
| .withHeader("UNLEASH-APPNAME", matching("test-app")) | ||
| .willReturn(aResponse().withStatus(200))); | ||
|
|
||
| URI uri = new URI("http://localhost:" + serverMock.getPort()); | ||
| UnleashConfig config = UnleashConfig.builder().appName("test-app").unleashAPI(uri).build(); | ||
|
|
||
| InMemoryMetricRegistry registry = new InMemoryMetricRegistry(); | ||
| Histogram histogram = | ||
| registry.histogram( | ||
| new BucketMetricOptions( | ||
| "test_histogram", "testing histogram", List.of(1.0, 5.0))); | ||
|
|
||
| histogram.observe(0.5); | ||
| histogram.observe(10.0); | ||
|
|
||
| List<CollectedMetric> impactMetrics = registry.collect(); | ||
|
|
||
| DefaultHttpMetricsSender sender = new DefaultHttpMetricsSender(config); | ||
| MetricsBucket bucket = new MetricsBucket(Instant.now(), Instant.now(), null); | ||
| ClientMetrics metrics = new ClientMetrics(config, bucket, impactMetrics); | ||
| sender.sendMetrics(metrics); | ||
|
|
||
| verify( | ||
| postRequestedFor(urlMatching("/client/metrics")) | ||
| .withRequestBody(matching(".*\"impactMetrics\".*")) | ||
| .withRequestBody(matching(".*\"name\"\\s*:\\s*\"test_histogram\".*")) | ||
| .withRequestBody(matching(".*\"type\"\\s*:\\s*\"HISTOGRAM\".*")) | ||
| .withRequestBody(matching(".*\"le\"\\s*:\\s*\"\\+Inf\".*")) | ||
| .withHeader( | ||
| UNLEASH_INTERVAL, matching(config.getSendMetricsIntervalMillis())) | ||
| .withHeader( | ||
| UNLEASH_CONNECTION_ID_HEADER, matching(config.getConnectionId())) | ||
| .withHeader("UNLEASH-APPNAME", matching("test-app"))); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,10 +8,14 @@ | |
| import io.getunleash.engine.UnleashEngine; | ||
| import io.getunleash.engine.YggdrasilError; | ||
| import io.getunleash.engine.YggdrasilInvalidInputException; | ||
| import io.getunleash.impactmetrics.CollectedMetric; | ||
| import io.getunleash.impactmetrics.ImpactMetricsDataSource; | ||
| import io.getunleash.util.UnleashConfig; | ||
| import io.getunleash.util.UnleashScheduledExecutor; | ||
| import java.time.LocalDateTime; | ||
| import java.util.Collections; | ||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Set; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.mockito.ArgumentCaptor; | ||
|
|
@@ -359,6 +363,129 @@ public void url_not_found_immediately_increases_interval_to_max() throws Yggdras | |
| assertThat(unleashMetricService.getSkips()).isEqualTo(0); | ||
| } | ||
|
|
||
| @Test | ||
| public void should_include_impact_metrics_in_clientmetrics_payload() throws YggdrasilError { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests are same as in node SDK. |
||
| UnleashConfig config = | ||
| UnleashConfig.builder() | ||
| .appName("test") | ||
| .sendMetricsInterval(10) | ||
| .unleashAPI("http://unleash.com") | ||
| .impactMetricsRegistry(mock(ImpactMetricsDataSource.class)) | ||
| .build(); | ||
|
|
||
| UnleashScheduledExecutor executor = mock(UnleashScheduledExecutor.class); | ||
| DefaultHttpMetricsSender sender = mock(DefaultHttpMetricsSender.class); | ||
| UnleashEngine engine = new UnleashEngine(); | ||
| ImpactMetricsDataSource registry = mock(ImpactMetricsDataSource.class); | ||
|
|
||
| CollectedMetric sample = | ||
| new CollectedMetric( | ||
| "feature_toggle_used", | ||
| "tracks toggle usage", | ||
| io.getunleash.impactmetrics.MetricType.COUNTER, | ||
| Collections.emptyList()); | ||
| when(registry.collect()).thenReturn(List.of(sample)); | ||
|
|
||
| UnleashConfig configWithRegistry = | ||
| UnleashConfig.builder() | ||
| .appName("test") | ||
| .sendMetricsInterval(10) | ||
| .unleashAPI("http://unleash.com") | ||
| .impactMetricsRegistry(registry) | ||
| .build(); | ||
|
|
||
| UnleashMetricServiceImpl unleashMetricService = | ||
| new UnleashMetricServiceImpl(configWithRegistry, sender, executor, engine); | ||
|
|
||
| ArgumentCaptor<Runnable> sendMetricsCallback = ArgumentCaptor.forClass(Runnable.class); | ||
| verify(executor).setInterval(sendMetricsCallback.capture(), anyLong(), anyLong()); | ||
|
|
||
| when(sender.sendMetrics(any(ClientMetrics.class))).thenReturn(200); | ||
|
|
||
| sendMetricsCallback.getValue().run(); | ||
|
|
||
| ArgumentCaptor<ClientMetrics> cmCaptor = ArgumentCaptor.forClass(ClientMetrics.class); | ||
| verify(sender).sendMetrics(cmCaptor.capture()); | ||
|
|
||
| ClientMetrics metricsSent = cmCaptor.getValue(); | ||
| assertThat(metricsSent.getImpactMetrics()).isNotNull(); | ||
| assertThat(metricsSent.getImpactMetrics()).hasSize(1); | ||
| assertThat(metricsSent.getImpactMetrics().get(0).getName()) | ||
| .isEqualTo("feature_toggle_used"); | ||
| } | ||
|
|
||
| @Test | ||
| public void should_restore_impact_metrics_on_failure() throws YggdrasilError { | ||
| ImpactMetricsDataSource registry = mock(ImpactMetricsDataSource.class); | ||
|
|
||
| CollectedMetric sample = | ||
| new CollectedMetric( | ||
| "feature_toggle_used", | ||
| "tracks toggle usage", | ||
| io.getunleash.impactmetrics.MetricType.COUNTER, | ||
| Collections.emptyList()); | ||
| when(registry.collect()).thenReturn(List.of(sample)); | ||
|
|
||
| UnleashConfig config = | ||
| UnleashConfig.builder() | ||
| .appName("test") | ||
| .sendMetricsInterval(10) | ||
| .unleashAPI("http://unleash.com") | ||
| .impactMetricsRegistry(registry) | ||
| .build(); | ||
|
|
||
| UnleashScheduledExecutor executor = mock(UnleashScheduledExecutor.class); | ||
| DefaultHttpMetricsSender sender = mock(DefaultHttpMetricsSender.class); | ||
| UnleashEngine engine = new UnleashEngine(); | ||
|
|
||
| UnleashMetricServiceImpl unleashMetricService = | ||
| new UnleashMetricServiceImpl(config, sender, executor, engine); | ||
|
|
||
| ArgumentCaptor<Runnable> sendMetricsCallback = ArgumentCaptor.forClass(Runnable.class); | ||
| verify(executor).setInterval(sendMetricsCallback.capture(), anyLong(), anyLong()); | ||
|
|
||
| when(sender.sendMetrics(any(ClientMetrics.class))).thenReturn(500); | ||
|
|
||
| sendMetricsCallback.getValue().run(); | ||
|
|
||
| verify(registry, times(1)).restore(List.of(sample)); | ||
| } | ||
|
|
||
| @Test | ||
| public void should_not_include_impact_metrics_field_when_empty() throws YggdrasilError { | ||
| ImpactMetricsDataSource registry = mock(ImpactMetricsDataSource.class); | ||
| when(registry.collect()).thenReturn(Collections.emptyList()); | ||
|
|
||
| UnleashConfig config = | ||
| UnleashConfig.builder() | ||
| .appName("test") | ||
| .sendMetricsInterval(10) | ||
| .unleashAPI("http://unleash.com") | ||
| .impactMetricsRegistry(registry) | ||
| .build(); | ||
|
|
||
| UnleashScheduledExecutor executor = mock(UnleashScheduledExecutor.class); | ||
| DefaultHttpMetricsSender sender = mock(DefaultHttpMetricsSender.class); | ||
| UnleashEngine engine = new UnleashEngine(); | ||
|
|
||
| UnleashMetricServiceImpl unleashMetricService = | ||
| new UnleashMetricServiceImpl(config, sender, executor, engine); | ||
|
|
||
| ArgumentCaptor<Runnable> sendMetricsCallback = ArgumentCaptor.forClass(Runnable.class); | ||
| verify(executor).setInterval(sendMetricsCallback.capture(), anyLong(), anyLong()); | ||
|
|
||
| when(sender.sendMetrics(any(ClientMetrics.class))).thenReturn(200); | ||
|
|
||
| sendMetricsCallback.getValue().run(); | ||
|
|
||
| ArgumentCaptor<ClientMetrics> cmCaptor = ArgumentCaptor.forClass(ClientMetrics.class); | ||
| verify(sender).sendMetrics(cmCaptor.capture()); | ||
|
|
||
| ClientMetrics metricsSent = cmCaptor.getValue(); | ||
| assertThat(metricsSent.getImpactMetrics()).isNull(); | ||
| verify(registry, times(1)).collect(); | ||
| } | ||
|
|
||
| @Test | ||
| public void should_add_new_metrics_data_to_bucket() { | ||
| UnleashConfig config = | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is design from node SDK, that if we do not have any metrics, we do not add the empty array to payload.