[jaeger-idl] Copy model/*go to jaeger-idl/model/v1 #6571
Comments
yurishkuro
added
good first issue
|
hello @yurishkuro
and then calling them in Jaeger repo to use instead of depending on generating them + only add minimum and only essential business functions / helpers used can you confirm I'm on the right path so I can start implementation? |
|
No, you are not on the right path. The issue is about copying manually written Go code, it has nothing to do with |
|
okay so as a starting point the models you stated should be copied to jaeger-idl repo along with the tests while avoid copying some functions that have external dependencies same as what you have done with ids model should I then after that I remove them from jager repo and use jaeger-idl repo or this is for another issue? |
that's part of the parent issue. |
## Which problem is this PR solving? [#6571](jaegertracing/jaeger#6571) ## Description of the changes - Copy set of types from original `model/` -> `dependencies.go`, `hash.go`, `keyvalue.go`, `process.go` ## How was this change tested? - `go test ./model/v1` ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for lint: `golangci-lint run ./model/v1/` - for tests: `go test ./model/v1` --------- Signed-off-by: nabil salah <nabil.salah203@gmail.com> Signed-off-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
…gertracing#123) [#6571](jaegertracing/jaeger#6571) - Copy set of types from original `model/` -> `dependencies.go`, `hash.go`, `keyvalue.go`, `process.go` - `go test ./model/v1` - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for lint: `golangci-lint run ./model/v1/` - for tests: `go test ./model/v1` --------- Signed-off-by: nabil salah <nabil.salah203@gmail.com> Signed-off-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
## Which problem is this PR solving? - Part of #6571 - Follow-up to jaegertracing/jaeger-idl#118 ## Description of the changes - Separate model files into more independent pieces that are easier to move to jaeger-idl ## How was this change tested? - CI Signed-off-by: Yuri Shkuro <github@ysh.us>
) ## Which problem is this PR solving? - Part of #6571 - Follow-up to jaegertracing/jaeger-idl#118 ## Description of the changes - Separate model files into more independent pieces that are easier to move to jaeger-idl ## How was this change tested? - `go test ./model/` - `go test ./internal/sampling/samplingstrategy/adaptive/` ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [ ] I have added unit tests for the new functionality - [ ] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: nabil salah <nabil.salah203@gmail.com>
…_test.proto (#125) ## Which problem is this PR solving? * Part of jaegertracing/jaeger#6571 ## Description of the changes - built on the prev [pr #123](#123) - Copy set of types from original `model/` -> `sort.go`, `spanref.go`, `time.go`, `trace.go` ## How was this change tested? - `go test ./model/v1` ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for lint: `golangci-lint run ./model/v1/` - for tests: `go test ./model/v1` --------- Signed-off-by: nabil salah <nabil.salah203@gmail.com> Signed-off-by: Yuri Shkuro <github@ysh.us> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Co-authored-by: Yuri Shkuro <github@ysh.us>
|
is there any remaining changes needed for this issue? |
Part of #jaegertracing/jaeger#6571 Signed-off-by: Yuri Shkuro <github@ysh.us>
|
I copied the last two files, this is done. Thanks! |
|
The next step is to update the main Jaeger repo to use model from jaeger-idl |
|
i can work on this after some couple of hours if that is okay you can mention me in the issue or give me a link :D |
|
@Nabil-Salah you can see the parent issue (#6494) at the top of this one, under the title. 
|
Part of #6494
In Jaeger the domain model is generated in
model/model.pb.gofrommodel.protobut then enriched with all kinds of business functions / helpers. To ease the transition of the existing code to the newjaeger-idl/model/v1package we should copy all those helpers (a minimal required set was done in jaegertracing/jaeger-idl#118).After copying and running
go mod tidythere should be no new dependencies ingo.mod. We should avoid copying some functions that have external dependencies, like translations between model IDs and OTEL IDs.As there are many different files in
model/, it's best to do this incrementally, not as a single PR.The text was updated successfully, but these errors were encountered: