Add models for sort.go, span.go, spanref.go, time.go, trace.go, model_test.proto #125

Nabil-Salah · 2025-01-21T01:03:49Z

Which problem is this PR solving?

Part of [jaeger-idl] Copy model/*go to jaeger-idl/model/v1 jaeger#6571

Description of the changes

built on the prev pr #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

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 lint: golangci-lint run ./model/v1/
- for tests: go test ./model/v1

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

codecov · 2025-01-21T01:12:08Z

Codecov Report

Attention: Patch coverage is 74.90909% with 69 lines in your changes missing coverage. Please review.

Project coverage is 8.33%. Comparing base (66db30d) to head (8224687).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
model/v1/prototest/model_test.pb.go	23.33%	67 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##            main    #125      +/-   ##
========================================
+ Coverage   3.46%   8.33%   +4.87%     
========================================
  Files         10      16       +6     
  Lines       7555    7830     +275     
========================================
+ Hits         262     653     +391     
+ Misses      7283    7135     -148     
- Partials      10      42      +32

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

Nabil-Salah · 2025-01-21T01:26:25Z

do you need a rebase with the new commit or is that fine?
@yurishkuro

model/v1/ids.go

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

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro

@Nabil-Salah I removed the following files from the PR because they are not direct copies, and I would like to understand the changes better. In fact, I think we should back-port the first change I made #118 by splitting certain logic into new files such that the same exact structure exists in the main repo - copying should really be no changes to the files. Can you do that?

	deleted:    model/v1/span.go
	deleted:    model/v1/span_pkg_test.go
	deleted:    model/v1/span_test.go

Nabil-Salah · 2025-01-21T15:12:42Z

yes of course will try to do so

I'll make my changes on this pr

yurishkuro · 2025-01-21T15:27:33Z

I created jaegertracing/jaeger#6581

yurishkuro · 2025-01-21T15:33:38Z

@Nabil-Salah if you hit Approve on that PR I can merge it to unblock you. But I think besides that PR there are more changes like that needed, for example

SpanID.ToOTELSpanID() (and similar for trace ID) need to be changed to standalone functions without receivers, since they will remain in jaeger while SpanID type will be in jaeger-idl
I separated out ids_proto_test.go which relies on a test .proto file that will need to be migrated too

Nabil-Salah · 2025-01-21T16:34:36Z

@yurishkuro
okay for now I'll try to copy files from your new merged pr and test it ->

the span and ids_proto_test.go files and the depends needed
other changes will keep them away like SpanID.ToOTELSpanID() and similar standalone functions

is it okay?

Nabil-Salah · 2025-01-21T17:00:33Z

@yurishkuro

for span to be exact copy I need

go.uber.org/zap dependency other than that I will remove one function GetSamplerParams
go.opentelemetry.io/otel/trace or remove TestIsRPCClientServer test from span_test.go file

the problem is that sort, trace depends on span so nothing further can be copied here

if okay I can make a new pr that contain those

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

yurishkuro · 2025-01-21T18:17:32Z

model/v1/prototest/model_test.pb.go

@@ -0,0 +1,230 @@
+// Copyright (c) 2018 Uber Technologies, Inc.


we cannot just copy generated file, it needs to be regenerated by make proto

yurishkuro · 2025-01-21T18:22:47Z

zap is used in GetSamplerParams which is only used in a single place in the code base:

internal/sampling/samplingstrategy/adaptive/aggregator.go
153:	samplerType, samplerParam := span.GetSamplerParams(logger)

Doesn't make sense to keep it in the model, let's move to a private method in the aggregator.

For OTEL dependency in TestIsRPCClientServer, let's just replace it with a plain string, it's fine for the test.

Can you make those changes in jaeger?

model/v1/spanref.go

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

yurishkuro · 2025-01-21T22:40:00Z

Makefile.Protobuf.mk

+.PHONY: proto-model
+proto-model:
+	$(call proto_compile, model/v1, proto/api_v2/model.proto)
+	$(PROTOC) -Imodel/proto --go_out=$(PWD)/model/v1/ model/v1/proto/model_test.proto


cannot use call proto_compile?

it run and works but give error here

yurishkuro · 2025-01-21T22:47:28Z

model/v1/proto/model_test.proto

+
+package prototest;
+
+option go_package = "./prototest";


let's not store it here. Since we have prototest directory anyway, we can store the .proto file there as well.

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

Nabil-Salah · 2025-01-21T22:54:21Z

@yurishkuro
for the makefile i didn't get it is that what you need?

yurishkuro · 2025-01-21T23:26:19Z

there were changes to makefiles in #120

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

Nabil-Salah · 2025-01-21T23:40:30Z

@yurishkuro
how to generate from model_test.proto or should i add this make?
make proto doesn't generate model/v1/prototest/model_test.pb.go file

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

yurishkuro · 2025-01-22T00:02:17Z

Makefile

@@ -179,6 +179,11 @@ proto-prepare-all:
 proto-prepare:
 	mkdir -p ${PROTO_GEN_GO_DIR}

+.PHONY: proto-model
+proto-model:
+	$(call proto_compile, model/v1, proto/api_v2/model.proto)


this is already done in proto-api-v2

yurishkuro · 2025-01-22T00:02:24Z

Makefile

@@ -179,6 +179,11 @@ proto-prepare-all:
 proto-prepare:
 	mkdir -p ${PROTO_GEN_GO_DIR}

+.PHONY: proto-model
+proto-model:


Suggested change

proto-model:

proto-prototest:

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

yurishkuro

Thanks!

Nabil-Salah added 3 commits January 21, 2025 01:13

feat: models for dependencies.go, hash.go, keyvalue.go, process.go

eaa3bb1

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

fix: linting and fmted

909d074

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

feat: add models for sort.go, span.go, spanref.go, time.go, trace.go

8ae63ed

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

yurishkuro changed the title ~~add models for sort.go, span.go, spanref.go, time.go, trace.go~~ Add models for sort.go, span.go, spanref.go, time.go, trace.go Jan 21, 2025

Merge branch 'main' into add_part_of_models_B

a39ae2a

yurishkuro reviewed Jan 21, 2025

View reviewed changes

model/v1/ids.go Outdated Show resolved Hide resolved

Nabil-Salah and others added 2 commits January 21, 2025 13:13

feat: add models for sort.go, span.go, spanref.go, time.go, trace.go

d5ac881

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

Remove span files

9637c13

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro approved these changes Jan 21, 2025

View reviewed changes

yurishkuro changed the title ~~Add models for sort.go, span.go, spanref.go, time.go, trace.go~~ Add models for sort.go, spanref.go, time.go, trace.go Jan 21, 2025

yurishkuro enabled auto-merge (squash) January 21, 2025 15:13

feat: add model_test.pb.go, ids_proto_test.go and remove span dependi…

2cbb813

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

auto-merge was automatically disabled January 21, 2025 17:18
Head branch was pushed to by a user without write access

Nabil-Salah changed the title ~~Add models for sort.go, spanref.go, time.go, trace.go~~ Add models for spanref.go, time.go Jan 21, 2025

yurishkuro reviewed Jan 21, 2025

View reviewed changes

model/v1/spanref.go Show resolved Hide resolved

feat: add remaining models

26039bb

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

Nabil-Salah changed the title ~~Add models for spanref.go, time.go~~ Add models for sort.go, span.go, spanref.go, time.go, trace.go, model_test.proto Jan 21, 2025

yurishkuro reviewed Jan 21, 2025

View reviewed changes

mode model_test.proto to prototest folder

e8e79d0

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

add new remote makefile

7141aa6

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

feat: generate prototest models using the makefile

6be9b3a

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

yurishkuro reviewed Jan 22, 2025

View reviewed changes

enhance: makefile naming and dublicate code

8224687

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

yurishkuro approved these changes Jan 22, 2025

View reviewed changes

yurishkuro merged commit 73f885e into jaegertracing:main Jan 22, 2025
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add models for sort.go, span.go, spanref.go, time.go, trace.go, model_test.proto #125

Add models for sort.go, span.go, spanref.go, time.go, trace.go, model_test.proto #125

Nabil-Salah commented Jan 21, 2025 •

edited by yurishkuro

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

Nabil-Salah commented Jan 21, 2025 •

edited

Loading

yurishkuro left a comment •

edited

Loading

Nabil-Salah commented Jan 21, 2025 •

edited

Loading

yurishkuro commented Jan 21, 2025

yurishkuro commented Jan 21, 2025

Nabil-Salah commented Jan 21, 2025 •

edited

Loading

Nabil-Salah commented Jan 21, 2025 •

edited

Loading

yurishkuro Jan 21, 2025

yurishkuro commented Jan 21, 2025

yurishkuro Jan 21, 2025

Nabil-Salah Jan 21, 2025

yurishkuro Jan 21, 2025

Nabil-Salah commented Jan 21, 2025 •

edited

Loading

yurishkuro commented Jan 21, 2025

Nabil-Salah commented Jan 21, 2025 •

edited

Loading

yurishkuro Jan 22, 2025

yurishkuro Jan 22, 2025

yurishkuro left a comment

		@@ -0,0 +1,230 @@
		// Copyright (c) 2018 Uber Technologies, Inc.

Add models for sort.go, span.go, spanref.go, time.go, trace.go, model_test.proto #125

Add models for sort.go, span.go, spanref.go, time.go, trace.go, model_test.proto #125

Conversation

Nabil-Salah commented Jan 21, 2025 • edited by yurishkuro Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Jan 21, 2025 • edited Loading

Codecov Report

Nabil-Salah commented Jan 21, 2025 • edited Loading

yurishkuro left a comment • edited Loading

Choose a reason for hiding this comment

Nabil-Salah commented Jan 21, 2025 • edited Loading

yurishkuro commented Jan 21, 2025

yurishkuro commented Jan 21, 2025

Nabil-Salah commented Jan 21, 2025 • edited Loading

Nabil-Salah commented Jan 21, 2025 • edited Loading

yurishkuro Jan 21, 2025

Choose a reason for hiding this comment

yurishkuro commented Jan 21, 2025

yurishkuro Jan 21, 2025

Choose a reason for hiding this comment

Nabil-Salah Jan 21, 2025

Choose a reason for hiding this comment

yurishkuro Jan 21, 2025

Choose a reason for hiding this comment

Nabil-Salah commented Jan 21, 2025 • edited Loading

yurishkuro commented Jan 21, 2025

Nabil-Salah commented Jan 21, 2025 • edited Loading

yurishkuro Jan 22, 2025

Choose a reason for hiding this comment

yurishkuro Jan 22, 2025

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Nabil-Salah commented Jan 21, 2025 •

edited by yurishkuro

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

Nabil-Salah commented Jan 21, 2025 •

edited

Loading

yurishkuro left a comment •

edited

Loading

Nabil-Salah commented Jan 21, 2025 •

edited

Loading

Nabil-Salah commented Jan 21, 2025 •

edited

Loading

Nabil-Salah commented Jan 21, 2025 •

edited

Loading

Nabil-Salah commented Jan 21, 2025 •

edited

Loading

Nabil-Salah commented Jan 21, 2025 •

edited

Loading