Redefine model/ and api_v2/ types as aliases to jaeger-idl/ types #6602

Nabil-Salah · 2025-01-23T22:53:08Z

Which problem is this PR solving?

part of Replace model imports with jaeger-idl #6595
part of Migrate proto-gen/thrift-gen to jaeger-idl #6494

Description of the changes

change models and api_v2 models

How was this change tested?

make lint, make test

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
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 <[email protected]>

Nabil-Salah · 2025-01-23T22:57:38Z

@yurishkuro
there's a problem i couldn't fix the alias fail in tests because of trace id in multiple testcases
can you suggest how to solve this?

codecov · 2025-01-23T23:02:21Z

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 96.08%. Comparing base (07e4f57) to head (48e2b6b).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/gogocodec/codec.go	50.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6602      +/-   ##
==========================================
- Coverage   96.21%   96.08%   -0.14%     
==========================================
  Files         377      366      -11     
  Lines       21421    20949     -472     
==========================================
- Hits        20611    20128     -483     
- Misses        616      624       +8     
- Partials      194      197       +3

Flag	Coverage Δ
badger_v1	`9.57% <0.00%> (-1.06%)`	⬇️
badger_v2	`1.75% <0.00%> (-1.04%)`	⬇️
cassandra-4.x-v1-manual	`15.29% <0.00%> (-1.36%)`	⬇️
cassandra-4.x-v2-auto	`1.74% <0.00%> (-0.98%)`	⬇️
cassandra-4.x-v2-manual	`1.74% <0.00%> (-0.98%)`	⬇️
cassandra-5.x-v1-manual	`15.29% <0.00%> (-1.36%)`	⬇️
cassandra-5.x-v2-auto	`1.74% <0.00%> (-0.98%)`	⬇️
cassandra-5.x-v2-manual	`1.74% <0.00%> (-0.98%)`	⬇️
elasticsearch-6.x-v1	`19.42% <0.00%> (-1.02%)`	⬇️
elasticsearch-7.x-v1	`19.50% <0.00%> (-1.01%)`	⬇️
elasticsearch-8.x-v1	`19.67% <0.00%> (-1.00%)`	⬇️
elasticsearch-8.x-v2	`1.75% <0.00%> (-1.04%)`	⬇️
grpc_v1	`11.02% <50.00%> (-1.18%)`	⬇️
grpc_v2	`7.77% <50.00%> (-1.29%)`	⬇️
kafka-3.x-v1	`9.86% <0.00%> (-0.50%)`	⬇️
kafka-3.x-v2	`1.75% <0.00%> (-1.04%)`	⬇️
memory_v2	`1.75% <0.00%> (-1.04%)`	⬇️
opensearch-1.x-v1	`19.55% <0.00%> (-1.00%)`	⬇️
opensearch-2.x-v1	`19.55% <0.00%> (-1.00%)`	⬇️
opensearch-2.x-v2	`1.75% <0.00%> (-1.04%)`	⬇️
tailsampling-processor	`0.47% <0.00%> (-0.05%)`	⬇️
unittests	`94.91% <0.00%> (-0.17%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yurishkuro

Looks very promising! The link check is expected to fail, and unit test needs a bit of investigation, but doesn't look too serious. And the best part the e2e CI tests are green. Feels like a much better approach than the copying we tried before :-)

model/flags.go

proto-gen/api_v2/collector.pb.go

Nabil-Salah · 2025-01-23T23:04:49Z

Looks very promising! The link check is expected to fail, and unit test needs a bit of investigation, but doesn't look too serious. And the best part the e2e CI tests are green. Feels like a much better approach than the copying we tried before :-)

yes and it will ease the transition if at any moment you wanted to remove model files

Signed-off-by: nabil salah <[email protected]>

yurishkuro · 2025-01-23T23:45:53Z

when running unit tests locally I am seeing many logs like this

2025/01/23 18:43:56 proto: duplicate proto type registered: jaeger.api_v2.PostSpansRequest

a bit strange.

Signed-off-by: nabil salah <[email protected]>

Nabil-Salah · 2025-01-24T00:11:38Z

when running unit tests locally I am seeing many logs like this
2025/01/23 18:43:56 proto: duplicate proto type registered: jaeger.api_v2.PostSpansRequest
a bit strange.

@yurishkuro

i think this happened after I changed .pb.go files to .go

should I remove them or removing their proto registers init functions?
i don't have clue why this happened

yurishkuro · 2025-01-24T00:17:23Z

I tracked one issue, in ./pkg/gogocodec package. You need this code in the codec.go instead of the existing mapping:

const (
	jaegerProtoGenPkgPath = "github.com/jaegertracing/jaeger-idl/proto-gen"
	jaegerModelPkgPath    = "github.com/jaegertracing/jaeger-idl/model/v1"
)

model/model.go

proto-gen/api_v2/collector.go

Signed-off-by: nabil salah <[email protected]>

model/model.go

model/process.go

proto-gen/api_v2/collector.go

yurishkuro · 2025-01-24T00:51:31Z

generated-files-check needs tweaks:

similar to mocks check step, we should be failing the test not only when there are diffs, but also when there are new files (see [storage]Generate mocks for dependency writer of v2 #6576)
exclude model and api_v2 generation from the Makefile.Protobuf.mk

yurishkuro · 2025-01-24T00:57:32Z

grpc tests failing with

    --- ❌ FAIL: TestGRPCRemoteStorage/GetTrace (1.20s)
panic: invalid Go type model.TraceID for field jaeger.storage.v1.GetTraceRequest.trace_id [recovered]
	panic: invalid Go type model.TraceID for field jaeger.storage.v1.GetTraceRequest.trace_id

Not sure why, you may need to make changes to plugin/storage/grpc/proto/storage.proto and regenerate make proto:

-       (gogoproto.customtype) = "github.com/jaegertracing/jaeger/model.TraceID",
+      (gogoproto.customtype) = "github.com/jaegertracing/jaeger-idl/model/v1.TraceID",

Signed-off-by: nabil salah <[email protected]>

Nabil-Salah · 2025-01-24T01:26:45Z

make proto keeps failing because can't find model.proto

yurishkuro · 2025-01-24T01:31:20Z

Run git submodule init, git submodule update --recursive

Signed-off-by: nabil salah <[email protected]>

Nabil-Salah · 2025-01-24T02:17:13Z

it runs with no problem but still giving e2e grpc fails

model/hash.go

Signed-off-by: nabil salah <[email protected]>

yurishkuro · 2025-01-24T02:26:46Z

ok, we will need to troubleshoot, I don't have an obvious next step

model/span.go

model/trace.go

proto-gen/api_v2/query.go

proto-gen/api_v2/collector.go

proto-gen/api_v2/sampling.go

Signed-off-by: nabil salah <[email protected]>

yurishkuro · 2025-01-24T02:49:38Z

try this patch. I don't know why gogo still encounters the old model, but I was able to reproduce the error in a unit test and this change below fixed it.

--- a/pkg/gogocodec/codec.go
+++ b/pkg/gogocodec/codec.go
@@ -17,6 +17,9 @@ import (
 const (
        jaegerProtoGenPkgPath = "github.com/jaegertracing/jaeger-idl/proto-gen"
        jaegerModelPkgPath    = "github.com/jaegertracing/jaeger-idl/model/v1"
+
+       jaegerProtoGenPkgPathOld = "github.com/jaegertracing/jaeger/proto-gen"
+       jaegerModelPkgPathOld    = "github.com/jaegertracing/jaeger/model"
 )

 var defaultCodec encoding.CodecV2
@@ -89,5 +92,11 @@ func useGogo(t reflect.Type) bool {
        if strings.HasPrefix(pkg, jaegerModelPkgPath) {
                return true
        }
+       if strings.HasPrefix(pkg, jaegerProtoGenPkgPathOld) {
+               return true
+       }
+       if strings.HasPrefix(pkg, jaegerModelPkgPathOld) {
+               return true
+       }
        return false
 }

Signed-off-by: nabil salah <[email protected]>

Nabil-Salah · 2025-01-24T03:06:50Z

try this patch. I don't know why gogo still encounters the old model, but I was able to reproduce the error in a unit test and this change below fixed it.

this is weird because we changed proto files that use model.proto already don't know why it happened

yurishkuro · 2025-01-24T03:10:36Z

Looks like we made it. This is awesome! I'm going to ignore codecov gap, we will soon be able to clean up that code anyway.

Nabil-Salah added 2 commits January 24, 2025 00:42

feat: convert models to alias jaegerIdlModel

b73fbad

Signed-off-by: nabil salah <[email protected]>

fix: fmt

fe5e350

Signed-off-by: nabil salah <[email protected]>

Nabil-Salah requested a review from a team as a code owner January 23, 2025 22:53

Nabil-Salah requested a review from joe-elliott January 23, 2025 22:53

yurishkuro reviewed Jan 23, 2025

View reviewed changes

model/flags.go Outdated Show resolved Hide resolved

proto-gen/api_v2/collector.pb.go Outdated Show resolved Hide resolved

yurishkuro added the changelog:bugfix-or-minor-feature label Jan 23, 2025

enhance:alias constants and top-level functions

e7e491d

Signed-off-by: nabil salah <[email protected]>

Nabil-Salah added 2 commits January 24, 2025 01:46

fix: linting and fmted

4dc49ee

Signed-off-by: nabil salah <[email protected]>

fix: proto duplicate enum registered

65346fd

Signed-off-by: nabil salah <[email protected]>