PrometheusTimeSeries performance fixes#6316
Conversation
Signed-off-by: Kondaka <krishkdk@amazon.com>
kkondaka
requested review from
KarstenSchnitter,
chenqi0805,
dinujoh,
dlvenable,
engechas,
graytaylor0,
oeyh,
san81,
sb2k16 and
srikanthjg
as code owners
| public List<TimeSeries> getTimeSeriesList() { | ||
| return timeSeriesList; | ||
| private int estimateLabelSize(String name, String value) { | ||
| return name.length() + value.length() + 8; // Approximate protobuf overhead |
There was a problem hiding this comment.
Make 8 a constant and name it APPROXIMATE_PROTOBUF_OVERHEAD.
| size += label.toByteArray().length; | ||
| Sample sample = Sample.newBuilder().setValue(sampleValue).setTimestamp(timestamp).build(); | ||
| size += sample.toByteArray().length; | ||
| size += estimateLabelSize(labelName, labelValue) + 16; // Sample overhead |
There was a problem hiding this comment.
Again, make 16 into a constant named SAMPLE_OVERHEAD. Better yet, is there a way to derive it as a constant?
| return timestamp; | ||
| } | ||
| public List<TimeSeries> getTimeSeriesList() { return timeSeriesList; } | ||
| public long getTimeStamp() { return timestamp; } |
There was a problem hiding this comment.
"Timestamp" is a single word, so this should be getTimestamp().
| public List<TimeSeries> getTimeSeriesList() { | ||
| return timeSeriesList; | ||
| private int estimateLabelSize(String name, String value) { | ||
| return name.length() + value.length() + 8; // Approximate protobuf overhead |
There was a problem hiding this comment.
Are you looking for length or size? This may not give you the actual value you are looking for.
I appreciate the desire to cut the extra buffer. But, the calculation may be incorrect. Could you add to the Remote.WriteRequest.Builder and get the value at that point to avoid double buffering?
There was a problem hiding this comment.
The 8 byte overhead includes on-wire overhead for labels for tags and length markers. Anyways, it would be approximate.
| Sample sample = Sample.newBuilder().setValue(sampleValue).setTimestamp(timestamp).build(); | ||
| size += sample.toByteArray().length; | ||
| final String labelValue, final Double sampleValue) { | ||
| size += estimateLabelSize(NAME_LABEL, metricName) + estimateLabelSize(labelName, labelValue) + 16; |
There was a problem hiding this comment.
What is 16 for? Similar to my other comments, make this a constant and give it a helpful name. e.g. INDIVIDUAL_TIME_SERIES_OVERHEAD.
|
These are good performance improvements. You should include a JMH test. See data-prepper-expression and http-source for examples of doing this. |
Signed-off-by: Kondaka <krishkdk@amazon.com>
|
@dlvenable will add JMH in a separate PR |
Signed-off-by: Kondaka <krishkdk@amazon.com>
* PrometheusTimeSeries performance fixes Signed-off-by: Kondaka <krishkdk@amazon.com> * Addressed review comments Signed-off-by: Kondaka <krishkdk@amazon.com> * Fixed checkStyle error Signed-off-by: Kondaka <krishkdk@amazon.com> --------- Signed-off-by: Kondaka <krishkdk@amazon.com>
* PrometheusTimeSeries performance fixes Signed-off-by: Kondaka <krishkdk@amazon.com> * Addressed review comments Signed-off-by: Kondaka <krishkdk@amazon.com> * Fixed checkStyle error Signed-off-by: Kondaka <krishkdk@amazon.com> --------- Signed-off-by: Kondaka <krishkdk@amazon.com> Signed-off-by: Nathan Wand <wandna@amazon.com>
3 participants
Description
Improved performance of PrometheusTimeSeries class by
creating new HashMaps each time
Also added negative credentials check integration test
Issues Resolved
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.