Enable queueing, retry, timeout for Kafka exporter #1455
Conversation
bogdandrutu
requested review from
dmitryax,
james-bebbington,
nilebox,
owais,
pjanotti and
tigrannajaryan
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1455 +/- ##
=======================================
Coverage 90.93% 90.93%
=======================================
Files 240 240
Lines 16718 16727 +9
=======================================
+ Hits 15202 15211 +9
Misses 1085 1085
Partials 431 431
Continue to review full report at Codecov.
|
| @@ -41,6 +41,8 @@ func newExporter(config Config, params component.ExporterCreateParams) (*kafkaPr | |||
| c.Producer.Return.Errors = true | |||
| // Wait only the local commit to succeed before responding. | |||
| c.Producer.RequiredAcks = sarama.WaitForLocal | |||
| // Because sarama does not accept a Context for every message, set the Timeout here. | |||
There was a problem hiding this comment.
not sure what you mean by a Context
There was a problem hiding this comment.
SendRequest does not accept a context.Context as an argument, so I cannot pass Timeout using the standard go way to do this via the context.Context
| }, | ||
| QueueSettings: exporterhelper.QueueSettings{ | ||
| Enabled: true, | ||
| NumConsumers: 2, |
There was a problem hiding this comment.
Should it? it is 2 here because that's what I put in the yaml.
There was a problem hiding this comment.
Looks good I haven't noticed that this is a test 😟
| MaxInterval: 1 * time.Minute, | ||
| MaxElapsedTime: 10 * time.Minute, | ||
| }, | ||
| QueueSettings: exporterhelper.QueueSettings{ |
There was a problem hiding this comment.
Are the default values based on something or just common sense (also for the retry) ?
There was a problem hiding this comment.
Just common sense. there is a documentation on how to configure the sending queue:
https://github.com/open-telemetry/opentelemetry-collector/blob/master/exporter/exporterhelper/queued_retry.go#L43
|
Could you please also update the readme with the new properties? |
|
@pavolloffay PTAL |
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
| - `timeout` (default = 5s): Is the timeout for every attempt to send data to the backend. | ||
| - `retry_on_failure` | ||
| - `enabled` (default = true) | ||
| - `initial_interval` (default = 5s): Time to wait after the first failure before retrying; ignored if `enabled` is `false` |
There was a problem hiding this comment.
Are the default values correct? Shouldn't we use the same defaults as the driver uses?
https://github.com/Shopify/sarama/blob/master/config.go#L462
There was a problem hiding this comment.
You pointed to the timeout this is the initial backoff delay after a request fail. We can tune the timeout to be 10sec if that is important, I would leave it as is for the moment and maybe collect feedback.
| - `max_elapsed_time` (default = 120s): Is the maximum amount of time spent trying to send a batch; ignored if `enabled` is `false` | ||
| - `sending_queue` | ||
| - `enabled` (default = false) | ||
| - `num_consumers` (default = 10): Number of consumers that dequeue batches; ignored if `enabled` is `false` |
There was a problem hiding this comment.
Does it mean that 10 instances of the exporter will be created?
There was a problem hiding this comment.
Just one instance, but will be 10 goroutines consuming items from the in-memory queue (similar with queue retry processor, but per exporter) and calling into the only one instance of the exporter created.
|
/cc @tigrannajaryan |
| exporterhelper.TimeoutSettings `mapstructure:",squash"` // squash ensures fields are correctly decoded in embedded struct. | ||
| exporterhelper.QueueSettings `mapstructure:"sending_queue"` | ||
| exporterhelper.RetrySettings `mapstructure:"retry_on_failure"` |
There was a problem hiding this comment.
Since all (?) of the exporters are going to need all 3 settings perhaps introduce a struct which embeds all 3 and use. Can be done in a future PR.
…lemetry#1455) * Bump github.com/stretchr/testify from 1.6.1 to 1.7.0 in /sdk Bumps [github.com/stretchr/testify](https://github.com/stretchr/testify) from 1.6.1 to 1.7.0. - [Release notes](https://github.com/stretchr/testify/releases) - [Commits](stretchr/testify@v1.6.1...v1.7.0) Signed-off-by: dependabot[bot] <support@github.com> * Auto-fix go.sum changes in dependent modules Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
…y#1455) Bumps [boto3](https://github.com/boto/boto3) from 1.21.35 to 1.21.38. - [Release notes](https://github.com/boto/boto3/releases) - [Changelog](https://github.com/boto/boto3/blob/develop/CHANGELOG.rst) - [Commits](boto/boto3@1.21.35...1.21.38) --- updated-dependencies: - dependency-name: boto3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
/cc @pavolloffay