Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
34 changes: 34 additions & 0 deletions src/main/java/io/getunleash/impactmetrics/CollectedMetric.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package io.getunleash.impactmetrics;

import java.util.List;

public class CollectedMetric {
private final String name;
private final String help;
private final MetricType type;
private final List<NumericMetricSample> samples;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this will nor work for histograms

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

they have BucketMetricSamples

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes histograms is different, but I will solve it when I get there.


public CollectedMetric(
String name, String help, MetricType type, List<NumericMetricSample> samples) {
this.name = name;
this.help = help;
this.type = type;
this.samples = samples;
}

public String getName() {
return name;
}

public String getHelp() {
return help;
}

public MetricType getType() {
return type;
}

public List<NumericMetricSample> getSamples() {
return samples;
}
}
11 changes: 11 additions & 0 deletions src/main/java/io/getunleash/impactmetrics/Counter.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package io.getunleash.impactmetrics;

import java.util.Map;

public interface Counter {
void inc();

void inc(long value);

void inc(long value, Map<String, String> labels);
}
77 changes: 77 additions & 0 deletions src/main/java/io/getunleash/impactmetrics/CounterImpl.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package io.getunleash.impactmetrics;

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.stream.Collectors;

public class CounterImpl implements Counter {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if we want to keep it compliant with Node then it should not be public

private final String name;
private final String help;
private final ConcurrentHashMap<String, Long> values = new ConcurrentHashMap<>();

public CounterImpl(String name, String help) {
this.name = name;
this.help = help;
}

@Override
public void inc() {
inc(1, null);
}

@Override
public void inc(long value) {
inc(value, null);
}

@Override
public void inc(long value, Map<String, String> labels) {
String key = getLabelKey(labels);
values.compute(key, (k, current) -> (current == null ? 0L : current) + value);
}

public CollectedMetric collect() {
List<NumericMetricSample> samples = new ArrayList<>();

for (String key : values.keySet()) {
Long value = values.remove(key);
if (value != null) {
samples.add(new NumericMetricSample(parseLabelKey(key), value));
}
}

if (samples.isEmpty()) {
samples.add(new NumericMetricSample(Collections.emptyMap(), 0L));
}
Comment on lines +47 to +49
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Populate with default value to at least have something.


return new CollectedMetric(name, help, MetricType.COUNTER, samples);
}

static String getLabelKey(Map<String, String> labels) {
if (labels == null || labels.isEmpty()) {
return "";
}
return labels.entrySet().stream()
.sorted(Map.Entry.comparingByKey())
.map(e -> e.getKey() + "=" + e.getValue())
.collect(Collectors.joining(","));
}

static Map<String, String> parseLabelKey(String key) {
if (key == null || key.isEmpty()) {
return Collections.emptyMap();
}
Map<String, String> labels = new HashMap<>();
for (String pair : key.split(",")) {
String[] parts = pair.split("=", 2);
if (parts.length == 2) {
labels.put(parts[0], parts[1]);
}
}
return labels;
}
Comment on lines +54 to +76
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Following same logic as we have in node SDK.

}
7 changes: 7 additions & 0 deletions src/main/java/io/getunleash/impactmetrics/MetricType.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package io.getunleash.impactmetrics;

public enum MetricType {
COUNTER,
GAUGE,
HISTOGRAM
}
Comment on lines +3 to +7
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Preparing for gauge and histogram.

21 changes: 21 additions & 0 deletions src/main/java/io/getunleash/impactmetrics/NumericMetricSample.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package io.getunleash.impactmetrics;

import java.util.Map;

public class NumericMetricSample {
private final Map<String, String> labels;
private final long value;

public NumericMetricSample(Map<String, String> labels, long value) {
this.labels = labels;
this.value = value;
}

public Map<String, String> getLabels() {
return labels;
}

public long getValue() {
return value;
}
}
130 changes: 130 additions & 0 deletions src/test/java/io/getunleash/impactmetrics/CounterImplTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
package io.getunleash.impactmetrics;

import static org.assertj.core.api.Assertions.assertThat;

import java.util.Map;
import org.junit.jupiter.api.Test;

public class CounterImplTest {

@Test
public void should_increment_by_default_value() {
CounterImpl counter = new CounterImpl("test_counter", "test help");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it doesn't follow testing convention from node where we have registry, counter on a registry and registry verification

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we do not have registry yet.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the test is showing behavior we don't want to expose as public. I think Node deliberately does not export CounterImpl


counter.inc();

CollectedMetric result = counter.collect();

assertThat(result.getName()).isEqualTo("test_counter");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks like missing equals implementation on a result

assertThat(result.getHelp()).isEqualTo("test help");
assertThat(result.getType()).isEqualTo(MetricType.COUNTER);
assertThat(result.getSamples()).hasSize(1);
assertThat(result.getSamples().get(0).getValue()).isEqualTo(1L);
assertThat(result.getSamples().get(0).getLabels()).isEmpty();
}

@Test
public void should_increment_with_custom_value() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in Node we had one test for values and labels. keeping test structure similar make it easier to review them side by side

CounterImpl counter = new CounterImpl("test_counter", "test help");

counter.inc(5);
counter.inc(3);

CollectedMetric result = counter.collect();

assertThat(result.getSamples()).hasSize(1);
assertThat(result.getSamples().get(0).getValue()).isEqualTo(8L);
}

@Test
public void should_increment_with_labels() {
CounterImpl counter = new CounterImpl("labeled_counter", "with labels");

counter.inc(3, Map.of("foo", "bar"));
counter.inc(2, Map.of("foo", "bar"));

CollectedMetric result = counter.collect();

assertThat(result.getSamples()).hasSize(1);
assertThat(result.getSamples().get(0).getValue()).isEqualTo(5L);
assertThat(result.getSamples().get(0).getLabels()).containsEntry("foo", "bar");
}

@Test
public void should_store_different_label_combinations_separately() {
CounterImpl counter = new CounterImpl("multi_label", "label test");

counter.inc(1, Map.of("a", "x"));
counter.inc(2, Map.of("b", "y"));
counter.inc(3);

CollectedMetric result = counter.collect();

assertThat(result.getSamples()).hasSize(3);

NumericMetricSample sampleAX =
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this logic it too complex for unit tests that should be declarative. we should strive for a single assertion as in node. prbbaly another missing equals implementation leaking complexity to the unit test

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The point was to check that things do not get bucketed in wrong places and provide real world example, with different buckets.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sure, I agree with the intent. My point is that we're still missing equals and hashCode on NumericMetricSample and CollectedMetric that would make some of the checks easier to parse

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what we do as toMatchObject in Node for value object equality checks is typically implemented in equals methods in Java and tests don't need to dissect every single field in a separate assertion

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

e.g.

        assertThat(result.getSamples())
                .containsExactlyInAnyOrder(
                        sample(Map.of("a", "x"), 1L), sample(Map.of("b", "y"), 2L), sample(3L))

result.getSamples().stream()
.filter(s -> s.getLabels().containsKey("a"))
.findFirst()
.orElseThrow();
NumericMetricSample sampleBY =
result.getSamples().stream()
.filter(s -> s.getLabels().containsKey("b"))
.findFirst()
.orElseThrow();
NumericMetricSample sampleEmpty =
result.getSamples().stream()
.filter(s -> s.getLabels().isEmpty())
.findFirst()
.orElseThrow();

assertThat(sampleAX.getValue()).isEqualTo(1L);
assertThat(sampleBY.getValue()).isEqualTo(2L);
assertThat(sampleEmpty.getValue()).isEqualTo(3L);
}

@Test
public void should_return_zero_value_when_empty() {
CounterImpl counter = new CounterImpl("empty_counter", "empty");

CollectedMetric result = counter.collect();

assertThat(result.getSamples()).hasSize(1);
assertThat(result.getSamples().get(0).getValue()).isEqualTo(0L);
assertThat(result.getSamples().get(0).getLabels()).isEmpty();
}

@Test
public void should_reset_after_collect() {
CounterImpl counter = new CounterImpl("reset_test", "reset");

counter.inc(5);
CollectedMetric first = counter.collect();
assertThat(first.getSamples().get(0).getValue()).isEqualTo(5L);

CollectedMetric second = counter.collect();
assertThat(second.getSamples()).hasSize(1);
assertThat(second.getSamples().get(0).getValue()).isEqualTo(0L);
}

@Test
public void should_generate_stable_label_key() {
Map<String, String> labels1 = Map.of("z", "1", "a", "2");
Map<String, String> labels2 = Map.of("a", "2", "z", "1");

String key1 = CounterImpl.getLabelKey(labels1);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as the next comment

String key2 = CounterImpl.getLabelKey(labels2);

assertThat(key1).isEqualTo(key2);
assertThat(key1).isEqualTo("a=2,z=1");
}

@Test
public void should_parse_label_key_correctly() {
String key = "env=prod,region=us-east";

Map<String, String> labels = CounterImpl.parseLabelKey(key);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is private bahavior that should not be exposed and only tested transitively as part of the core behavior

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Node doesn't export parseLabelKey


assertThat(labels).containsEntry("env", "prod").containsEntry("region", "us-east");
}
}
Loading