-
Notifications
You must be signed in to change notification settings - Fork 78
feat: impact metric counter #336
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 4 commits
7c602ee
5f9c581
79c6ded
deb6b19
3758b46
2ab676f
2e62df7
0981398
21d5f2a
94b2be4
025878b
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 |
|---|---|---|
| @@ -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; | ||
|
|
||
| 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; | ||
| } | ||
| } | ||
| 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); | ||
| } |
| 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 { | ||
|
Contributor
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. 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
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. 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
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 same logic as we have in node SDK. |
||
| } | ||
| 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
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. Preparing for gauge and histogram. |
||
| 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; | ||
| } | ||
| } |
| 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"); | ||
|
Contributor
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. it doesn't follow testing convention from node where we have registry, counter on a registry and registry verification
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. we do not have registry yet.
Contributor
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. 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"); | ||
|
Contributor
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. 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() { | ||
|
Contributor
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. 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 = | ||
|
Contributor
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. 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
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. The point was to check that things do not get bucketed in wrong places and provide real world example, with different buckets.
Contributor
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. 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
Contributor
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. 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
Contributor
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. e.g. |
||
| 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); | ||
|
Contributor
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. 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); | ||
|
Contributor
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. this is private bahavior that should not be exposed and only tested transitively as part of the core behavior
Contributor
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. Node doesn't export parseLabelKey |
||
|
|
||
| assertThat(labels).containsEntry("env", "prod").containsEntry("region", "us-east"); | ||
| } | ||
| } | ||
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.
this will nor work for histograms
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.
they have BucketMetricSamples
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.
Yes histograms is different, but I will solve it when I get there.