Add a new metric for OpenSearch Sink plugin: bulkRequestSizeBytes #572
Conversation
Signed-off-by: Han Jiang <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #572 +/- ##
=========================================
Coverage 92.28% 92.28%
Complexity 569 569
=========================================
Files 71 71
Lines 1725 1725
Branches 144 144
=========================================
Hits 1592 1592
Misses 102 102
Partials 31 31 Continue to review full report at Codecov.
|
| public OpenSearchSink(final PluginSetting pluginSetting) { | ||
| super(pluginSetting); | ||
| bulkRequestTimer = pluginMetrics.timer(BULKREQUEST_LATENCY); | ||
| bulkRequestErrorsCounter = pluginMetrics.counter(BULKREQUEST_ERRORS); | ||
| bulkRequestSizeBytes = pluginMetrics.gauge(BULKREQUEST_SIZE_BYTES, new AtomicLong()); |
There was a problem hiding this comment.
Is there a reason you use a guage here? It appears that you are only incrementing. If that is the case you should use a Counter
There was a problem hiding this comment.
I would recommend using a summary metric here. This is the metric type we have used in the HttpSource to measure payload size of the requests.
There was a problem hiding this comment.
Is there a reason you use a
guagehere? It appears that you are only incrementing. If that is the case you should use a Counter
I would recommend using a summary metric here. This is the metric type we have used in the HttpSource to measure payload size of the requests.
My understanding is counters simply add metrics together. It's hard to tell the different sizes for different bulks. Thus, I was finding a different mechanism. Now Chris' summary metric type seems to be the best. It help tell the sizes of individual bulks.
I have published the new revision using summary
| @@ -165,6 +170,7 @@ private void flushBatch(final BulkRequest bulkRequest) { | |||
| bulkRequestTimer.record(() -> { | |||
| try { | |||
| bulkRetryStrategy.execute(bulkRequest); | |||
| bulkRequestSizeBytes.addAndGet(bulkRequest.estimatedSizeInBytes()); | |||
There was a problem hiding this comment.
Can you explain why an AtomicLong is required here? Why wouldn't it be thread safe to just increment a Counter?
There was a problem hiding this comment.
I saw all other places using gauge also uses AtomicLong, and it seems to be safer since we do have many threads. Now we are using Summary, we don't have this concern any longer.
| public OpenSearchSink(final PluginSetting pluginSetting) { | ||
| super(pluginSetting); | ||
| bulkRequestTimer = pluginMetrics.timer(BULKREQUEST_LATENCY); | ||
| bulkRequestErrorsCounter = pluginMetrics.counter(BULKREQUEST_ERRORS); | ||
| bulkRequestSizeBytes = pluginMetrics.gauge(BULKREQUEST_SIZE_BYTES, new AtomicLong()); |
There was a problem hiding this comment.
I would recommend using a summary metric here. This is the metric type we have used in the HttpSource to measure payload size of the requests.
…stSizeBytes metrics Signed-off-by: Han Jiang <[email protected]>
Updated README. Thanks. |
…ensearch-project#572) * Add a new metric for OpenSearch Sink plugin: bulkRequestSizeBytes Signed-off-by: Han Jiang <[email protected]> * Using summary as the instrumenting mechanism for collecting bulkRequestSizeBytes metrics Signed-off-by: Han Jiang <[email protected]>
Signed-off-by: Han Jiang [email protected]
Description
Having the OpenSearch Sink emit a metric, bulkRequestSizeBytes, right before the request sending the batch to OpenSearch.
Issues Resolved
#571
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.