[chore][exporter/elasticsearch] Use RunParallel in benchmarks #33621
Conversation
exporter/elasticsearchexporter/integrationtest/exporter_bench_test.go
Outdated
Show resolved
Hide resolved
| type counters struct { | ||
| observedBulkRequests atomic.Int64 | ||
| } |
There was a problem hiding this comment.
Non-blocking nit: I would prefer an interface that allows overriding the bulk request counter rather than this similar to the data-provider.
So, something like func (es *esDataReceiver) SetBulkRequestCounter(c *atomic.Uint64) { es.bulkRequestCounter = c } with a default value set in newElasticsearchDataReceiver. The current interfaces kinda encourages adding more counters which I am a bit hesitant to do. Will leave it entirely up to you.
There was a problem hiding this comment.
The data provider has SetLoadGeneratorCounters to satisfy the DataProvider interface, while there isn't a ElasticsearchDataReceiver interface to satisfy. There is only 1 concrete implementation and no interface in our case. Given I can't think of any more counters to add at the moment, the extra code of SetBulkRequestCounter isn't offering any obvious benefits imo. We can revisit this when we find the need to make changes to esDataReceiver again.
There was a problem hiding this comment.
The data provider has SetLoadGeneratorCounters to satisfy the DataProvider interface, while there isn't a ElasticsearchDataReceiver interface to satisfy. There is only 1 concrete implementation and no interface in our case
That was just an example and I am not suggesting introducing/satisfying any interface here. Introducing a method like SetBulkRequestsCounter keeps the counters required by users in their code and prevents the counters struct leaking/sharing counters. IMO, the current implementation encourages setting more counters which I am not a fan of but no strong feelings about it.
We can revisit this when we find the need to make changes to esDataReceiver again.
SGTM!
Description:
This will be needed to measure performance of #32632