Closed nginthfs closed 8 months ago
Ah very useful metrics indeed! Thank you for adding this feature 😊
Several thoughts:
totalUploadedDocs
being passed as a mutable AtomicLong
is to have the outer logic to eventually be able to read it to print in the final json output. Therefore your proposed bytesUploaded
follows the same pattern which makes a lot of sense. However, if we are only going to use it for the prometheus metrics, may I suggest not passing that value around (Besides, I sometimes find it a bit hard to track mutable variables being passed into ctor/method, as there's a much bigger scope to track the mutation). Instead, we can change UploadDocs#call
to return the "result of the Upload operation", which can be the bytes uploaded. Then we no longer need to embed that metrics via the key
but read it from the "UploadDoc operation result". Perhaps we can introduce a new type (PrometheusUploadMetricsListener
?) of listener and use it at here that handles specifically Upload metrics as the existing PrometheusHttpRequestDurationListener
applies to both indexing and querying.UploadDocs#call
can instead return a class of UploadDocResult
, which contains 2 fields , bytesUploaded
and docsUploaded
, we can populate those in that method, and then PrometheusUploadMetricsListener#onExecutionComplete
should have both values available for reporting. The added benefit of returning a UploadDocResult
class is that it's much more readable than returning a generic long IMHO. 😊 +1, great idea! Will look into it in more detail tomorrow.
On Fri, 2 Feb, 2024, 1:01 am patsonluk, @.***> wrote:
Ah very useful metrics indeed! Thank you for adding this feature 😊
Several thoughts:
- My guess of the existing totalUploadedDocs being passed as a mutable AtomicLong is to have the outer logic to eventually be able to read it to print in the final json output. Therefore your proposed bytesUploaded follows the same pattern which makes a lot of sense. However, if we are only going to use it for the prometheus metrics, may I suggest not passing that value around (Besides, I sometimes find it a bit hard to track mutable variables being passed into ctor/method, as there's a much bigger scope to track the mutation). Instead, we can change UploadDocs#call to return the "result of the operation", which can be the bytes uploaded. Then we no longer need to embed that metrics via the key but read it from the "UploadDoc operation result". Perhaps we can introduce a new type (PrometheusUploadMetricsListener?) of listener and use it at here https://github.com/fullstorydev/solr-bench/blob/master/src/main/java/org/apache/solr/benchmarks/BenchmarksMain.java#L309 that handles specifically Upload metrics as the existing [PrometheusHttpRequestDurationListener] applies to both indexing and querying.
- Do you think it's helpful to report the # of docs uploaded via Prometheus as well? If we use the suggestion in 1., then UploadDocs#call can instead return a class of UploadDocResult, which contains 2 fields , bytesUploaded and docsUploaded, we can populate those in that method, and then PrometheusUploadMetricsListener#onExecutionComplete should have both values available for reporting. 😊
— Reply to this email directly, view it on GitHub https://github.com/fullstorydev/solr-bench/pull/91#issuecomment-1922078240, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDCR5FIGCJ2UWL3RNADO5LYRPUR7AVCNFSM6AAAAABCVLT5CCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRSGA3TQMRUGA . You are receiving this because you were assigned.Message ID: @.***>
Ah very useful metrics indeed! Thank you for adding this feature 😊
Several thoughts:
- My guess of the existing
totalUploadedDocs
being passed as a mutableAtomicLong
is to have the outer logic to eventually be able to read it to print in the final json output. Therefore your proposedbytesUploaded
follows the same pattern which makes a lot of sense. However, if we are only going to use it for the prometheus metrics, may I suggest not passing that value around (Besides, I sometimes find it a bit hard to track mutable variables being passed into ctor/method, as there's a much bigger scope to track the mutation). Instead, we can changeUploadDocs#call
to return the "result of the Upload operation", which can be the bytes uploaded. Then we no longer need to embed that metrics via thekey
but read it from the "UploadDoc operation result". Perhaps we can introduce a new type (PrometheusUploadMetricsListener
?) of listener and use it at here that handles specifically Upload metrics as the existingPrometheusHttpRequestDurationListener
applies to both indexing and querying.- Do you think it's helpful to report the # of docs uploaded via Prometheus as well? If we use the suggestion in 1., then
UploadDocs#call
can instead return a class ofUploadDocResult
, which contains 2 fields ,bytesUploaded
anddocsUploaded
, we can populate those in that method, and thenPrometheusUploadMetricsListener#onExecutionComplete
should have both values available for reporting. The added benefit of returning aUploadDocResult
class is that it's much more readable than returning a generic long IMHO. 😊
Background
We currently export metrics for indexing request counts and latencies, but we do not have a metric for throughput. This is important to accurately measure against real running Solr clusters.
Solution
This PR first adds the ability to register a Prometheus counter with the
PrometheusExportManager
. Next, it adds a Prometheus Counter metric calledsolr_bench_data_write_total
that represents the total number of bytes written to the Solr cluster viaUploadDocs
.