Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move ownership of otelsarama #2510

Open
pellared opened this issue Jul 19, 2023 · 17 comments
Open

Move ownership of otelsarama #2510

pellared opened this issue Jul 19, 2023 · 17 comments

Comments

@pellared
Copy link

pellared commented Jul 19, 2023

Hello 馃憢

I am one of OpenTelemetry Go maintainers.

Recently I was reviewing open-telemetry/opentelemetry-go-contrib#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 than github.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?

@pellared
Copy link
Author

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.

@dnwe
Copy link
Collaborator

dnwe commented Jul 22, 2023

@pellared thanks for getting in touch and raising this issue. I hadn鈥檛 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鈥檛 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鈥檇 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鈥檇 be happy to learn from your expertise

@pellared
Copy link
Author

pellared commented Jul 24, 2023

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鈥檇 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).

@pellared
Copy link
Author

@dnwe Any heads-up?

@pellared
Copy link
Author

pellared commented Aug 8, 2023

@dnwe Bump 馃槈

@dnwe
Copy link
Collaborator

dnwe commented Aug 8, 2023

馃憢 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

@vincentfree
Copy link

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?

@dnwe
Copy link
Collaborator

dnwe commented Sep 11, 2023

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

@vincentfree
Copy link

vincentfree commented Sep 11, 2023

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 :)

@pellared
Copy link
Author

pellared commented Sep 12, 2023

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:

  1. github.com/IBM/sarama will not be coupled to OTel. It reduces the risk of potential unwanted changed.
  2. Seperate "maturity" of github.com/IBM/sarama and github.com/xyz/otelsarama. I think you could even start with publishing experimental versions like v0.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.
  3. It does not "close the door" for adding OTel as part of github.com/IBM/sarama in future.

@benclauss
Copy link

Any update here?

@geberl
Copy link

geberl commented Oct 12, 2023

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.

Copy link

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.

@github-actions github-actions bot added the stale Issues and pull requests without any recent activity label Jan 25, 2024
@pellared
Copy link
Author

@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?

@github-actions github-actions bot removed the stale Issues and pull requests without any recent activity label Jan 25, 2024
@dnwe
Copy link
Collaborator

dnwe commented Jan 25, 2024

@pellared yes that sounds like a good idea, thanks Robert

@pellared
Copy link
Author

I created open-telemetry/opentelemetry.io#3858. Do you want to close the issue or do you have some other plans?

Copy link

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.

@github-actions github-actions bot added stale Issues and pull requests without any recent activity and removed stale Issues and pull requests without any recent activity labels Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants