dsmiley commented on code in PR #1627: URL: https://github.com/apache/solr/pull/1627#discussion_r1211048052
########## solr/prometheus-exporter/src/java/org/apache/solr/prometheus/collector/MetricSamples.java: ########## @@ -19,24 +19,42 @@ import io.prometheus.client.Collector; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.stream.Collectors; public class MetricSamples { private final Map<String, Collector.MetricFamilySamples> samplesByMetricName; + private final Set<String> sampleMetricsCache; + public MetricSamples(Map<String, Collector.MetricFamilySamples> input) { samplesByMetricName = input; + sampleMetricsCache = new HashSet<>(); + for (Collector.MetricFamilySamples metricFamilySamples : input.values()) { + addSamplesToCache(metricFamilySamples); + } } public MetricSamples() { this(new HashMap<>()); } - public void addSamplesIfNotPresent(String metricName, Collector.MetricFamilySamples samples) { - samplesByMetricName.putIfAbsent(metricName, samples); + private void addSamplesToCache(Collector.MetricFamilySamples metricFamilySamples) { Review Comment: The diff was confusing to read here. I think it would be clearer if you move this method to *below* the method that calls it; not before. ########## solr/prometheus-exporter/src/java/org/apache/solr/prometheus/collector/MetricSamples.java: ########## @@ -19,24 +19,42 @@ import io.prometheus.client.Collector; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.stream.Collectors; public class MetricSamples { private final Map<String, Collector.MetricFamilySamples> samplesByMetricName; + private final Set<String> sampleMetricsCache; + public MetricSamples(Map<String, Collector.MetricFamilySamples> input) { samplesByMetricName = input; + sampleMetricsCache = new HashSet<>(); + for (Collector.MetricFamilySamples metricFamilySamples : input.values()) { + addSamplesToCache(metricFamilySamples); + } } public MetricSamples() { this(new HashMap<>()); } - public void addSamplesIfNotPresent(String metricName, Collector.MetricFamilySamples samples) { - samplesByMetricName.putIfAbsent(metricName, samples); + private void addSamplesToCache(Collector.MetricFamilySamples metricFamilySamples) { + for (Collector.MetricFamilySamples.Sample sample : metricFamilySamples.samples) { + sampleMetricsCache.add(sample.toString()); + } + } + + public void addSamplesIfNotPresent( + String metricName, Collector.MetricFamilySamples metricFamilySamples) { + if (!samplesByMetricName.containsKey(metricName)) { + samplesByMetricName.put(metricName, metricFamilySamples); Review Comment: There used to be a putIfAbsent call here, which is more elegant (and efficient) than a check and put. You can still use that; just rely on the return value of putIfAbsent being null to check if you then should call addSamplesToCache. If you think it's still unclear, you could add a comment. ########## solr/prometheus-exporter/src/java/org/apache/solr/prometheus/collector/MetricSamples.java: ########## @@ -47,7 +65,8 @@ public void addSampleIfMetricExists( return; } - if (!sampleFamily.samples.contains(sample)) { + if (!sampleMetricsCache.contains(sample.toString())) { + sampleMetricsCache.add(sample.toString()); Review Comment: Again, you could use a sampleMetricCache.add(sample.toString()) and look at the response boolean to know if it was added (wasn't already present). Is more efficient and moreover avoids a second sample.toString() call easily. Why is sampleMetricsCache using the toString of the Sample at all; why isn't it simply `Set<Sample>`? Also; sampleMetricsCache isn't a cache; it's not the ideal name/word for it. I would call it "seenSamples". -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org