mlbiscoc commented on code in PR #1627:
URL: https://github.com/apache/solr/pull/1627#discussion_r1192578929
##########
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:
No problem! I am also still learning this code base as well but please ask
anything if it needs clarification.
Exactly, this PR is to address the fact that we are searching in `O(n)` but
we can change that to `O(1)`. Testing locally, I found significant performance
increase.
As for the `.toString` not including `exemplar`, I felt it was a non-issue
because when we scrape the metrics from Solr, the prometheus exporter creates
the sample object, but with `timestamp and exemplar = null`. So there will
never be a string with a `timestamp` or `exemplar`. You can see here ->
https://github.com/apache/solr/blob/7a31a1e23f6034c14639d7a208c5aaec64608cfd/solr/prometheus-exporter/src/java/org/apache/solr/prometheus/scraper/SolrScraper.java#L205
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
