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.

Sign up for GitHub

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

Jump to bottom

Migration from github.com/Shopify/sarama to github.com/IBM/sarama #4090

Closed
wants to merge 3 commits into from
Closed

Migration from github.com/Shopify/sarama to github.com/IBM/sarama #4090

wants to merge 3 commits into from

Conversation

batazor
Copy link

@batazor batazor commented Jul 17, 2023 •
edited

Need to change the URL to sarama.
Changelog: https://github.com/IBM/sarama/releases/tag/v1.40.0

@batazor batazor requested review from a team, pellared and hanyuancheung as code owners July 17, 2023 21:18
@batazor batazor force-pushed the main branch 2 times, most recently from e7f126d to 9ba65fc Compare July 17, 2023 21:47
@batazor
Copy link
Author

batazor commented Jul 17, 2023

I think I've updated all the necessary places.

MrAlias
MrAlias requested changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unclear we would want to change our package name to match the upstream redirect. If we did end up wanting to do so, it will require a deprecation of the existing package.

@pellared
Copy link
Member

pellared commented Jul 18, 2023 •
edited

@batazor Can the new instrumentation library be located somewhere in https://github.com/IBM/sarama or in a new repository e.g. https://github.com/IBM/otelsarama?

@batazor
Copy link
Author

batazor commented Jul 18, 2023

@MrAlias I left only changing the path to the dependency, don't change the path in this package, this will save the working state of observability

@pellared The library has a new main contributor, I think we can ask him about that.

@batazor
Migration from github.com/Shopify/sarama to github.com/IBM/sarama
0fdc289 
Signed-off-by: Victor Login <batazor@evrone.com>
@batazor batazor requested a review from MrAlias July 18, 2023 08:58
MrAlias
MrAlias reviewed Jul 18, 2023
View reviewed changes
CODEOWNERS Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
batazor and others added 2 commits July 18, 2023 21:13
@batazor @MrAlias
Update CODEOWNERS
476e6d7 
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@batazor @MrAlias
Update CHANGELOG.md
83d374e 
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@batazor batazor requested a review from MrAlias July 18, 2023 16:14
@dmathieu dmathieu mentioned this pull request Jul 19, 2023
Closed
@pellared pellared mentioned this pull request Jul 19, 2023
Open
@pellared
Copy link
Member

I created IBM/sarama#2510.

@pellared
Copy link
Member

It is unclear we would want to change our package name to match the upstream redirect. If we did end up wanting to do so, it will require a deprecation of the existing package.

I left only changing the path to the dependency, don't change the path in this package, this will save the working state of observability

I would rather change the package name to match upstream redirect as the users already need to change the import paths for the library 😉

@dmathieu
Copy link
Member

I agree that if we end up keeping this instrumentation, we should rename the package as well.
Ideally, now would be a good time to move it into a better location though.

pellared
pellared requested changes Jul 19, 2023
View reviewed changes
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking mainly because of #4090 (comment) but also awaiting feedback under IBM/sarama#2510

@MrAlias
Copy link
Contributor

MrAlias commented Jul 19, 2023

I don't think the question of renaming this package should block the update of the dependency.

Aneurysm9
Aneurysm9 reviewed Jul 19, 2023
View reviewed changes
Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it's appropriate to simply change the dependency here. Won't that break anyone still using the prior import paths who updates this instrumentation? Should we deprecate this instrumentation? New instrumentation for the new import path can be created, ideally owned by the sarama maintainers, but anywhere really.

instrumentation/README.md
Comment on lines +43 to +61
| Instrumentation Package | Metrics | Traces |
|:----------------------------------------------------------------------------------------:|:-------:|:------:|
| [github.com/astaxie/beego](./github.com/astaxie/beego/otelbeego) | ✓ | ✓ |
| [github.com/aws/aws-sdk-go-v2](./github.com/aws/aws-sdk-go-v2/otelaws) | | ✓ |
| [github.com/bradfitz/gomemcache](./github.com/bradfitz/gomemcache/memcache/otelmemcache) | | ✓ |
| [github.com/emicklei/go-restful](./github.com/emicklei/go-restful/otelrestful) | | ✓ |
| [github.com/gin-gonic/gin](./github.com/gin-gonic/gin/otelgin) | | ✓ |
| [github.com/go-kit/kit](./github.com/go-kit/kit/otelkit) | | ✓ |
| [github.com/gocql/gocql](./github.com/gocql/gocql/otelgocql) | ✓ | ✓ |
| [github.com/gorilla/mux](./github.com/gorilla/mux/otelmux) | | ✓ |
| [github.com/labstack/echo](./github.com/labstack/echo/otelecho) | | ✓ |
| [github.com/IBM/sarama](./github.com/Shopify/sarama/otelsarama) | | ✓ |
| [go.mongodb.org/mongo-driver](./go.mongodb.org/mongo-driver/mongo/otelmongo) | | ✓ |
| [google.golang.org/grpc](./google.golang.org/grpc/otelgrpc) | ✓ | ✓ |
| [gopkg.in/macaron.v1](./gopkg.in/macaron.v1/otelmacaron) | | ✓ |
| [host](./host) | ✓ | |
| [net/http](./net/http/otelhttp) | ✓ | ✓ |
| [net/http/httptrace](./net/http/httptrace/otelhttptrace) | | ✓ |
| [runtime](./runtime) | ✓ | |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, no.

@MrAlias
Copy link
Contributor

MrAlias commented Jul 19, 2023

I'm not sure it's appropriate to simply change the dependency here. Won't that break anyone still using the prior import paths who updates this instrumentation?

Good point.

@pellared
Copy link
Member

Closing as there is no consensus to ONLY update the dependency using the new import path.

@pellared pellared closed this Jul 21, 2023
@pellared pellared mentioned this pull request Jul 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aneurysm9 Aneurysm9 Aneurysm9 left review comments

@pellared pellared pellared requested changes

@MrAlias MrAlias MrAlias approved these changes

@XSAM XSAM XSAM approved these changes

@hanyuancheung hanyuancheung hanyuancheung approved these changes

@MadVikingGod MadVikingGod Awaiting requested review from MadVikingGod

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@batazor @pellared @dmathieu @MrAlias @Aneurysm9 @XSAM @hanyuancheung