Conversation
| 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(); | ||
| } | ||
| ConcurrentHashMap<String, String> labels = new ConcurrentHashMap<>(); | ||
| for (String pair : key.split(",")) { | ||
| String[] parts = pair.split("=", 2); | ||
| if (parts.length == 2) { | ||
| labels.put(parts[0], parts[1]); | ||
| } | ||
| } | ||
| return labels; | ||
| } |
There was a problem hiding this comment.
Following same logic as we have in node SDK.
| if (samples.isEmpty()) { | ||
| samples.add(new NumericMetricSample(Collections.emptyMap(), 0L)); | ||
| } |
There was a problem hiding this comment.
Populate with default value to at least have something.
| public enum MetricType { | ||
| COUNTER, | ||
| GAUGE, | ||
| HISTOGRAM | ||
| } |
There was a problem hiding this comment.
Preparing for gauge and histogram.
Pull Request Test Coverage Report for Build 20232798914Details
💛 - Coveralls |
| private final String name; | ||
| private final String help; | ||
| private final MetricType type; | ||
| private final List<NumericMetricSample> samples; |
There was a problem hiding this comment.
this will nor work for histograms
There was a problem hiding this comment.
they have BucketMetricSamples
There was a problem hiding this comment.
Yes histograms is different, but I will solve it when I get there.
| if (key == null || key.isEmpty()) { | ||
| return Collections.emptyMap(); | ||
| } | ||
| ConcurrentHashMap<String, String> labels = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
you're not sharing this so Map<String, String> labels = new HashMap<>(); would suffice.
| (key, value) -> { | ||
| samples.add(new NumericMetricSample(parseLabelKey(key), value)); | ||
| }); | ||
| values.clear(); |
There was a problem hiding this comment.
increments between forEach and clear can be lost. Java is not running single threaded event loop with run to completion guarantee as Node
There was a problem hiding this comment.
good point, fixing.
|
|
||
| CollectedMetric result = counter.collect(); | ||
|
|
||
| assertThat(result.getName()).isEqualTo("test_counter"); |
There was a problem hiding this comment.
looks like missing equals implementation on a result
|
|
||
| @Test | ||
| public void should_increment_by_default_value() { | ||
| CounterImpl counter = new CounterImpl("test_counter", "test help"); |
There was a problem hiding this comment.
it doesn't follow testing convention from node where we have registry, counter on a registry and registry verification
There was a problem hiding this comment.
we do not have registry yet.
There was a problem hiding this comment.
the test is showing behavior we don't want to expose as public. I think Node deliberately does not export CounterImpl
|
|
||
| assertThat(result.getSamples()).hasSize(3); | ||
|
|
||
| NumericMetricSample sampleAX = |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
The point was to check that things do not get bucketed in wrong places and provide real world example, with different buckets.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
e.g.
assertThat(result.getSamples())
.containsExactlyInAnyOrder(
sample(Map.of("a", "x"), 1L), sample(Map.of("b", "y"), 2L), sample(3L))
| public void should_parse_label_key_correctly() { | ||
| String key = "env=prod,region=us-east"; | ||
|
|
||
| Map<String, String> labels = CounterImpl.parseLabelKey(key); |
There was a problem hiding this comment.
this is private bahavior that should not be exposed and only tested transitively as part of the core behavior
There was a problem hiding this comment.
Node doesn't export parseLabelKey
| } | ||
|
|
||
| @Test | ||
| public void should_increment_with_custom_value() { |
There was a problem hiding this comment.
in Node we had one test for values and labels. keeping test structure similar make it easier to review them side by side
| 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); |
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class CounterImpl implements Counter { |
There was a problem hiding this comment.
if we want to keep it compliant with Node then it should not be public
4 participants
Add first iteration of impact metrics. Counters. Next up is gauges.