Add Go code generated for api_v2 #116

adityachopra29 · 2025-01-14T20:51:26Z

Which problem is this PR solving?

Part 1 and 2 of Migrate proto-gen/thrift-gen to jaeger-idl jaeger#6494 for api_v2

Description of the changes

Create makefile for compiling and generating code from .proto files present
All locations of proto files, as well as generated code are inspired from main jaeger repo. (Since no other alteranative was suggested in issue.)
Add all the generated code from the proto files as well.
Create ci-lint file for same.

How was this change tested?

CI

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: adityachopra29 <adityachopra2912@gmail.com>

adityachopra29 · 2025-01-14T20:54:59Z

@yurishkuro Please have a look now.
I was not sure where all required proto files must be added, so i took inspiration from the main repo, can change it if that is not required.

Similiarly, since make proto command was already used, I created the command make new-proto for the generation. We can change that according to need

yurishkuro

I would recommend skipping /model/ for now, because it has a much higher knock-off effect on Jaeger repo, while the other types are easier to swap, and do not require much of organization. The original ticket was not asking to migrate /model/.

Please ensure that all generated files are organized into a consistent folder hierarchy, rooted at proto-gen

I would recommend starting with a single .proto, e.g. api_v2. There is more housekeeping work that needs to be done first, such as setting up CI to lint & compile, defining a release process, etc. Let's do one module e2e before migrating everything.

yurishkuro · 2025-01-15T01:41:34Z

internal/proto/api_v3/query_service.pb.go

@@ -0,0 +1,3357 @@
+// Code generated by protoc-gen-gogo. DO NOT EDIT.


it won't be usable if it's in the internal package.

yurishkuro · 2025-01-15T01:41:53Z

plugin/storage/grpc/proto/storage_test.proto

@@ -0,0 +1,27 @@
+// Copyright (c) 2019 The Jaeger Authors


this is not useful here, we can leave it in Jaeger

yurishkuro · 2025-01-15T01:42:28Z

model/prototest/model_test.pb.go

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


not needed in this repo

Signed-off-by: adityachopra29 <adityachopra2912@gmail.com>

adityachopra29 · 2025-01-15T16:03:31Z

I would recommend starting with a single .proto, e.g. api_v2. There is more housekeeping work that needs to be done first, such as setting up CI to lint & compile, defining a release process, etc. Let's do one module e2e before migrating everything.

Okay. I was trying to understand how to setup CI, etc. Can you please explain what all is left in setting up of the CI? @yurishkuro

Also, I have made the rest of the changes you have told above regarding api_v3 and removing the 'internal" folder, but since we are first completing api_v2 e2e, I didnt push them right now
I have not added proto-gen/api_v2/metrics, since it was dependant on /model
OPENMETRICS_PROTO_FILES=$(wildcard model/proto/metrics/*.proto)

yurishkuro · 2025-01-15T19:59:03Z

what we want in the CI:

a re-run of the code generation and fail CI if it finds a difference (see how it's done in the jaeger repo)
I think that's it for now

Signed-off-by: adityachopra29 <adityachopra2912@gmail.com>

yurishkuro · 2025-01-16T14:51:14Z

Makefile.Protobuf.mk

+endef
+
+.PHONY: new-proto
+new-proto: new-proto-api-v2


@yurishkuro
There is already a command make proto in the main Makefile. I didn't want to remove that right now before confirming, so I made the make new-proto command in the meantime.
If we can remove the old make proto command, we can rename it.

why do we need two different paths at all? The difference between the two commands is that the old one generates for all languages (and is used mostly as a linter step), while the new one only generates Go types. I would prefer to avoid duplication and be able to support both Go and All languages with a single set of commands.

you know, let's hold off on this topic - we can refactor this as as separate issue, I would like to get api_v2 types into this repo first.

why do we need two different paths at all?

Yes, I agree. I had planned on replacing that test in the end. Just wanted to show you the changes before making them; hence, I made a new file.

Okay, I will create another pr to make these changes

yurishkuro · 2025-01-16T19:37:37Z

I am seeing CI failures

	proto/api_v3/query_service.proto
opentelemetry/proto/trace/v1/trace.proto: File not found.
proto/api_v3/query_service.proto:19:1: Import "opentelemetry/proto/trace/v1/trace.proto" was not found or had errors.
proto/api_v3/query_service.proto:165:3: "opentelemetry.proto.trace.v1.TracesData" is not defined.
proto/api_v3/query_service.proto:125:49: "opentelemetry.proto.trace.v1.TracesData" is not defined.
proto/api_v3/query_service.proto:129:53: "opentelemetry.proto.trace.v1.TracesData" is not defined.
make: *** [Makefile:153: proto-api-v3] Error 1

why is the original workflow affected, what changed? Does your new makefile override something (like variables) in the old one?

Signed-off-by: adityachopra29 <adityachopra2912@gmail.com>

adityachopra29 · 2025-01-16T20:29:07Z

@yurishkuro My bad, I had redefined the PROTO_INCLUDES variable, because of which the error was coming. Have pushed the changes

Along the way, I also saw some command variables that were hardcoded in the Makefile (such as docker, sed). We can also change them in another pr.

yurishkuro · 2025-01-16T22:24:09Z

Makefile.Protobuf.mk

+DOCKER_PROTOBUF_VERSION=0.5.0
+DOCKER_PROTOBUF=jaegertracing/protobuf:$(DOCKER_PROTOBUF_VERSION)


there are already vars defined in the main Makefile for the same concept, should be using them.

yurishkuro · 2025-01-16T22:25:54Z

As discussed, I am going to merge this so that we can start using these types in the jaeger repo. A clean-up / deduplication can be done separately.

yurishkuro · 2025-01-17T02:43:19Z

@adityachopra29 we will also need go.mod somewhere to make this usable as a go library. And it would be good to have some minimal test to ensure the generated code compiles.

create makefile for proto code generation, add generated code

0c9a172

Signed-off-by: adityachopra29 <adityachopra2912@gmail.com>

yurishkuro reviewed Jan 15, 2025

View reviewed changes

remove everything except api_v2

681cb18

Signed-off-by: adityachopra29 <adityachopra2912@gmail.com>

adityachopra29 added 2 commits January 16, 2025 15:59

add ci for generated-files-check

3a15999

Signed-off-by: adityachopra29 <adityachopra2912@gmail.com>

fix ci-lint-file to use make new-proto command

cfe7532

Signed-off-by: adityachopra29 <adityachopra2912@gmail.com>

adityachopra29 requested a review from yurishkuro January 16, 2025 10:38

adityachopra29 changed the title ~~create makefile for proto code generation, add generated code~~ create makefile for proto code generation, add generated code, for api_v2 Jan 16, 2025

yurishkuro reviewed Jan 16, 2025

View reviewed changes

fix ci failure

a73055e

Signed-off-by: adityachopra29 <adityachopra2912@gmail.com>

yurishkuro changed the title ~~create makefile for proto code generation, add generated code, for api_v2~~ Add Go code generated for api_v2 Jan 16, 2025

yurishkuro reviewed Jan 16, 2025

View reviewed changes

yurishkuro approved these changes Jan 16, 2025

View reviewed changes

yurishkuro merged commit c003366 into jaegertracing:main Jan 16, 2025
3 checks passed

adityachopra29 deleted the migrate-proto-gen-jaeger branch January 17, 2025 05:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Go code generated for api_v2 #116

Add Go code generated for api_v2 #116

adityachopra29 commented Jan 14, 2025 •

edited

Loading

adityachopra29 commented Jan 14, 2025 •

edited

Loading

yurishkuro left a comment

yurishkuro Jan 15, 2025

yurishkuro Jan 15, 2025

yurishkuro Jan 15, 2025

adityachopra29 commented Jan 15, 2025 •

edited

Loading

yurishkuro commented Jan 15, 2025

yurishkuro Jan 16, 2025

adityachopra29 Jan 16, 2025

yurishkuro Jan 16, 2025

yurishkuro Jan 16, 2025

adityachopra29 Jan 16, 2025

yurishkuro commented Jan 16, 2025

adityachopra29 commented Jan 16, 2025

yurishkuro Jan 16, 2025

yurishkuro commented Jan 16, 2025

yurishkuro commented Jan 17, 2025

		@@ -0,0 +1,3357 @@
		// Code generated by protoc-gen-gogo. DO NOT EDIT.

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

		DOCKER_PROTOBUF_VERSION=0.5.0
		DOCKER_PROTOBUF=jaegertracing/protobuf:$(DOCKER_PROTOBUF_VERSION)

Add Go code generated for api_v2 #116

Add Go code generated for api_v2 #116

Conversation

adityachopra29 commented Jan 14, 2025 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

adityachopra29 commented Jan 14, 2025 • edited Loading

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityachopra29 commented Jan 15, 2025 • edited Loading

yurishkuro commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Jan 16, 2025

adityachopra29 commented Jan 16, 2025

Choose a reason for hiding this comment

yurishkuro commented Jan 16, 2025

yurishkuro commented Jan 17, 2025

adityachopra29 commented Jan 14, 2025 •

edited

Loading

adityachopra29 commented Jan 14, 2025 •

edited

Loading

adityachopra29 commented Jan 15, 2025 •

edited

Loading