sarama
sarama copied to clipboard
Move ownership of otelsarama
Hello 👋
I am one of OpenTelemetry Go maintainers.
Recently I was reviewing https://github.com/open-telemetry/opentelemetry-go-contrib/pull/4090 and I started to think that it may be more beneficial for the community if the OpenTelemetry instrumentation would be moved to this repository or organization.
You can even consider instrumenting with OpenTelemetry "natively" (without requiring the users to use an instrumentation library) or just moing the otelsarama
instrumentation library to this repository or to a new one (e.g. github.com/IBM/otelsarama
).
The reasons are the following
- the import path has changed https://github.com/IBM/sarama/releases/tag/v1.40.0 so basically from Go semantics
github.com/IBM/sarama
is a different module thangithub.com/Shopify/sarama
- this is currently a more preferred way according to our recommendation
- in general, OTel maintainers and approvers are on low capacity
Do you have any opinions?
The alternative way would be that we will deprecate go.opentelemetry.io/contrib/instrumentation/github.com/Shopify/sarama/otelsarama
and create go.opentelemetry.io/contrib/instrumentation/github.com/IBM/sarama/otelsarama
. This is for us the least preferred recommendation (number 3), but we will probably go for if you will not want to follow our "number 1" or "number 2" recommendation.
@pellared thanks for getting in touch and raising this issue. I hadn’t previously been aware of the sarama instrumentation wrappers in the opentelemetry contrib repo and will explore them. Do you have any metrics or estimate as to the amount of user interest and takeup?
I certainly wouldn’t be against in principal nominally taking on a maintainer role for the instrumentation. However, if we were grabbing the code as-is, the existing license mismatch (MIT vs Apache) would probably steer it towards being maintained in a separate repo rather than being imported, just for a clean separation on that. We’d also need to determine how best to regression test going forward.
It might be worth going ahead with your option 3, to provide a short term solution for the existing user base who need to pick-up sarama under the new import path today, and then we can continue to talk about how best to proceed in the long term
As an aside, sarama is long overdue a metrics overhaul from the existing rcrowley/go-metrics system, so if you have a recommended approach there I’d be happy to learn from your expertise
Do you have any metrics or estimate as to the amount of user interest and takeup?
In open source there we can find some usage here: https://pkg.go.dev/go.opentelemetry.io/contrib/instrumentation/github.com/Shopify/sarama/otelsarama?tab=importedby. The usage is for sure larger as the package is used transitively via watermill, see:. https://pkg.go.dev/github.com/ThreeDotsLabs/watermill-kafka/v2/pkg/kafka?tab=importedby
At Splunk, according to our telemetry, otelsarama
is the thrid (sic!) most popular instrumentation library (after otelhttp
and otelgrpc
) used by our customers.
It might be worth going ahead with your option 3, to provide a short term solution for the existing user base who need to pick-up sarama under the new import path today, and then we can continue to talk about how best to proceed in the long term
We may do that. However, right now, we are not planning any release in upcoming days. We can calmly continue talking 😉
It would be perfect if you find some time to review the instrumentation library (I think it is OK if it takes even 2 weeks) so that we can discuss, brainstorm, PoC different options. If it helps, I can still help maintaining the instrumentation after it is being moved outside OpenTelemetry. I think that having the library maintained by both sarama and OTel maintainers can have a positive impact on the libraries. If you want, we can also arrange some meeting (e.g. via Zoom). As OTel Go community, we also have a weekly meeting and you can reach us on Slack (see: https://github.com/open-telemetry/opentelemetry-go/blob/main/CONTRIBUTING.md).
As an aside, sarama is long overdue a metrics overhaul from the existing rcrowley/go-metrics system, so if you have a recommended approach there I’d be happy to learn from your expertise
Wouldn't changing the existing rcrowley/go-metrics integration be seen as breaking change for the existing users? One of the benefits of option 1 is that you could use https://pkg.go.dev/go.opentelemetry.io/otel/metric instead of https://pkg.go.dev/github.com/rcrowley/go-metrics. OpenTelemetry is also creating the semantic conventions for metrics related to messaging systems as well as Kafka.
would probably steer it towards being maintained in a separate repo
The reason why option 1 may be worth exploring is that metrics support my be added via https://pkg.go.dev/go.opentelemetry.io/otel/metric. I have not checked if all the metrics can determined externally (I mean if the instrumentation can be added via a separate package).
@dnwe Any heads-up?
@dnwe Bump 😉
👋 I do still plan to look into this, but with vacation and work I just haven't had any free cycles yet. I should hopefully find some time soon
Any update on this issue? I want update the imports for multiple applications I'm maintaining but I'm also a user of otelsarama
so I'd like to know if I have to temporarily plug this issue or if I can expect something from IBM or otel?
I'd started to explore the source code and was prototyping how it might be managed/maintained here. I need to become more familiar with running and using OpenTelemetry myself
I'd started to explore the source code and was prototyping how it might be managed/maintained here. I need to become more familiar with running and using OpenTelemetry myself
It already seems quite functional. It seems some of the ideas of the initial otelsarama
carryover so that would make it easy on current users. If it's going to be a external(from the actual project) dependency than the wrapper approach makes a lot of sense.
Though the other options discussed by @pellared might also offer a way to leverage traces and metrics with one approach. You could even relate traces to your structured logs as well for even better relations between the signals.
But all in due time I guess :)
Personally, I think it will be safer and more pragmatic for sarama maintainers to first have it as a separate repository.
The reasons are following:
-
github.com/IBM/sarama
will not be coupled to OTel. It reduces the risk of potential unwanted changed. - Seperate "maturity" of
github.com/IBM/sarama
andgithub.com/xyz/otelsarama
. I think you could even start with publishing experimental versions likev0.1.0
. The users who badly want OTel integration could already profit. At the same time you will have a chance to get some feedback and experience in OTel. Once you create some initial experimental version it would be good to add it to the OTel registry. - It does not "close the door" for adding OTel as part of
github.com/IBM/sarama
in future.
Any update here?
There is now https://github.com/dnwe/otelsarama (not in the IBM namespace, but maintained by dnwe, the guy who for IBM maintains sarama). I think this is as good as it will get. So I just use this.
Thank you for taking the time to raise this issue. However, it has not had any activity on it in the past 90 days and will be closed in 30 days if no updates occur. Please check if the main branch has already resolved the issue since it was raised. If you believe the issue is still valid and you would like input from the maintainers then please comment to ask for it to be reviewed.
@dnwe Do you want me to add an entry for IBM/sarama here https://opentelemetry.io/ecosystem/registry/?s=sarama linking to https://github.com/dnwe/otelsarama?
@pellared yes that sounds like a good idea, thanks Robert
I created https://github.com/open-telemetry/opentelemetry.io/pull/3858. Do you want to close the issue or do you have some other plans?
Thank you for taking the time to raise this issue. However, it has not had any activity on it in the past 90 days and will be closed in 30 days if no updates occur. Please check if the main branch has already resolved the issue since it was raised. If you believe the issue is still valid and you would like input from the maintainers then please comment to ask for it to be reviewed.
Thank you for taking the time to raise this issue. However, it has not had any activity on it in the past 90 days and will be closed in 30 days if no updates occur. Please check if the main branch has already resolved the issue since it was raised. If you believe the issue is still valid and you would like input from the maintainers then please comment to ask for it to be reviewed.