Replace the asyncProducer struct field in syncProducer with the AsyncProducer interface

[Cirilla-zmh]

Description

Hi everyone,

I am currently working on providing compile-time auto-instrumentation for IBM/sarama library. While compiling Go applications, my tools aim to match functions in libraries based on predefined rules, allowing us to instrument them with generated code. This enables us to create spans and add trace context to messages, thereby enhancing observability for applications.

But I’ve encountered some challenges while implementing the instrumentation for IBM/sarama. In the current design, both SyncProducer and AsyncProducer are based on the asyncProducer struct. Theoretically, this means I should only need to wrap asyncProducer with a proxy, similar to the implementation found in otelsarama.

You can view that implementation here:

OtelSaram Implementation

However, in the definition of syncProducer, there's a direct reference to an instance of the asyncProducer struct instead of the AsyncProducer interface, and the return value of NewAsyncProducer() is cast to the asyncProducer type. This design choice restricts us from wrapping the producer effectively.

See relevant code here:

https://github.com/IBM/sarama/blob/893978c87fe7af13a2b6849ba62b003493f97f25/sync_producer.go#L55-L58 https://github.com/IBM/sarama/blob/893978c87fe7af13a2b6849ba62b003493f97f25/sync_producer.go#L71-L75

After reviewing the code, I believe that having a struct field instead of an interface in syncProducer is unnecessary. This decision seems to create an unwarranted coupling between the two producers. An AsyncProducer interface field may serve as a better alternative.

Additional context

You can find more informations about our project here. https://github.com/alibaba/opentelemetry-go-auto-instrumentation/issues/92

[puellanivis]

Proper Golang design is to prefer use of concrete data types. There is no reason why we should want to use the AsyncProducer interface internally over the concrete data type, which we know it is. This would only add overhead to everyone devirtualizing the AsyncProducer interface to access to the (again, we know) concrete asyncProducer struct.

This decision seems to create an unwarranted coupling between the two producers.

This is not an unwarranted coupling. It is a known coupling. The two are coupled. And for good reason.

PS: That they are coupled is an implementation detail, which should not concern callers in general.

[Cirilla-zmh]

Hi @puellanivis , thanks for your reply!

This would only add overhead to everyone devirtualizing the AsyncProducer interface to access to the (again, we know) concrete asyncProducer struct.

You are right! However, upon reviewing the existing implementation, it appears that there is no direct access to the asyncProducer instance within the syncProducer. I would like to explore the possibility of decoupling these components.

PS: That they are coupled is an implementation detail, which should not concern callers in general.

As a user of the library, I can see that the current design works well in most scenarios. However, in cases involving compile-time instrumentation, this design introduces challenges. Specifically, we will be adding code to the library during compilation, and the coupling of the producers may complicate the maintenance of the instrumentation. Additionally, if sarama introduces any type casting in future versions, it could render the instrumentation ineffective.