Change distribution value type to ByteString #14
Conversation
sming
left a comment
sming
left a comment
There was a problem hiding this comment.
with my limited understanding of the scope or strategic level, it LGTM :)
| ByteBuffer out = ByteBuffer.allocate(bytes.length); | ||
| out.put(bytes); | ||
| return out; | ||
| return ByteString.copyFrom(out.array()); |
There was a problem hiding this comment.
Seems unnecessary to go from byte[] -> ByteBuffer -> ByteString
Simplify to ByteString.copyFromUtf8(...)?
| byteBuffer.put(pointVal); | ||
| Metric metricV1 = (new Metric()).value(Value.distributionValue(byteBuffer)); | ||
| Metric metricV1 = (new Metric()).value(Value | ||
| .distributionValue(ByteString.copyFrom(byteBuffer.array()))); |
There was a problem hiding this comment.
Nit: ByteString.copyFrom(byteBuffer) might be cleaner, but perhaps you would then first need to flip the buffer
There was a problem hiding this comment.
Actually - just do ByteString.copyFromUtf8(DISTRIBUTION_DATA_POINT) instead
|
|
||
| public static Value distributionValue(ByteBuffer byteBuffer) { | ||
| return DistributionValue.create(byteBuffer); | ||
| public static Value distributionValue(ByteString byteString) { |
There was a problem hiding this comment.
This would be a breaking change, which I think we can live with if we are sure no one is depending on this.
Otherwise, we would need to keep the old method signature too
There was a problem hiding this comment.
No one is using it. Nothing will break.
4 participants
No description provided.