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
