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

feat: Separate /model into its own module #4899

Open
6 tasks
yurishkuro opened this issue Oct 26, 2023 · 15 comments · May be fixed by #5144
Open
6 tasks

feat: Separate /model into its own module #4899

yurishkuro opened this issue Oct 26, 2023 · 15 comments · May be fixed by #5144
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@yurishkuro
Copy link
Member

yurishkuro commented Oct 26, 2023 •
edited

Jaeger repo has always been a monorepo with a single module / go.mod, because it was not meant to be used as a library. But it is often used as a library, in particular in the OpenTelemetry Collector Contrib, where many modules depend on our data model. This creates an undesired coupling, e.g. all our dependencies get pull as OTEL dependencies, sometimes causing conflicts.

In contrast, OTEL's repos are organized as multiple modules, which allows more selective dependencies.

The ask from OTEL project is to make /model as separate module (open-telemetry/opentelemetry-collector#8743 (comment)).

I don't know all that it takes to achieve that. Some trial and error will be needed. OTEL repos may serve as a blueprint.

  • Do we need special handling of the model module in the main go.mod for local development?
  • How does this change the build?
  • How does this affect code coverage computations?
  • How does this change the release / versioning process?
  • How will dependency updates be handled? OTEL apparently switched from dependabot to renovatebot because of multi-module setup.
  • What additional maintenance changes are needed? E.g. we have go 1.20 in go.mod, which is checked by scripts/check-go-version.sh - are there more places like that?
@yurishkuro yurishkuro added help wanted Features that maintainers are willing to accept but do not have cycles to implement good first issue Good for beginners labels Oct 26, 2023
@yurishkuro yurishkuro mentioned this issue Oct 26, 2023
Open
@AHB102
Copy link

AHB102 commented Nov 5, 2023

I am new to open source, can I start with this issue ? I am familiar with Go.

@yurishkuro
Copy link
Member Author

You can give it a try. You should start by outlining a proposal how this could be done, what changes are required.

@AHB102
Copy link

AHB102 commented Nov 10, 2023 •
edited

@yurishkuro Okay I will give it a shot, are any groups/chats which I can join ? I have quite a lot of questions, which might not be entirely related to the scope of this issue.

@yurishkuro
Copy link
Member Author

https://www.jaegertracing.io/get-in-touch/

@kanha-gupta
Copy link

@yurishkuro Any entry point to begin with ? I am new to jaeger :)

@yurishkuro
Copy link
Member Author

OTEL repos may serve as a blueprint.

that is the entry point (e.g. https://github.com/open-telemetry/opentelemetry-collector)

@kanha-gupta
Copy link

@yurishkuro Thanks, I will come up with a proposal soon :)

@AHB102
Copy link

AHB102 commented Nov 14, 2023

@yurishkuro I shared some guidelines for the changes I plan to make. Are they good to start working on a PR?

@yurishkuro
Copy link
Member Author

  • I would recommend a prototype first with a fake module, not starting with /model
  • I don't think this solution should require import path updates
  • I don't know if any additional build or CI changes would be required (thus start with prototype)
  • I don't know how the release process would be affected - do we just tag the whole repo with a new version tag as we do today and it would work?
  • do we need dependabot changes to keep shared dependencies in sync across modules?

@kanha-gupta
Copy link

kanha-gupta commented Nov 16, 2023 •
edited

Hi @yurishkuro This is the prototype I have created https://github.com/kanha-gupta/jaegerModelSeperation
It contains go.mod file, go.sum file, A MakeFile and a possible readme (if needed)
Tests are successful. There is a minor protobuf dependency error which I'll resolve during finalisation.
What are your thoughts ? :)

@yurishkuro
Copy link
Member Author

@kanha-gupta you moved /model into a separate repo, this is explicitly NOT what we're going after. We want to keep jaeger as the mono-repo which hosts multiple go-modules.

@RipulHandoo
Copy link
Contributor

Hey @yurishkuro,

I've created a prototype for /model. Check out the demo. If it looks good, I'll replace package demo -> package model.

Let me know if I'm on the right track and if there are any issues.

@yurishkuro
Copy link
Member Author

I just updated the ticket description. To make it clear - it's not rocket science to add a new go.mod file under /model, this is not what the ticket is about.

@shikharish
Copy link

@yurishkuro What is the status of this issue? Can I work on this?

@RipulHandoo
Copy link
Contributor

Hey @shikharish you can work on this issue.

@chirag-ghosh chirag-ghosh linked a pull request Jan 25, 2024 that will close this issue
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Separate /model into its own module chirag-ghosh/jaeger
5 participants
@yurishkuro @kanha-gupta @RipulHandoo @AHB102 @shikharish