-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace the asyncProducer struct field in syncProducer with the AsyncProducer interface #2986
Comments
Proper Golang design is to prefer use of concrete data types. There is no reason why we should want to use the
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. |
Hi @puellanivis , thanks for your reply!
You are right! However, upon reviewing the existing implementation, it appears that there is no direct access to the
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 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
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. |
Thanks very much for your reply. I would find out a better way to do so. And let me close this issue. |
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
sarama/sync_producer.go
Lines 71 to 75 in 893978c
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
The text was updated successfully, but these errors were encountered: