[exporter/kafkaexporter] ExporterFactory log/metric marshaler options #13433
Conversation
Adding WithMetricsMarshalers/WithLogMarshalers options to parallel WithTracesMarshalers
|
pinging @pavolloffay @MovieStoreGuy as code owners |
…:bgranetzke/opentelemetry-collector-contrib into exporter/kafkaexporter_marshaler_options
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
@MovieStoreGuy What do we want to do here? I'd prefer not to fork just go get these factory options. |
|
@bgranetzke is there an open issue for this change? |
|
@MovieStoreGuy No, I don't see an open issue for this specifically, however, the capability to add custom marshalers would empower these features to progress without forking: open closed-no solution There are also the three PRs mentioned in the description. I can create a new issue if you would like. |
There was a problem hiding this comment.
I am sorry but I am not convinced on the design at present.
The concern I have here is embedding the SaramaProducer function as part of the kafkaExporterFactory struct would result in nil reference if not correctly set.
I am also unsure how this would improve user's usage of the exporter.
If you can raise an issue to discuss the design you're intending to achieve, I would be more than happy to spare on that with you.
| @@ -107,10 +132,13 @@ func createDefaultConfig() config.Exporter { | |||
| } | |||
| } | |||
|
|
|||
| type ProducerFactoryFunc func(config Config) (sarama.SyncProducer, error) | |||
There was a problem hiding this comment.
Comment exported types for their intended use.
| // WithProducerFactory sets an alternative producer factory. | ||
| func WithProducerFactory(producerFactory ProducerFactoryFunc) FactoryOption { | ||
| return func(factory *kafkaExporterFactory) { | ||
| factory.producerFactory = producerFactory | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Could you expand upon why you would need an alternative producer factory, it isn't clear to me why you would need this and why you'd want to use this.
There was a problem hiding this comment.
It was only to test the custom marshaler option. I did not like it either, but due to how the actual factory is wrapped inside the component.ExporterFactory, I had to inject the mock during NewFactory(). I didn't want to create kafkaExporterFactory directly because then if felt like I wasn't testing the new options I was adding.
Totally open to an alternative approach.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closing as this PR is replaced by #16540 |
[exporter/kafkaexporter] ExporterFactory log/metric marshaler options
Description:
Adding ExportFactory options to add custom log/metric marshalers (similar to existing: WithTracesMarshalers)
This is a reboot of this PR. It eventually went stale and closed. The outstanding issue was related to improving the existing test for WithTracesMarshalers and adding similar to the new options. After struggling with struct/func visibility constraints, I ended up adding yet another FactoryOption which allowed me to inject a mock sarama.SyncProducer to verify the messages produced by the configured marshaler. The change to support the test was much more invasive than I expected and am open to alternatives.
There are also other related PRs which also went stale that I think could benefit from the new options:
#8336
#8272