Use official OTLP types in api_v3 and avoid triple-marshaling (#5098)

## Which problem is this PR solving? - Part of #5079 - Avoid triple marshaling due to our use of internally generated OTLP proto types, which prevents us from directly using the output of jaeger->otlp conversion from collector contrib. ## Description of the changes - 🛑 breaking: the JSON format is changed to match [OTLP-JSON][otlp-json], specifically - the trace & span IDs are returned as hex-encoded strings, not base64 as required by Proto-JSON - enums are returned as integers, not strings - Use Proposal 1 from open-telemetry/opentelemetry-collector#9233 (comment) - API-V3 proto is already declared to use official OTLP types; remove the overrides to our internally generated OTLP proto types - Replace `SpansResponseChunk` with official `otlp.TracesData`, but override it internally to use a custom type - Depends on jaegertracing/jaeger-idl#103 ## How was this change tested? - Unit tests [otlp-json]: https://github.com/open-telemetry/opentelemetry-proto/blob/main/docs/specification.md#json-protobuf-encoding --------- Signed-off-by: Yuri Shkuro <[email protected]>
jaegertracing · Jan 15, 2024 · 2f592b8 · 2f592b8
1 parent 09f7a30
commit 2f592b8
Show file tree

Hide file tree

Showing 19 changed files with 328 additions and 249 deletions.
diff --git a/Makefile.Protobuf.mk b/Makefile.Protobuf.mk
@@ -146,14 +146,17 @@ proto-otel: proto-prepare-otel
 	$(foreach file,$(OTEL_PROTO_FILES), \
 	  $(call proto_compile, proto-gen/otel, $(file), -I$(PATCHED_OTEL_PROTO_DIR), paths=source_relative))
 
-# Similar to 'proto-prepare-otel', this target modifies OTEL Protos by changing their Go package.
-# This way the API v3 service uses official OTEL types like opentelemetry.proto.trace.v1.ResourceSpans
-# which at runtime are mapped to our internal classes generated in proto-gen/otel by 'proto-otel' target.
+# The API v3 service uses official OTEL type opentelemetry.proto.trace.v1.TracesData,
+# which at runtime is mapped to a custom type in cmd/query/app/internal/api_v3/traces.go
+# Unfortunately, gogoproto.customtype annotation cannot be applied to a method's return type,
+# only to fields in a struct, so we use regex search/replace to swap it.
+# Note that the .pb.go types must be generated into the same internal package $(API_V3_PATH)
+# where a manually defined traces.go file is located.
+API_V3_PATH=cmd/query/app/internal/api_v3
 .PHONY: proto-api-v3
 proto-api-v3:
-	$(call print_caption, Enriching OpenTelemetry Protos into $(PATCHED_OTEL_PROTO_DIR))
-	rm -rf $(PATCHED_OTEL_PROTO_DIR)/*
-	cp -R idl/opentelemetry-proto/* $(PATCHED_OTEL_PROTO_DIR)
-	find $(PATCHED_OTEL_PROTO_DIR) -name "*.proto" | xargs -L 1 $(SED) -i 's+go.opentelemetry.io/proto/otlp+github.com/jaegertracing/jaeger/proto-gen/otel+g'
-
-	$(call proto_compile, proto-gen/api_v3, idl/proto/api_v3/query_service.proto, -I$(PATCHED_OTEL_PROTO_DIR))
+	$(call proto_compile, $(API_V3_PATH), idl/proto/api_v3/query_service.proto, -Iidl/opentelemetry-proto)
+	@echo "🏗️  replace TracesData with internal custom type"
+	$(SED) -i 's/v1.TracesData/TracesData/g' $(API_V3_PATH)/query_service.pb.go
+	@echo "🏗️  remove OTEL import because we're not using any other OTLP types"
+	$(SED) -i 's+^.*v1 "go.opentelemetry.io/proto/otlp/trace/v1".*$$++' $(API_V3_PATH)/query_service.pb.go
diff --git a/cmd/all-in-one/all_in_one_test.go b/cmd/all-in-one/all_in_one_test.go
@@ -31,7 +31,6 @@ import (
 
 	ui "github.com/jaegertracing/jaeger/model/json"
 	"github.com/jaegertracing/jaeger/proto-gen/api_v2"
-	"github.com/jaegertracing/jaeger/proto-gen/api_v3"
 )
 
 // These tests are only run when the environment variable TEST_MODE=integration is set.
@@ -180,9 +179,11 @@ func getServicesAPIV3(t *testing.T) {
 	require.NoError(t, err)
 	body, _ := io.ReadAll(resp.Body)
 
-	var servicesResponse api_v3.GetServicesResponse
+	var servicesResponse struct {
+		Services []string
+	}
 	err = json.Unmarshal(body, &servicesResponse)
 	require.NoError(t, err)
-	require.Len(t, servicesResponse.GetServices(), 1)
-	assert.Contains(t, servicesResponse.GetServices()[0], "jaeger")
+	require.Len(t, servicesResponse.Services, 1)
+	assert.Contains(t, servicesResponse.Services[0], "jaeger")
 }
diff --git a/cmd/query/app/apiv3/gateway_test.go b/cmd/query/app/apiv3/gateway_test.go
@@ -30,10 +30,10 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/jaegertracing/jaeger/cmd/query/app/internal/api_v3"
 	"github.com/jaegertracing/jaeger/model"
 	_ "github.com/jaegertracing/jaeger/pkg/gogocodec" // force gogo codec registration
 	"github.com/jaegertracing/jaeger/pkg/tenancy"
-	"github.com/jaegertracing/jaeger/proto-gen/api_v3"
 	"github.com/jaegertracing/jaeger/storage/spanstore"
 	spanstoremocks "github.com/jaegertracing/jaeger/storage/spanstore/mocks"
 )
@@ -155,18 +155,7 @@ func (gw *testGateway) runGatewayGetOperations(t *testing.T) {
 func (gw *testGateway) runGatewayGetTrace(t *testing.T) {
 	trace, traceID := makeTestTrace()
 	gw.reader.On("GetTrace", matchContext, traceID).Return(trace, nil).Once()
-
-	body, statusCode := gw.execRequest(t, "/api/v3/traces/"+traceID.String())
-	require.Equal(t, http.StatusOK, statusCode, "response=%s", string(body))
-	body = gw.verifySnapshot(t, body)
-
-	var response api_v3.GRPCGatewayWrapper
-	parseResponse(t, body, &response)
-
-	assert.Len(t, response.Result.ResourceSpans, 1)
-	assert.EqualValues(t,
-		bytesOfTraceID(t, traceID.High, traceID.Low),
-		response.Result.ResourceSpans[0].ScopeSpans[0].Spans[0].TraceID)
+	gw.getTracesAndVerify(t, "/api/v3/traces/"+traceID.String(), traceID)
 }
 
 func (gw *testGateway) runGatewayFindTraces(t *testing.T) {
@@ -175,23 +164,18 @@ func (gw *testGateway) runGatewayFindTraces(t *testing.T) {
 	gw.reader.
 		On("FindTraces", matchContext, qp).
 		Return([]*model.Trace{trace}, nil).Once()
-	body, statusCode := gw.execRequest(t, "/api/v3/traces?"+q.Encode())
+	gw.getTracesAndVerify(t, "/api/v3/traces?"+q.Encode(), traceID)
+}
+
+func (gw *testGateway) getTracesAndVerify(t *testing.T, url string, expectedTraceID model.TraceID) {
+	body, statusCode := gw.execRequest(t, url)
 	require.Equal(t, http.StatusOK, statusCode, "response=%s", string(body))
 	body = gw.verifySnapshot(t, body)
 
 	var response api_v3.GRPCGatewayWrapper
 	parseResponse(t, body, &response)
-
-	assert.Len(t, response.Result.ResourceSpans, 1)
-	assert.EqualValues(t,
-		bytesOfTraceID(t, traceID.High, traceID.Low),
-		response.Result.ResourceSpans[0].ScopeSpans[0].Spans[0].TraceID)
-}
-
-func bytesOfTraceID(t *testing.T, high, low uint64) []byte {
-	traceID := model.NewTraceID(high, low)
-	buf := make([]byte, 16)
-	_, err := traceID.MarshalTo(buf)
-	require.NoError(t, err)
-	return buf
+	td := response.Result.ToTraces()
+	assert.EqualValues(t, 1, td.SpanCount())
+	traceID := td.ResourceSpans().At(0).ScopeSpans().At(0).Spans().At(0).TraceID()
+	assert.Equal(t, expectedTraceID.String(), traceID.String())
 }
diff --git a/cmd/query/app/apiv3/grpc_handler.go b/cmd/query/app/apiv3/grpc_handler.go
@@ -22,9 +22,9 @@ import (
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 
+	"github.com/jaegertracing/jaeger/cmd/query/app/internal/api_v3"
 	"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
 	"github.com/jaegertracing/jaeger/model"
-	"github.com/jaegertracing/jaeger/proto-gen/api_v3"
 	"github.com/jaegertracing/jaeger/storage/spanstore"
 )
 
@@ -33,6 +33,7 @@ type Handler struct {
 	QueryService *querysvc.QueryService
 }
 
+// remove me
 var _ api_v3.QueryServiceServer = (*Handler)(nil)
 
 // GetTrace implements api_v3.QueryServiceServer's GetTrace
@@ -46,13 +47,12 @@ func (h *Handler) GetTrace(request *api_v3.GetTraceRequest, stream api_v3.QueryS
 	if err != nil {
 		return fmt.Errorf("cannot retrieve trace: %w", err)
 	}
-	resourceSpans, err := modelToOTLP(trace.GetSpans())
+	td, err := modelToOTLP(trace.GetSpans())
 	if err != nil {
 		return err
 	}
-	return stream.Send(&api_v3.SpansResponseChunk{
-		ResourceSpans: resourceSpans,
-	})
+	tracesData := api_v3.TracesData(td)
+	return stream.Send(&tracesData)
 }
 
 // FindTraces implements api_v3.QueryServiceServer's FindTraces
@@ -106,13 +106,12 @@ func (h *Handler) FindTraces(request *api_v3.FindTracesRequest, stream api_v3.Qu
 		return err
 	}
 	for _, t := range traces {
-		resourceSpans, err := modelToOTLP(t.GetSpans())
+		td, err := modelToOTLP(t.GetSpans())
 		if err != nil {
 			return err
 		}
-		stream.Send(&api_v3.SpansResponseChunk{
-			ResourceSpans: resourceSpans,
-		})
+		tracesData := api_v3.TracesData(td)
+		stream.Send(&tracesData)
 	}
 	return nil
 }

diff --git a/cmd/query/app/apiv3/grpc_handler_test.go b/cmd/query/app/apiv3/grpc_handler_test.go
@@ -27,10 +27,10 @@ import (
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials/insecure"
 
+	"github.com/jaegertracing/jaeger/cmd/query/app/internal/api_v3"
 	"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
 	"github.com/jaegertracing/jaeger/model"
 	_ "github.com/jaegertracing/jaeger/pkg/gogocodec" // force gogo codec registration
-	"github.com/jaegertracing/jaeger/proto-gen/api_v3"
 	dependencyStoreMocks "github.com/jaegertracing/jaeger/storage/dependencystore/mocks"
 	"github.com/jaegertracing/jaeger/storage/spanstore"
 	spanstoremocks "github.com/jaegertracing/jaeger/storage/spanstore/mocks"
@@ -106,10 +106,12 @@ func TestGetTrace(t *testing.T) {
 		},
 	)
 	require.NoError(t, err)
-	spansChunk, err := getTraceStream.Recv()
+	recv, err := getTraceStream.Recv()
 	require.NoError(t, err)
-	require.Len(t, spansChunk.GetResourceSpans(), 1)
-	assert.Equal(t, "foobar", spansChunk.GetResourceSpans()[0].GetScopeSpans()[0].GetSpans()[0].GetName())
+	td := recv.ToTraces()
+	require.EqualValues(t, 1, td.SpanCount())
+	assert.Equal(t, "foobar",
+		td.ResourceSpans().At(0).ScopeSpans().At(0).Spans().At(0).Name())
 }
 
 func TestGetTraceStorageError(t *testing.T) {
@@ -121,10 +123,10 @@ func TestGetTraceStorageError(t *testing.T) {
 		TraceId: "156",
 	})
 	require.NoError(t, err)
-	spansChunk, err := getTraceStream.Recv()
+	recv, err := getTraceStream.Recv()
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "storage_error")
-	assert.Nil(t, spansChunk)
+	assert.Nil(t, recv)
 }
 
 func TestGetTraceTraceIDError(t *testing.T) {
@@ -138,10 +140,10 @@ func TestGetTraceTraceIDError(t *testing.T) {
 		TraceId: "Z",
 	})
 	require.NoError(t, err)
-	spansChunk, err := getTraceStream.Recv()
+	recv, err := getTraceStream.Recv()
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "strconv.ParseUint:")
-	assert.Nil(t, spansChunk)
+	assert.Nil(t, recv)
 }
 
 func TestFindTraces(t *testing.T) {
@@ -171,7 +173,8 @@ func TestFindTraces(t *testing.T) {
 	require.NoError(t, err)
 	recv, err := responseStream.Recv()
 	require.NoError(t, err)
-	assert.Len(t, recv.GetResourceSpans(), 1)
+	td := recv.ToTraces()
+	require.EqualValues(t, 1, td.SpanCount())
 }
 
 func TestFindTracesQueryNil(t *testing.T) {

diff --git a/cmd/query/app/apiv3/http_gateway.go b/cmd/query/app/apiv3/http_gateway.go
@@ -15,15 +15,15 @@ import (
 	"github.com/gogo/protobuf/jsonpb"
 	"github.com/gogo/protobuf/proto"
 	"github.com/gorilla/mux"
+	"go.opentelemetry.io/collector/pdata/ptrace"
 	"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
 	"go.uber.org/zap"
 
+	"github.com/jaegertracing/jaeger/cmd/query/app/internal/api_v3"
 	"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
 	"github.com/jaegertracing/jaeger/model"
 	"github.com/jaegertracing/jaeger/pkg/jtracer"
 	"github.com/jaegertracing/jaeger/pkg/tenancy"
-	"github.com/jaegertracing/jaeger/proto-gen/api_v3"
-	tracev1 "github.com/jaegertracing/jaeger/proto-gen/otel/trace/v1"
 	"github.com/jaegertracing/jaeger/storage/spanstore"
 )
 
@@ -118,18 +118,16 @@ func (h *HTTPGateway) returnSpans(spans []*model.Span, w http.ResponseWriter) {
 func (h *HTTPGateway) returnSpansTestable(
 	spans []*model.Span,
 	w http.ResponseWriter,
-	modelToOTLP func(spans []*model.Span) ([]*tracev1.ResourceSpans, error),
+	modelToOTLP func(spans []*model.Span) (ptrace.Traces, error),
 ) {
-	resourceSpans, err := modelToOTLP(spans)
+	td, err := modelToOTLP(spans)
 	if h.tryHandleError(w, err, http.StatusInternalServerError) {
 		return
 	}
+	tracesData := api_v3.TracesData(td)
 	response := &api_v3.GRPCGatewayWrapper{
-		Result: &api_v3.SpansResponseChunk{
-			ResourceSpans: resourceSpans,
-		},
+		Result: &tracesData,
 	}
-
 	h.marshalResponse(response, w)
 }
 

diff --git a/cmd/query/app/apiv3/http_gateway_test.go b/cmd/query/app/apiv3/http_gateway_test.go
@@ -15,14 +15,14 @@ import (
 	"github.com/gorilla/mux"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.opentelemetry.io/collector/pdata/ptrace"
 	"go.uber.org/zap"
 
 	"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
 	"github.com/jaegertracing/jaeger/model"
 	"github.com/jaegertracing/jaeger/pkg/jtracer"
 	"github.com/jaegertracing/jaeger/pkg/tenancy"
 	"github.com/jaegertracing/jaeger/pkg/testutils"
-	tracev1 "github.com/jaegertracing/jaeger/proto-gen/otel/trace/v1"
 	dependencyStoreMocks "github.com/jaegertracing/jaeger/storage/dependencystore/mocks"
 	"github.com/jaegertracing/jaeger/storage/spanstore"
 	spanstoremocks "github.com/jaegertracing/jaeger/storage/spanstore/mocks"
@@ -117,8 +117,8 @@ func TestHTTPGatewayOTLPError(t *testing.T) {
 	}
 	const simErr = "simulated error"
 	gw.returnSpansTestable(nil, w,
-		func(spans []*model.Span) ([]*tracev1.ResourceSpans, error) {
-			return nil, fmt.Errorf(simErr)
+		func(spans []*model.Span) (ptrace.Traces, error) {
+			return ptrace.Traces{}, fmt.Errorf(simErr)
 		},
 	)
 	assert.Contains(t, w.Body.String(), simErr)

diff --git a/cmd/query/app/apiv3/otlp_translator.go b/cmd/query/app/apiv3/otlp_translator.go
@@ -15,33 +15,13 @@
 package apiv3
 
 import (
-	"fmt"
-
-	"github.com/gogo/protobuf/proto"
 	model2otel "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/jaeger"
-	"go.opentelemetry.io/collector/pdata/ptrace/ptraceotlp"
+	"go.opentelemetry.io/collector/pdata/ptrace"
 
 	"github.com/jaegertracing/jaeger/model"
-	"github.com/jaegertracing/jaeger/proto-gen/api_v3"
-	tracev1 "github.com/jaegertracing/jaeger/proto-gen/otel/trace/v1"
 )
 
-func modelToOTLP(spans []*model.Span) ([]*tracev1.ResourceSpans, error) {
+func modelToOTLP(spans []*model.Span) (ptrace.Traces, error) {
 	batch := &model.Batch{Spans: spans}
-	td, err := model2otel.ProtoToTraces([]*model.Batch{batch})
-	if err != nil {
-		return nil, fmt.Errorf("cannot convert trace to OpenTelemetry: %w", err)
-	}
-	req := ptraceotlp.NewExportRequestFromTraces(td)
-	// OTEL Collector hides the internal proto implementation, so do a roundtrip conversion (inefficient)
-	b, err := req.MarshalProto()
-	if err != nil {
-		return nil, fmt.Errorf("cannot marshal OTLP: %w", err)
-	}
-	// use api_v3.SpansResponseChunk which has the same shape as otlp.ExportTraceServiceRequest
-	var chunk api_v3.SpansResponseChunk
-	if err := proto.Unmarshal(b, &chunk); err != nil {
-		return nil, fmt.Errorf("cannot marshal OTLP: %w", err)
-	}
-	return chunk.ResourceSpans, nil
+	return model2otel.ProtoToTraces([]*model.Batch{batch})
 }
diff --git a/cmd/query/app/apiv3/snapshots/FindTraces.json b/cmd/query/app/apiv3/snapshots/FindTraces.json
@@ -9,15 +9,15 @@
             "spans": [
               {
                 "endTimeUnixNano": "11651379494838206464",
-                "kind": "SPAN_KIND_SERVER",
+                "kind": 2,
                 "name": "foobar",
                 "parentSpanId": "",
-                "spanId": "AAAAAAAAALQ=",
+                "spanId": "00000000000000b4",
                 "startTimeUnixNano": "11651379494838206464",
                 "status": {
-                  "code": "STATUS_CODE_ERROR"
+                  "code": 2
                 },
-                "traceId": "AAAAAAAAAJYAAAAAAAAAoA=="
+                "traceId": "000000000000009600000000000000a0"
               }
             ]
           }

diff --git a/cmd/query/app/apiv3/snapshots/GetTrace.json b/cmd/query/app/apiv3/snapshots/GetTrace.json
@@ -9,15 +9,15 @@
             "spans": [
               {
                 "endTimeUnixNano": "11651379494838206464",
-                "kind": "SPAN_KIND_SERVER",
+                "kind": 2,
                 "name": "foobar",
                 "parentSpanId": "",
-                "spanId": "AAAAAAAAALQ=",
+                "spanId": "00000000000000b4",
                 "startTimeUnixNano": "11651379494838206464",
                 "status": {
-                  "code": "STATUS_CODE_ERROR"
+                  "code": 2
                 },
-                "traceId": "AAAAAAAAAJYAAAAAAAAAoA=="
+                "traceId": "000000000000009600000000000000a0"
               }
             ]
           }