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:

sarama/sync_producer.go

Lines 55 to 58 in 893978c

	type syncProducer struct {
	producer *asyncProducer
	wg sync.WaitGroup
	}

sarama/sync_producer.go

Lines 71 to 75 in 893978c

	p, err := NewAsyncProducer(addrs, config)
	if err != nil {
	return nil, err
	}
	return newSyncProducerFromAsyncProducer(p.(*asyncProducer)), nil

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.
alibaba/opentelemetry-go-auto-instrumentation#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.

[puellanivis]

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.

I suppose that you mean that we only ever call functions on the concrete data type. That is true. That’s also no reason to switch from the concrete data-type to the interface. Switching to the interface—as mentioned—imposes a devirtualization cost. One that is unnecessary, since we own both the asyncProducer and the syncProducer and thus a hidden implementation detail.

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.

I’m sorry to be blunt about this, but this is the risk you face by modifying library internals during or just before compilation. No one is under any obligation of ensuring that internal implementations remain compatible with on-the-fly patching. We are hiding this implementation detail for a reason, and if you’re piercing that veil, then you’re responsible for the complications that arise from it.

You’re breaking the rules by opening up the blackbox and tweaking implementation details, so you have to own the responsibility for compatibility.