Skip to content

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.

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
sjaanus merged 3 commits into from
Dec 18, 2025
Merged

feat: start sending impact metrics in payload #341

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ecccf2e
feat: start sending impact metrics in payload
sjaanus Dec 18, 2025
62d26a2
Fix DI
sjaanus Dec 18, 2025
8c31733
Fix DI
sjaanus Dec 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/main/java/io/getunleash/metric/UnleashMetricServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,15 @@

import io.getunleash.engine.MetricsBucket;
import io.getunleash.engine.UnleashEngine;
import io.getunleash.impactmetrics.CollectedMetric;
import io.getunleash.impactmetrics.ImpactMetricsDataSource;
import io.getunleash.impactmetrics.InMemoryMetricRegistry;
import io.getunleash.util.Throttler;
import io.getunleash.util.UnleashConfig;
import io.getunleash.util.UnleashScheduledExecutor;
import java.time.LocalDateTime;
import java.time.ZoneId;
import java.util.List;
import java.util.Set;

public class UnleashMetricServiceImpl implements UnleashMetricService {
Expand All @@ -19,6 +23,8 @@ public class UnleashMetricServiceImpl implements UnleashMetricService {

private final Throttler throttler;

private final ImpactMetricsDataSource impactMetricsRegistry;

public UnleashMetricServiceImpl(
UnleashConfig unleashConfig, UnleashScheduledExecutor executor, UnleashEngine engine) {
this(
Expand All @@ -42,6 +48,9 @@ public UnleashMetricServiceImpl(
300,
unleashConfig.getUnleashURLs().getClientMetricsURL());
this.engine = engine;
ImpactMetricsDataSource configuredRegistry = unleashConfig.getImpactMetricsRegistry();
Copy link
Copy Markdown
Contributor

@kwasniew kwasniew Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the registry lives in the config and is not passed as other dependencies via constructor?

Copy link
Copy Markdown
Contributor Author

@sjaanus sjaanus Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ImpactMetricsDataSource is configurable, so it belongs in UnleashConfig, and UnleashMetricServiceImpl reads it from there (with a default fallback), just like it does for other configurable dependencies.

this.impactMetricsRegistry =
Copy link
Copy Markdown
Contributor

@kwasniew kwasniew Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a code smell (https://lab.abilian.com/Tech/Architecture%20%26%20Software%20Design/Dependency%20Inversion/DI%20anti-patterns/#bastard-injection-poor-mans-injection). as a user I don't know when I should pass the registry and when it will be created automatically.

Copy link
Copy Markdown
Contributor Author

@sjaanus sjaanus Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed it by following what we do with UnleashScheduledExecutorImpl

configuredRegistry != null ? configuredRegistry : new InMemoryMetricRegistry();
long metricsInterval = unleashConfig.getSendMetricsInterval();

executor.setInterval(sendMetrics(), metricsInterval, metricsInterval);
Expand All @@ -59,13 +68,21 @@ private Runnable sendMetrics() {
if (throttler.performAction()) {
MetricsBucket bucket = this.engine.getMetrics();

ClientMetrics metrics = new ClientMetrics(unleashConfig, bucket);
List<CollectedMetric> impactMetrics = impactMetricsRegistry.collect();
List<CollectedMetric> impactMetricsOrNull =
impactMetrics.isEmpty() ? null : impactMetrics;

ClientMetrics metrics =
new ClientMetrics(unleashConfig, bucket, impactMetricsOrNull);
Comment on lines +69 to +73
Copy link
Copy Markdown
Contributor Author

@sjaanus sjaanus Dec 18, 2025

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.

int statusCode = metricSender.sendMetrics(metrics);
if (statusCode >= 200 && statusCode < 400) {
throttler.decrementFailureCountAndResetSkips();
}
if (statusCode >= 400) {
throttler.handleHttpErrorCodes(statusCode);
if (impactMetricsOrNull != null) {
impactMetricsRegistry.restore(impactMetricsOrNull);
}
}

} else {
Expand Down
59 changes: 0 additions & 59 deletions src/test/java/io/getunleash/impactmetrics/MetricTypeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Copy link
Copy Markdown
Contributor Author

@sjaanus sjaanus Dec 18, 2025

Choose a reason for hiding this comment

The 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\":");
}
}
45 changes: 45 additions & 0 deletions src/test/java/io/getunleash/metric/DefaultHttpMetricsSenderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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()
Copy link
Copy Markdown
Contributor Author

@sjaanus sjaanus Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the java test style, we verify that +Inf is being sent.

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")));
}
}
127 changes: 127 additions & 0 deletions src/test/java/io/getunleash/metric/UnleashMetricServiceImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Copy link
Copy Markdown
Contributor Author

@sjaanus sjaanus Dec 18, 2025

Choose a reason for hiding this comment

The 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 =
Expand Down
Loading