Uh oh!
There was an error while loading. Please reload this page.
Refactored M3 reporting to remove any unnecessary allocations - #77
Conversation
Introduced `ImmutableBuckets`;
Added additional test;
…eBuckets` immutable;
Tidying up;
| this.buckets = new ArrayList<>(); | ||
| } else { | ||
| this.buckets = new ArrayList<>(Arrays.asList(buckets)); | ||
| throw new IllegalArgumentException("provided buckets could not be null"); |
There was a problem hiding this comment.
confirming that this is is now ok because the Scope always provides non-null buckets because of #76, yeah?
| AbstractBuckets() { | ||
| this(null); | ||
| public void validate(T[] buckets) { |
| }; | ||
| } | ||
| Collections.sort(buckets); |
There was a problem hiding this comment.
again, confirming this is fine because of the validate above?
There was a problem hiding this comment.
This code is actually not used anymore
| String bucketIdFmt = String.format("%%0%sd", bucketIdLen); | ||
| BucketPair[] bucketPairs = BucketPairImpl.bucketPairs(buckets); | ||
| ImmutableMap.Builder<String, String> builder = new ImmutableMap.Builder<>(); |
There was a problem hiding this comment.
mind dropping a TODO here that this can be optimized to avoid repeated builder creations in the future?
There was a problem hiding this comment.
How do you want to optimize this? This Map is not static
There was a problem hiding this comment.
talked offline: we can address if it comes up in future performance profiles
prateek
left a comment
There was a problem hiding this comment.
LGTM save the comment nit
Subject;
NOTE: There's a breaking change:
DurationBucketsandLatencyBucketswill no longer be modifiable.Introduced
ImmutableBucketsinterface to eventually replacedBuckets;Rebased
HistogramImpl,M3ReporterontoImmutableBuckets