From ca2856c2ab6ff0783cacfd330b7daea12c8d5bbd Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Thu, 28 Dec 2023 00:23:43 -0500 Subject: [PATCH 01/15] IDL update Signed-off-by: Yuri Shkuro --- cmd/query/app/apiv3/grpc_gateway.go | 10 +- cmd/query/app/apiv3/grpc_gateway_test.go | 53 +++-- idl | 2 +- pkg/tenancy/grpc.go | 2 +- pkg/tenancy/grpc_test.go | 2 +- proto-gen/api_v3/query_service.pb.go | 255 ++++++++++++++++++----- 6 files changed, 250 insertions(+), 74 deletions(-) diff --git a/cmd/query/app/apiv3/grpc_gateway.go b/cmd/query/app/apiv3/grpc_gateway.go index a300252b2ad..e93f024fa54 100644 --- a/cmd/query/app/apiv3/grpc_gateway.go +++ b/cmd/query/app/apiv3/grpc_gateway.go @@ -32,7 +32,15 @@ import ( ) // RegisterGRPCGateway registers api_v3 endpoints into provided mux. -func RegisterGRPCGateway(ctx context.Context, logger *zap.Logger, r *mux.Router, basePath string, grpcEndpoint string, grpcTLS *tlscfg.Options, tm *tenancy.Manager) error { +func RegisterGRPCGateway( + ctx context.Context, + logger *zap.Logger, + r *mux.Router, + basePath string, + grpcEndpoint string, + grpcTLS *tlscfg.Options, + tm *tenancy.Manager, +) error { grpcEndpoint = netutils.FixLocalhost([]string{grpcEndpoint})[0] jsonpb := &runtime.JSONPb{} diff --git a/cmd/query/app/apiv3/grpc_gateway_test.go b/cmd/query/app/apiv3/grpc_gateway_test.go index 673d7912304..bb9698395e1 100644 --- a/cmd/query/app/apiv3/grpc_gateway_test.go +++ b/cmd/query/app/apiv3/grpc_gateway_test.go @@ -60,6 +60,10 @@ const ( // REGENERATE_SNAPSHOTS=true go test -v ./cmd/query/app/apiv3/... var regenerateSnapshots = os.Getenv("REGENERATE_SNAPSHOTS") == "true" +// When run with USE_APIV3_HTTP_GATEWAY=true, the HTTP gateway is +// used for tests instead of grpc-gateway. +var useHTTPGateway = os.Getenv("USE_APIV3_HTTP_GATEWAY") == "true" + type testGateway struct { reader *spanstoremocks.Reader url string @@ -76,6 +80,9 @@ func setupGRPCGateway( serverTLS, clientTLS *tlscfg.Options, tenancyOptions tenancy.Options, ) *testGateway { + if useHTTPGateway { + return setupHTTPGateway(t, basePath, serverTLS, clientTLS, tenancyOptions) + } gw := &testGateway{ reader: &spanstoremocks.Reader{}, } @@ -144,7 +151,9 @@ func setupGRPCGateway( } func (gw *testGateway) execRequest(t *testing.T, gwReq *gatewayRequest) ([]byte, int) { - req, err := http.NewRequest(http.MethodGet, gw.url+gwReq.url, nil) + url := gw.url + gwReq.url + t.Logf("executing request %s", url) + req, err := http.NewRequest(http.MethodGet, url, nil) require.NoError(t, err) req.Header.Set("Content-Type", "application/json") gwReq.setupRequest(req) @@ -159,11 +168,14 @@ func (gw *testGateway) execRequest(t *testing.T, gwReq *gatewayRequest) ([]byte, func verifySnapshot(t *testing.T, body []byte) []byte { // reformat JSON body with indentation, to make diffing easier var data interface{} - require.NoError(t, json.Unmarshal(body, &data)) + require.NoError(t, json.Unmarshal(body, &data), "response: %s", string(body)) body, err := json.MarshalIndent(data, "", " ") require.NoError(t, err) - snapshotFile := filepath.Join(snapshotLocation, strings.ReplaceAll(t.Name(), "/", "_")+".json") + testName := strings.ReplaceAll(t.Name(), "/", "_") + testName = strings.ReplaceAll(testName, "TestHTTPGateway_", "") // use same fixtures + // TODO strip all test names except the last one, the fixtures are likely identical + snapshotFile := filepath.Join(snapshotLocation, testName+".json") if regenerateSnapshots { os.WriteFile(snapshotFile, body, 0o644) } @@ -177,17 +189,6 @@ func parseResponse(t *testing.T, body []byte, obj gogoproto.Message) { require.NoError(t, gogojsonpb.Unmarshal(bytes.NewBuffer(body), obj)) } -func parseChunkResponse(t *testing.T, body []byte, obj gogoproto.Message) { - // Unwrap the 'result' container generated by the gateway. - // See https://github.com/grpc-ecosystem/grpc-gateway/issues/2189 - type resultWrapper struct { - Result json.RawMessage `json:"result"` - } - var result resultWrapper - require.NoError(t, json.Unmarshal(body, &result)) - parseResponse(t, result.Result, obj) -} - func makeTestTrace() (*model.Trace, model.TraceID) { traceID := model.NewTraceID(150, 160) return &model.Trace{ @@ -282,11 +283,13 @@ func runGatewayGetTrace(t *testing.T, gw *testGateway, setupRequest func(*http.R require.Equal(t, http.StatusOK, statusCode, "response=%s", string(body)) body = verifySnapshot(t, body) - var spansResponse api_v3.SpansResponseChunk - parseChunkResponse(t, body, &spansResponse) + var response api_v3.GRPCGatewayWrapper + parseResponse(t, body, &response) - assert.Len(t, spansResponse.GetResourceSpans(), 1) - assert.Equal(t, bytesOfTraceID(t, traceID.High, traceID.Low), spansResponse.GetResourceSpans()[0].GetScopeSpans()[0].GetSpans()[0].GetTraceId()) + assert.Len(t, response.Result.ResourceSpans, 1) + assert.Equal(t, + bytesOfTraceID(t, traceID.High, traceID.Low), + response.Result.ResourceSpans[0].ScopeSpans[0].Spans[0].TraceId) } func runGatewayFindTraces(t *testing.T, gw *testGateway, setupRequest func(*http.Request)) { @@ -307,11 +310,13 @@ func runGatewayFindTraces(t *testing.T, gw *testGateway, setupRequest func(*http require.Equal(t, http.StatusOK, statusCode, "response=%s", string(body)) body = verifySnapshot(t, body) - var spansResponse api_v3.SpansResponseChunk - parseChunkResponse(t, body, &spansResponse) + var response api_v3.GRPCGatewayWrapper + parseResponse(t, body, &response) - assert.Len(t, spansResponse.GetResourceSpans(), 1) - assert.Equal(t, bytesOfTraceID(t, traceID.High, traceID.Low), spansResponse.GetResourceSpans()[0].GetScopeSpans()[0].GetSpans()[0].GetTraceId()) + assert.Len(t, response.Result.ResourceSpans, 1) + assert.Equal(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 { @@ -382,8 +387,10 @@ func TestGRPCGatewayTenancyRejection(t *testing.T) { // We don't set tenant header response, err := http.DefaultClient.Do(req) require.NoError(t, err) + body, err := io.ReadAll(response.Body) + require.NoError(t, err) require.NoError(t, response.Body.Close()) - require.Equal(t, http.StatusForbidden, response.StatusCode) + require.Equal(t, http.StatusUnauthorized, response.StatusCode, "response=%s", string(body)) // Try again with tenant header set tm := tenancy.NewManager(&tenancyOptions) diff --git a/idl b/idl index de44a4e9373..cd5d410a252 160000 --- a/idl +++ b/idl @@ -1 +1 @@ -Subproject commit de44a4e93731f57a6a926e16dffa1767de7a0f22 +Subproject commit cd5d410a252cc7e4683ad2aa0e7ffe2263539e37 diff --git a/pkg/tenancy/grpc.go b/pkg/tenancy/grpc.go index 9b2ad64c4a0..80d069d3825 100644 --- a/pkg/tenancy/grpc.go +++ b/pkg/tenancy/grpc.go @@ -89,7 +89,7 @@ func NewGuardingStreamInterceptor(tc *Manager) grpc.StreamServerInterceptor { func tenantFromMetadata(md metadata.MD, tenancyHeader string) (string, error) { tenants := md.Get(tenancyHeader) if len(tenants) < 1 { - return "", status.Errorf(codes.PermissionDenied, "missing tenant header") + return "", status.Errorf(codes.Unauthenticated, "missing tenant header") } else if len(tenants) > 1 { return "", status.Errorf(codes.PermissionDenied, "extra tenant header") } diff --git a/pkg/tenancy/grpc_test.go b/pkg/tenancy/grpc_test.go index 35ce3c7f4fc..1411e72c67a 100644 --- a/pkg/tenancy/grpc_test.go +++ b/pkg/tenancy/grpc_test.go @@ -60,7 +60,7 @@ func TestTenancyInterceptors(t *testing.T) { name: "missing tenant header", tenancyMgr: NewManager(&Options{Enabled: true, Tenants: []string{"megacorp"}}), ctx: metadata.NewIncomingContext(context.Background(), map[string][]string{}), - errMsg: "rpc error: code = PermissionDenied desc = missing tenant header", + errMsg: "rpc error: code = Unauthenticated desc = missing tenant header", }, { name: "valid tenant header", diff --git a/proto-gen/api_v3/query_service.pb.go b/proto-gen/api_v3/query_service.pb.go index 3cf2bf386dd..27c35b3148d 100644 --- a/proto-gen/api_v3/query_service.pb.go +++ b/proto-gen/api_v3/query_service.pb.go @@ -489,6 +489,156 @@ func (m *GetOperationsResponse) GetOperations() []*Operation { return nil } +// GRPCGatewayError is the type returned when GRPC server returns an error. +// Note that for streaming responses it would be wrapped in GRPCGatewayWrapper below. +// Example: {"error":{"grpcCode":2,"httpCode":500,"message":"...","httpStatus":"text..."}}. +type GRPCGatewayError struct { + Error *GRPCGatewayError_GRPCGatewayErrorDetails `protobuf:"bytes,1,opt,name=error,proto3" json:"error,omitempty"` + XXX_NoUnkeyedLiteral struct{} `json:"-"` + XXX_unrecognized []byte `json:"-"` + XXX_sizecache int32 `json:"-"` +} + +func (m *GRPCGatewayError) Reset() { *m = GRPCGatewayError{} } +func (m *GRPCGatewayError) String() string { return proto.CompactTextString(m) } +func (*GRPCGatewayError) ProtoMessage() {} +func (*GRPCGatewayError) Descriptor() ([]byte, []int) { + return fileDescriptor_5fcb6756dc1afb8d, []int{9} +} +func (m *GRPCGatewayError) XXX_Unmarshal(b []byte) error { + return xxx_messageInfo_GRPCGatewayError.Unmarshal(m, b) +} +func (m *GRPCGatewayError) XXX_Marshal(b []byte, deterministic bool) ([]byte, error) { + return xxx_messageInfo_GRPCGatewayError.Marshal(b, m, deterministic) +} +func (m *GRPCGatewayError) XXX_Merge(src proto.Message) { + xxx_messageInfo_GRPCGatewayError.Merge(m, src) +} +func (m *GRPCGatewayError) XXX_Size() int { + return xxx_messageInfo_GRPCGatewayError.Size(m) +} +func (m *GRPCGatewayError) XXX_DiscardUnknown() { + xxx_messageInfo_GRPCGatewayError.DiscardUnknown(m) +} + +var xxx_messageInfo_GRPCGatewayError proto.InternalMessageInfo + +func (m *GRPCGatewayError) GetError() *GRPCGatewayError_GRPCGatewayErrorDetails { + if m != nil { + return m.Error + } + return nil +} + +type GRPCGatewayError_GRPCGatewayErrorDetails struct { + GrpcCode int32 `protobuf:"varint,1,opt,name=grpcCode,proto3" json:"grpcCode,omitempty"` + HttpCode int32 `protobuf:"varint,2,opt,name=httpCode,proto3" json:"httpCode,omitempty"` + Message string `protobuf:"bytes,3,opt,name=message,proto3" json:"message,omitempty"` + HttpStatus string `protobuf:"bytes,4,opt,name=httpStatus,proto3" json:"httpStatus,omitempty"` + XXX_NoUnkeyedLiteral struct{} `json:"-"` + XXX_unrecognized []byte `json:"-"` + XXX_sizecache int32 `json:"-"` +} + +func (m *GRPCGatewayError_GRPCGatewayErrorDetails) Reset() { + *m = GRPCGatewayError_GRPCGatewayErrorDetails{} +} +func (m *GRPCGatewayError_GRPCGatewayErrorDetails) String() string { return proto.CompactTextString(m) } +func (*GRPCGatewayError_GRPCGatewayErrorDetails) ProtoMessage() {} +func (*GRPCGatewayError_GRPCGatewayErrorDetails) Descriptor() ([]byte, []int) { + return fileDescriptor_5fcb6756dc1afb8d, []int{9, 0} +} +func (m *GRPCGatewayError_GRPCGatewayErrorDetails) XXX_Unmarshal(b []byte) error { + return xxx_messageInfo_GRPCGatewayError_GRPCGatewayErrorDetails.Unmarshal(m, b) +} +func (m *GRPCGatewayError_GRPCGatewayErrorDetails) XXX_Marshal(b []byte, deterministic bool) ([]byte, error) { + return xxx_messageInfo_GRPCGatewayError_GRPCGatewayErrorDetails.Marshal(b, m, deterministic) +} +func (m *GRPCGatewayError_GRPCGatewayErrorDetails) XXX_Merge(src proto.Message) { + xxx_messageInfo_GRPCGatewayError_GRPCGatewayErrorDetails.Merge(m, src) +} +func (m *GRPCGatewayError_GRPCGatewayErrorDetails) XXX_Size() int { + return xxx_messageInfo_GRPCGatewayError_GRPCGatewayErrorDetails.Size(m) +} +func (m *GRPCGatewayError_GRPCGatewayErrorDetails) XXX_DiscardUnknown() { + xxx_messageInfo_GRPCGatewayError_GRPCGatewayErrorDetails.DiscardUnknown(m) +} + +var xxx_messageInfo_GRPCGatewayError_GRPCGatewayErrorDetails proto.InternalMessageInfo + +func (m *GRPCGatewayError_GRPCGatewayErrorDetails) GetGrpcCode() int32 { + if m != nil { + return m.GrpcCode + } + return 0 +} + +func (m *GRPCGatewayError_GRPCGatewayErrorDetails) GetHttpCode() int32 { + if m != nil { + return m.HttpCode + } + return 0 +} + +func (m *GRPCGatewayError_GRPCGatewayErrorDetails) GetMessage() string { + if m != nil { + return m.Message + } + return "" +} + +func (m *GRPCGatewayError_GRPCGatewayErrorDetails) GetHttpStatus() string { + if m != nil { + return m.HttpStatus + } + return "" +} + +// GRPCGatewayWrapper is a type returned when GRPC service returns a stream. +// For some unknown reason grpc-gateway/v1 wraps chunk responses in {"result": {actual output}}. +// See https://github.com/grpc-ecosystem/grpc-gateway/issues/2189 +// TODO: it's not clear what happens when the server returns more than one chunk. +// The gateway will presumably combine then into a single HTTP response. +// Currently this is not possible because even though APIv3 GRPC Service is using output stream, +// its implementation reads all spans from QueryService at once and forms only a single chunk. +type GRPCGatewayWrapper struct { + Result *SpansResponseChunk `protobuf:"bytes,1,opt,name=result,proto3" json:"result,omitempty"` + XXX_NoUnkeyedLiteral struct{} `json:"-"` + XXX_unrecognized []byte `json:"-"` + XXX_sizecache int32 `json:"-"` +} + +func (m *GRPCGatewayWrapper) Reset() { *m = GRPCGatewayWrapper{} } +func (m *GRPCGatewayWrapper) String() string { return proto.CompactTextString(m) } +func (*GRPCGatewayWrapper) ProtoMessage() {} +func (*GRPCGatewayWrapper) Descriptor() ([]byte, []int) { + return fileDescriptor_5fcb6756dc1afb8d, []int{10} +} +func (m *GRPCGatewayWrapper) XXX_Unmarshal(b []byte) error { + return xxx_messageInfo_GRPCGatewayWrapper.Unmarshal(m, b) +} +func (m *GRPCGatewayWrapper) XXX_Marshal(b []byte, deterministic bool) ([]byte, error) { + return xxx_messageInfo_GRPCGatewayWrapper.Marshal(b, m, deterministic) +} +func (m *GRPCGatewayWrapper) XXX_Merge(src proto.Message) { + xxx_messageInfo_GRPCGatewayWrapper.Merge(m, src) +} +func (m *GRPCGatewayWrapper) XXX_Size() int { + return xxx_messageInfo_GRPCGatewayWrapper.Size(m) +} +func (m *GRPCGatewayWrapper) XXX_DiscardUnknown() { + xxx_messageInfo_GRPCGatewayWrapper.DiscardUnknown(m) +} + +var xxx_messageInfo_GRPCGatewayWrapper proto.InternalMessageInfo + +func (m *GRPCGatewayWrapper) GetResult() *SpansResponseChunk { + if m != nil { + return m.Result + } + return nil +} + func init() { proto.RegisterType((*GetTraceRequest)(nil), "jaeger.api_v3.GetTraceRequest") proto.RegisterType((*SpansResponseChunk)(nil), "jaeger.api_v3.SpansResponseChunk") @@ -500,58 +650,69 @@ func init() { proto.RegisterType((*GetOperationsRequest)(nil), "jaeger.api_v3.GetOperationsRequest") proto.RegisterType((*Operation)(nil), "jaeger.api_v3.Operation") proto.RegisterType((*GetOperationsResponse)(nil), "jaeger.api_v3.GetOperationsResponse") + proto.RegisterType((*GRPCGatewayError)(nil), "jaeger.api_v3.GRPCGatewayError") + proto.RegisterType((*GRPCGatewayError_GRPCGatewayErrorDetails)(nil), "jaeger.api_v3.GRPCGatewayError.GRPCGatewayErrorDetails") + proto.RegisterType((*GRPCGatewayWrapper)(nil), "jaeger.api_v3.GRPCGatewayWrapper") } func init() { proto.RegisterFile("query_service.proto", fileDescriptor_5fcb6756dc1afb8d) } var fileDescriptor_5fcb6756dc1afb8d = []byte{ - // 726 bytes of a gzipped FileDescriptorProto - 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x94, 0x54, 0xed, 0x6e, 0xd3, 0x48, - 0x14, 0xad, 0xf3, 0xd1, 0x24, 0x37, 0x49, 0xbb, 0x3b, 0xcd, 0x6a, 0x5d, 0xaf, 0xb6, 0x4d, 0xdc, - 0x5d, 0x29, 0x12, 0x92, 0x43, 0xd3, 0x3f, 0x05, 0x8a, 0x80, 0xf2, 0x51, 0x21, 0xd4, 0x42, 0xdd, - 0xc2, 0x0f, 0x84, 0x64, 0x4d, 0xeb, 0x4b, 0x6a, 0x5a, 0x8f, 0x53, 0x7b, 0x1c, 0x25, 0x6f, 0x81, - 0xc4, 0x5b, 0xf0, 0x52, 0xbc, 0x01, 0xcf, 0x80, 0x3c, 0x33, 0x76, 0x13, 0x07, 0x95, 0xf0, 0x2b, - 0x33, 0x77, 0xce, 0x39, 0xf7, 0xe6, 0x9e, 0x7b, 0x0d, 0x6b, 0xd7, 0x31, 0x86, 0x13, 0x27, 0xc2, - 0x70, 0xe4, 0x9d, 0xa3, 0x35, 0x0c, 0x03, 0x1e, 0x90, 0xe6, 0x27, 0x8a, 0x03, 0x0c, 0x2d, 0x3a, - 0xf4, 0x9c, 0xd1, 0x8e, 0xd1, 0x0d, 0x86, 0xc8, 0x38, 0x5e, 0xa1, 0x8f, 0x3c, 0x9c, 0xf4, 0x04, - 0xa6, 0xc7, 0x43, 0x7a, 0x8e, 0xbd, 0xd1, 0xb6, 0x3c, 0x48, 0xa2, 0xd1, 0x1a, 0x04, 0x83, 0x40, - 0xbe, 0x27, 0x27, 0x15, 0xdd, 0x1c, 0x04, 0xc1, 0xe0, 0x0a, 0x25, 0xf1, 0x2c, 0xfe, 0xd8, 0xe3, - 0x9e, 0x8f, 0x11, 0xa7, 0xfe, 0x50, 0x01, 0x36, 0xf2, 0x00, 0x37, 0x0e, 0x29, 0xf7, 0x02, 0x26, - 0xdf, 0xcd, 0xaf, 0x1a, 0xac, 0x1e, 0x20, 0x3f, 0x4d, 0x32, 0xd9, 0x78, 0x1d, 0x63, 0xc4, 0xc9, - 0x3a, 0x54, 0x45, 0x66, 0xc7, 0x73, 0x75, 0xad, 0xad, 0x75, 0x6b, 0x76, 0x45, 0xdc, 0x5f, 0xba, - 0xe4, 0x11, 0x40, 0xc4, 0x69, 0xc8, 0x9d, 0x24, 0x8f, 0x5e, 0x68, 0x6b, 0xdd, 0x7a, 0xdf, 0xb0, - 0x64, 0x0e, 0x2b, 0xcd, 0x61, 0x9d, 0xa6, 0x45, 0xec, 0x97, 0x3e, 0x7f, 0xdb, 0xd4, 0xec, 0x9a, - 0xe0, 0x24, 0x51, 0xf2, 0x00, 0xaa, 0xc8, 0x5c, 0x49, 0x2f, 0x2e, 0x48, 0xaf, 0x20, 0x73, 0x93, - 0x98, 0x79, 0x01, 0xe4, 0x64, 0x48, 0x59, 0x64, 0x63, 0x34, 0x0c, 0x58, 0x84, 0x4f, 0x2f, 0x62, - 0x76, 0x49, 0x6c, 0x58, 0x09, 0x31, 0x0a, 0xe2, 0xf0, 0x1c, 0x9d, 0x28, 0x79, 0xd6, 0xb5, 0x76, - 0xb1, 0x5b, 0xef, 0xdf, 0xb1, 0x66, 0x9a, 0x2b, 0xf5, 0x2d, 0xd9, 0xd3, 0xd1, 0xb6, 0x65, 0x2b, - 0x8e, 0x54, 0x6c, 0x86, 0xd3, 0x57, 0xf3, 0x4b, 0x09, 0x5a, 0xa2, 0x27, 0xc7, 0x89, 0x87, 0x6f, - 0x68, 0x48, 0x7d, 0xe4, 0x18, 0x46, 0xa4, 0x03, 0x0d, 0x65, 0xa8, 0xc3, 0xa8, 0x8f, 0xaa, 0x3f, - 0x75, 0x15, 0x3b, 0xa2, 0x3e, 0x92, 0xff, 0x61, 0x25, 0x18, 0xa2, 0xec, 0xb2, 0x04, 0x15, 0x04, - 0xa8, 0x99, 0x45, 0x05, 0xec, 0x04, 0x80, 0x72, 0x1e, 0x7a, 0x67, 0x31, 0xc7, 0x48, 0x2f, 0x8a, - 0x92, 0x77, 0xac, 0x99, 0xf1, 0xb0, 0x7e, 0x56, 0x82, 0xf5, 0x24, 0x63, 0x3d, 0x67, 0x3c, 0x9c, - 0xd8, 0x53, 0x32, 0xe4, 0x31, 0xac, 0xdc, 0xf8, 0xe3, 0xf8, 0x1e, 0xd3, 0x4b, 0xbf, 0x6a, 0xb2, - 0xdd, 0xc8, 0xdc, 0x39, 0xf4, 0x58, 0x5e, 0x81, 0x8e, 0xf5, 0xf2, 0xef, 0x28, 0xd0, 0x31, 0xd9, - 0x83, 0x46, 0x3a, 0x64, 0xa2, 0x82, 0x65, 0xc1, 0x5f, 0x9f, 0xe3, 0x3f, 0x53, 0x20, 0xbb, 0x9e, - 0xc2, 0x93, 0xfc, 0x33, 0x6c, 0x3a, 0xd6, 0x2b, 0x8b, 0xb3, 0xe9, 0x98, 0xfc, 0x0b, 0xc0, 0x62, - 0xdf, 0x11, 0x26, 0x47, 0x7a, 0xb5, 0xad, 0x75, 0xcb, 0x76, 0x8d, 0xc5, 0xbe, 0x68, 0x64, 0x64, - 0x3c, 0x84, 0xd5, 0x5c, 0xf7, 0xc8, 0x1f, 0x50, 0xbc, 0xc4, 0x89, 0xf2, 0x31, 0x39, 0x92, 0x16, - 0x94, 0x47, 0xf4, 0x2a, 0x4e, 0x6d, 0x93, 0x97, 0xfb, 0x85, 0x5d, 0xcd, 0x3c, 0x82, 0x3f, 0x5f, - 0x78, 0xcc, 0x95, 0x62, 0xe9, 0xb6, 0xdc, 0x83, 0xb2, 0x58, 0x74, 0x21, 0x51, 0xef, 0x6f, 0x2d, - 0x60, 0xa1, 0x2d, 0x19, 0x66, 0x0b, 0xc8, 0x01, 0xf2, 0x13, 0x39, 0x3b, 0xa9, 0xa0, 0xb9, 0x0d, - 0x6b, 0x33, 0x51, 0x39, 0xeb, 0xc4, 0x80, 0xaa, 0x9a, 0x32, 0x39, 0xe0, 0x35, 0x3b, 0xbb, 0x9b, - 0x87, 0xd0, 0x3a, 0x40, 0xfe, 0x3a, 0x9d, 0xaf, 0xac, 0x36, 0x1d, 0x2a, 0x0a, 0x93, 0x2e, 0xb2, - 0xba, 0x92, 0x7f, 0xa0, 0x96, 0xec, 0x8a, 0x73, 0xe9, 0x31, 0x57, 0xfd, 0xd1, 0x6a, 0x12, 0x78, - 0xe5, 0x31, 0xd7, 0xdc, 0x83, 0x5a, 0xa6, 0x45, 0x08, 0x94, 0xa6, 0x26, 0x5d, 0x9c, 0x6f, 0x67, - 0x1f, 0xc3, 0x5f, 0xb9, 0x62, 0xd4, 0x3f, 0xd8, 0x05, 0xc8, 0x56, 0x20, 0x5d, 0x52, 0x3d, 0xd7, - 0xae, 0x8c, 0x66, 0x4f, 0x61, 0xfb, 0xdf, 0x0b, 0xd0, 0x10, 0x3d, 0x54, 0x5d, 0x21, 0xc7, 0x50, - 0x4d, 0xbf, 0x5a, 0x64, 0x23, 0x27, 0x91, 0xfb, 0x9c, 0x19, 0x9d, 0xdc, 0xfb, 0xfc, 0x27, 0xc4, - 0x5c, 0xba, 0xab, 0x91, 0xb7, 0x00, 0x37, 0xe6, 0x92, 0x76, 0x8e, 0x34, 0xe7, 0xfb, 0xa2, 0xb2, - 0xef, 0xa0, 0x3e, 0xe5, 0x26, 0xe9, 0xcc, 0x17, 0x9b, 0xf3, 0xdf, 0x30, 0x6f, 0x83, 0x48, 0x79, - 0x73, 0x89, 0x7c, 0x80, 0xe6, 0x4c, 0x97, 0xc9, 0xd6, 0x3c, 0x6d, 0x6e, 0x20, 0x8c, 0xff, 0x6e, - 0x07, 0xa5, 0xea, 0xfb, 0x1d, 0xf8, 0xdb, 0x0b, 0x14, 0x36, 0x59, 0x26, 0x8f, 0x0d, 0x14, 0xe5, - 0xfd, 0xb2, 0xfc, 0x3d, 0x5b, 0x16, 0xab, 0xb8, 0xf3, 0x23, 0x00, 0x00, 0xff, 0xff, 0xe0, 0xd2, - 0xc5, 0xf1, 0xe7, 0x06, 0x00, 0x00, + // 852 bytes of a gzipped FileDescriptorProto + 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x94, 0x55, 0xef, 0x6e, 0x1b, 0x45, + 0x10, 0xef, 0x39, 0x71, 0x62, 0x8f, 0x93, 0xb4, 0x6c, 0x8d, 0x7a, 0x3d, 0x44, 0xea, 0x5c, 0x41, + 0xb2, 0x84, 0x74, 0x21, 0xc9, 0x07, 0x5a, 0x28, 0x02, 0x9a, 0x16, 0x0b, 0xa1, 0xb4, 0xcd, 0xa6, + 0x80, 0x84, 0x90, 0x4e, 0x9b, 0xdc, 0xe0, 0x1c, 0xf1, 0xed, 0x5d, 0x77, 0xf7, 0x4c, 0xfc, 0x0c, + 0x7c, 0x41, 0xe2, 0x2d, 0x78, 0x29, 0xde, 0x00, 0x5e, 0x01, 0xed, 0x9f, 0xbb, 0xda, 0x67, 0x48, + 0xd3, 0x4f, 0xde, 0x99, 0xfd, 0xfd, 0x66, 0xe6, 0x7e, 0x33, 0xb3, 0x86, 0xdb, 0xaf, 0x4a, 0x14, + 0xb3, 0x58, 0xa2, 0x98, 0xa6, 0x67, 0x18, 0x15, 0x22, 0x57, 0x39, 0xd9, 0xfc, 0x85, 0xe1, 0x18, + 0x45, 0xc4, 0x8a, 0x34, 0x9e, 0x1e, 0x04, 0xc3, 0xbc, 0x40, 0xae, 0x70, 0x82, 0x19, 0x2a, 0x31, + 0xdb, 0x35, 0x98, 0x5d, 0x25, 0xd8, 0x19, 0xee, 0x4e, 0xf7, 0xec, 0xc1, 0x12, 0x83, 0xfe, 0x38, + 0x1f, 0xe7, 0xf6, 0x5e, 0x9f, 0x9c, 0xf7, 0xde, 0x38, 0xcf, 0xc7, 0x13, 0xb4, 0xc4, 0xd3, 0xf2, + 0xe7, 0x5d, 0x95, 0x66, 0x28, 0x15, 0xcb, 0x0a, 0x07, 0xd8, 0x6e, 0x02, 0x92, 0x52, 0x30, 0x95, + 0xe6, 0xdc, 0xde, 0x87, 0x7f, 0x7a, 0x70, 0x73, 0x84, 0xea, 0xa5, 0xce, 0x44, 0xf1, 0x55, 0x89, + 0x52, 0x91, 0xbb, 0xd0, 0x31, 0x99, 0xe3, 0x34, 0xf1, 0xbd, 0x81, 0x37, 0xec, 0xd2, 0x75, 0x63, + 0x7f, 0x93, 0x90, 0x2f, 0x00, 0xa4, 0x62, 0x42, 0xc5, 0x3a, 0x8f, 0xdf, 0x1a, 0x78, 0xc3, 0xde, + 0x7e, 0x10, 0xd9, 0x1c, 0x51, 0x95, 0x23, 0x7a, 0x59, 0x15, 0xf1, 0x78, 0xf5, 0xf7, 0xbf, 0xee, + 0x79, 0xb4, 0x6b, 0x38, 0xda, 0x4b, 0x3e, 0x83, 0x0e, 0xf2, 0xc4, 0xd2, 0x57, 0xae, 0x49, 0x5f, + 0x47, 0x9e, 0x68, 0x5f, 0x78, 0x0e, 0xe4, 0xa4, 0x60, 0x5c, 0x52, 0x94, 0x45, 0xce, 0x25, 0x1e, + 0x9e, 0x97, 0xfc, 0x82, 0x50, 0xd8, 0x12, 0x28, 0xf3, 0x52, 0x9c, 0x61, 0x2c, 0xf5, 0xb5, 0xef, + 0x0d, 0x56, 0x86, 0xbd, 0xfd, 0x8f, 0xa2, 0x05, 0x71, 0x6d, 0xfc, 0xc8, 0x6a, 0x3a, 0xdd, 0x8b, + 0xa8, 0xe3, 0xd8, 0x88, 0x9b, 0x62, 0xde, 0x0c, 0xff, 0x58, 0x85, 0xbe, 0xd1, 0xe4, 0x58, 0xf7, + 0xf0, 0x05, 0x13, 0x2c, 0x43, 0x85, 0x42, 0x92, 0x1d, 0xd8, 0x70, 0x0d, 0x8d, 0x39, 0xcb, 0xd0, + 0xe9, 0xd3, 0x73, 0xbe, 0x67, 0x2c, 0x43, 0xf2, 0x21, 0x6c, 0xe5, 0x05, 0x5a, 0x95, 0x2d, 0xa8, + 0x65, 0x40, 0x9b, 0xb5, 0xd7, 0xc0, 0x4e, 0x00, 0x98, 0x52, 0x22, 0x3d, 0x2d, 0x15, 0x4a, 0x7f, + 0xc5, 0x94, 0x7c, 0x10, 0x2d, 0x8c, 0x47, 0xf4, 0x5f, 0x25, 0x44, 0x5f, 0xd5, 0xac, 0xa7, 0x5c, + 0x89, 0x19, 0x9d, 0x0b, 0x43, 0xbe, 0x84, 0xad, 0xd7, 0xfd, 0x89, 0xb3, 0x94, 0xfb, 0xab, 0x6f, + 0x12, 0x99, 0x6e, 0xd4, 0xdd, 0x39, 0x4a, 0x79, 0x33, 0x02, 0xbb, 0xf4, 0xdb, 0x6f, 0x13, 0x81, + 0x5d, 0x92, 0x47, 0xb0, 0x51, 0x0d, 0x99, 0xa9, 0x60, 0xcd, 0xf0, 0xef, 0x2e, 0xf1, 0x9f, 0x38, + 0x10, 0xed, 0x55, 0x70, 0x9d, 0x7f, 0x81, 0xcd, 0x2e, 0xfd, 0xf5, 0xeb, 0xb3, 0xd9, 0x25, 0x79, + 0x1f, 0x80, 0x97, 0x59, 0x6c, 0x9a, 0x2c, 0xfd, 0xce, 0xc0, 0x1b, 0xb6, 0x69, 0x97, 0x97, 0x99, + 0x11, 0x52, 0x06, 0x9f, 0xc3, 0xcd, 0x86, 0x7a, 0xe4, 0x16, 0xac, 0x5c, 0xe0, 0xcc, 0xf5, 0x51, + 0x1f, 0x49, 0x1f, 0xda, 0x53, 0x36, 0x29, 0xab, 0xb6, 0x59, 0xe3, 0xd3, 0xd6, 0x03, 0x2f, 0x7c, + 0x06, 0xef, 0x7c, 0x9d, 0xf2, 0xc4, 0x06, 0xab, 0xb6, 0xe5, 0x21, 0xb4, 0xcd, 0xa2, 0x9b, 0x10, + 0xbd, 0xfd, 0xfb, 0xd7, 0x68, 0x21, 0xb5, 0x8c, 0xb0, 0x0f, 0x64, 0x84, 0xea, 0xc4, 0xce, 0x4e, + 0x15, 0x30, 0xdc, 0x83, 0xdb, 0x0b, 0x5e, 0x3b, 0xeb, 0x24, 0x80, 0x8e, 0x9b, 0x32, 0x3b, 0xe0, + 0x5d, 0x5a, 0xdb, 0xe1, 0x11, 0xf4, 0x47, 0xa8, 0x9e, 0x57, 0xf3, 0x55, 0xd7, 0xe6, 0xc3, 0xba, + 0xc3, 0x54, 0x8b, 0xec, 0x4c, 0xf2, 0x1e, 0x74, 0xf5, 0xae, 0xc4, 0x17, 0x29, 0x4f, 0xdc, 0x87, + 0x76, 0xb4, 0xe3, 0xdb, 0x94, 0x27, 0xe1, 0x23, 0xe8, 0xd6, 0xb1, 0x08, 0x81, 0xd5, 0xb9, 0x49, + 0x37, 0xe7, 0xab, 0xd9, 0xc7, 0xf0, 0x6e, 0xa3, 0x18, 0xf7, 0x05, 0x0f, 0x00, 0xea, 0x15, 0xa8, + 0x96, 0xd4, 0x6f, 0xc8, 0x55, 0xd3, 0xe8, 0x1c, 0x36, 0xfc, 0xc7, 0x83, 0x5b, 0x23, 0xfa, 0xe2, + 0x70, 0xc4, 0x14, 0xfe, 0xca, 0x66, 0x4f, 0x85, 0xc8, 0x05, 0x39, 0x82, 0x36, 0xea, 0x83, 0x13, + 0xfe, 0x93, 0x46, 0xa4, 0x26, 0x7e, 0xc9, 0xf1, 0x04, 0x15, 0x4b, 0x27, 0x92, 0xda, 0x28, 0xc1, + 0x6f, 0x1e, 0xdc, 0xf9, 0x1f, 0x88, 0xd6, 0x7e, 0x2c, 0x8a, 0xb3, 0xc3, 0x3c, 0xb1, 0x3a, 0xb4, + 0x69, 0x6d, 0xeb, 0xbb, 0x73, 0xa5, 0x0a, 0x73, 0xd7, 0xb2, 0x77, 0x95, 0xad, 0xf5, 0xcf, 0x50, + 0x4a, 0x36, 0xb6, 0x8f, 0x5d, 0x97, 0x56, 0x26, 0xd9, 0x06, 0xd0, 0xa8, 0x13, 0xc5, 0x54, 0x29, + 0xcd, 0x92, 0x76, 0xe9, 0x9c, 0x27, 0x7c, 0x0e, 0x64, 0xae, 0x98, 0x1f, 0x04, 0x2b, 0x0a, 0x14, + 0xe4, 0x21, 0xac, 0x09, 0x94, 0xe5, 0x44, 0xb9, 0x6f, 0xde, 0x69, 0x7c, 0xf3, 0xf2, 0xeb, 0x48, + 0x1d, 0x61, 0xff, 0xef, 0x16, 0x6c, 0x98, 0x31, 0x74, 0x83, 0x45, 0x8e, 0xa1, 0x53, 0x3d, 0xfc, + 0x64, 0xbb, 0xa9, 0xdd, 0xe2, 0x3f, 0x42, 0xf0, 0xe6, 0x3c, 0xe1, 0x8d, 0x8f, 0x3d, 0xf2, 0x1d, + 0xc0, 0xeb, 0xfd, 0x20, 0x83, 0x06, 0x69, 0x69, 0x75, 0xae, 0x1b, 0xf6, 0x7b, 0xe8, 0xcd, 0x2d, + 0x04, 0xd9, 0x59, 0x2e, 0xb6, 0xb1, 0x42, 0x41, 0x78, 0x15, 0xc4, 0x86, 0x0f, 0x6f, 0x90, 0x9f, + 0x60, 0x73, 0x61, 0x50, 0xc9, 0xfd, 0x65, 0xda, 0xd2, 0x4e, 0x05, 0x1f, 0x5c, 0x0d, 0xaa, 0xa2, + 0x3f, 0xde, 0x81, 0x3b, 0x69, 0xee, 0xb0, 0xfa, 0x3d, 0x4a, 0xf9, 0xd8, 0x51, 0x7e, 0x5c, 0xb3, + 0xbf, 0xa7, 0x6b, 0xe6, 0x35, 0x3b, 0xf8, 0x37, 0x00, 0x00, 0xff, 0xff, 0x01, 0x5e, 0x3d, 0xbf, + 0x2a, 0x08, 0x00, 0x00, } // Reference imports to suppress errors if they are not otherwise used. From 8e6a3940431183b2ea60cd70774cc75f067b2e7d Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Thu, 28 Dec 2023 00:24:21 -0500 Subject: [PATCH 02/15] fix Signed-off-by: Yuri Shkuro --- cmd/query/app/apiv3/http_gateway.go | 227 +++++++++++++++++++++++ cmd/query/app/apiv3/http_gateway_test.go | 64 +++++++ 2 files changed, 291 insertions(+) create mode 100644 cmd/query/app/apiv3/http_gateway.go create mode 100644 cmd/query/app/apiv3/http_gateway_test.go diff --git a/cmd/query/app/apiv3/http_gateway.go b/cmd/query/app/apiv3/http_gateway.go new file mode 100644 index 00000000000..3411ac5f10e --- /dev/null +++ b/cmd/query/app/apiv3/http_gateway.go @@ -0,0 +1,227 @@ +// Copyright (c) 2023 The Jaeger Authors. +// SPDX-License-Identifier: Apache-2.0 + +package apiv3 + +import ( + "encoding/json" + "errors" + "fmt" + "net/http" + "strconv" + "time" + + "github.com/gogo/protobuf/jsonpb" + "github.com/gogo/protobuf/proto" + "github.com/gorilla/mux" + "go.uber.org/zap" + + "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" + "github.com/jaegertracing/jaeger/model" + "github.com/jaegertracing/jaeger/pkg/tenancy" + "github.com/jaegertracing/jaeger/proto-gen/api_v3" + "github.com/jaegertracing/jaeger/storage/spanstore" +) + +const ( + traceIDParam = "traceID" + + routeGetTrace = "/api/v3/traces/{traceID}" + routeFindTraces = "/api/v3/traces" + routeGetServices = "/api/v3/services" + routeGetOperations = "/api/v3/operations" +) + +// HTTPGateway exposes APIv3 HTTP endpoints. +type HTTPGateway struct { + QueryService *querysvc.QueryService + TenancyMgr *tenancy.Manager + Logger *zap.Logger +} + +// RegisterRoutes registers HTTP endpoints for APIv3 into provided mux. +func (h *HTTPGateway) RegisterRoutes(router *mux.Router) { + h.addRoute(router, h.getTrace, routeGetTrace).Methods(http.MethodGet) + h.addRoute(router, h.findTraces, routeFindTraces).Methods(http.MethodGet) + h.addRoute(router, h.getServices, routeGetServices).Methods(http.MethodGet) + h.addRoute(router, h.getOperations, routeGetOperations).Methods(http.MethodGet) +} + +// addRoute adds a new endpoint to the router with given path and handler function. +// This code is mostly copied from ../http_handler. +// TODO add tracing middleware. +func (h *HTTPGateway) addRoute( + router *mux.Router, + f func(http.ResponseWriter, *http.Request), + route string, + args ...interface{}, +) *mux.Route { + // route := aH.formatRoute(routeFmt, args...) + var handler http.Handler = http.HandlerFunc(f) + if h.TenancyMgr.Enabled { + handler = tenancy.ExtractTenantHTTPHandler(h.TenancyMgr, handler) + } + // traceMiddleware := otelhttp.NewHandler( + // otelhttp.WithRouteTag(route, traceResponseHandler(handler)), + // route, + // otelhttp.WithTracerProvider(aH.tracer.OTEL)) + // return router.HandleFunc(route, traceMiddleware.ServeHTTP) + return router.HandleFunc(route, handler.ServeHTTP) +} + +// tryHandleError checks if the passed error is not nil and handles it by writing +// an error response to the client. Otherwise it returns false. +func (h *HTTPGateway) tryHandleError(w http.ResponseWriter, err error, statusCode int) bool { + if err == nil { + return false + } + if errors.Is(err, spanstore.ErrTraceNotFound) { + statusCode = http.StatusNotFound + } + if statusCode == http.StatusInternalServerError { + h.Logger.Error("HTTP handler, Internal Server Error", zap.Error(err)) + } + errorResponse := api_v3.GRPCGatewayError{ + Error: &api_v3.GRPCGatewayError_GRPCGatewayErrorDetails{ + HttpCode: int32(statusCode), + Message: err.Error(), + }, + } + resp, _ := json.Marshal(&errorResponse) + http.Error(w, string(resp), statusCode) + return true +} + +func (h *HTTPGateway) returnSpans(spans []*model.Span, w http.ResponseWriter) { + resourceSpans, err := modelToOTLP(spans) + if h.tryHandleError(w, err, http.StatusInternalServerError) { + return + } + for _, rs := range resourceSpans { + for _, ss := range rs.ScopeSpans { + for _, s := range ss.Spans { + if len(s.ParentSpanId) == 0 { + // If ParentSpanId is empty array then gogo/jsonpb renders it as empty string. + // To match the output with grpc-gateway we set it to nil and it won't be included. + s.ParentSpanId = nil + } + } + } + } + response := &api_v3.GRPCGatewayWrapper{ + Result: &api_v3.SpansResponseChunk{ + ResourceSpans: resourceSpans, + }, + } + + h.marshalResponse(response, w) +} + +func (h *HTTPGateway) marshalResponse(response proto.Message, w http.ResponseWriter) { + m := &jsonpb.Marshaler{ + EmitDefaults: false, + } + _ = m.Marshal(w, response) +} + +func (h *HTTPGateway) getTrace(w http.ResponseWriter, r *http.Request) { + vars := mux.Vars(r) + traceIDVar := vars[traceIDParam] + traceID, err := model.TraceIDFromString(traceIDVar) + if h.tryHandleError(w, err, http.StatusBadRequest) { + return + } + trace, err := h.QueryService.GetTrace(r.Context(), traceID) + // TODO how do we distinguish internal error from bad parameters for FindTrace? + if h.tryHandleError(w, err, http.StatusInternalServerError) { + return + } + h.returnSpans(trace.Spans, w) +} + +func (h *HTTPGateway) findTraces(w http.ResponseWriter, r *http.Request) { + query := r.URL.Query() + queryParams := &spanstore.TraceQueryParameters{ + ServiceName: query.Get("query.service_name"), + OperationName: query.Get("query.operation_name"), + Tags: nil, // most curiously not supported by grpc-gateway + } + if n := query.Get("query.num_traces"); n != "" { + numTraces, err := strconv.Atoi(n) + if h.tryHandleError(w, err, http.StatusBadRequest) { + return + } + queryParams.NumTraces = numTraces + } + timeMin := query.Get("query.start_time_min") + timeMax := query.Get("query.start_time_max") + if timeMin == "" || timeMax == "" { + err := fmt.Errorf("query.start_time_min and query.start_time_max are required") + h.tryHandleError(w, err, http.StatusBadRequest) + return + } + timeMinParsed, err := time.Parse(time.RFC3339Nano, timeMin) + if h.tryHandleError(w, err, http.StatusBadRequest) { + return + } + queryParams.StartTimeMin = timeMinParsed + timeMaxParsed, err := time.Parse(time.RFC3339Nano, timeMax) + if h.tryHandleError(w, err, http.StatusBadRequest) { + return + } + queryParams.StartTimeMax = timeMaxParsed + if d := query.Get("duration_min"); d != "" { + dur, err := time.ParseDuration(d) + if h.tryHandleError(w, err, http.StatusBadRequest) { + return + } + queryParams.DurationMin = dur + } + if d := query.Get("duration_max"); d != "" { + dur, err := time.ParseDuration(d) + if h.tryHandleError(w, err, http.StatusBadRequest) { + return + } + queryParams.DurationMax = dur + } + + traces, err := h.QueryService.FindTraces(r.Context(), queryParams) + if h.tryHandleError(w, err, http.StatusInternalServerError) { + return + } + var spans []*model.Span + for _, trace := range traces { + spans = append(spans, trace.Spans...) + } + h.returnSpans(spans, w) +} + +func (h *HTTPGateway) getServices(w http.ResponseWriter, r *http.Request) { + services, err := h.QueryService.GetServices(r.Context()) + if h.tryHandleError(w, err, http.StatusInternalServerError) { + return + } + h.marshalResponse(&api_v3.GetServicesResponse{ + Services: services, + }, w) +} + +func (h *HTTPGateway) getOperations(w http.ResponseWriter, r *http.Request) { + query := r.URL.Query() + queryParams := spanstore.OperationQueryParameters{ + ServiceName: query.Get("service"), + SpanKind: query.Get("span_kind"), + } + operations, err := h.QueryService.GetOperations(r.Context(), queryParams) + if h.tryHandleError(w, err, http.StatusInternalServerError) { + return + } + apiOperations := make([]*api_v3.Operation, len(operations)) + for i := range operations { + apiOperations[i] = &api_v3.Operation{ + Name: operations[i].Name, + SpanKind: operations[i].SpanKind, + } + } + h.marshalResponse(&api_v3.GetOperationsResponse{Operations: apiOperations}, w) +} diff --git a/cmd/query/app/apiv3/http_gateway_test.go b/cmd/query/app/apiv3/http_gateway_test.go new file mode 100644 index 00000000000..bdfd4142f25 --- /dev/null +++ b/cmd/query/app/apiv3/http_gateway_test.go @@ -0,0 +1,64 @@ +// Copyright (c) 2023 The Jaeger Authors. +// SPDX-License-Identifier: Apache-2.0 + +package apiv3 + +import ( + "net/http/httptest" + "testing" + + "github.com/gorilla/mux" + "go.uber.org/zap" + + "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" + "github.com/jaegertracing/jaeger/pkg/config/tlscfg" + "github.com/jaegertracing/jaeger/pkg/tenancy" + dependencyStoreMocks "github.com/jaegertracing/jaeger/storage/dependencystore/mocks" + spanstoremocks "github.com/jaegertracing/jaeger/storage/spanstore/mocks" +) + +func setupHTTPGateway( + t *testing.T, + basePath string, + serverTLS, clientTLS *tlscfg.Options, + tenancyOptions tenancy.Options, +) *testGateway { + gw := &testGateway{ + reader: &spanstoremocks.Reader{}, + } + + q := querysvc.NewQueryService(gw.reader, + &dependencyStoreMocks.Reader{}, + querysvc.QueryServiceOptions{}, + ) + + hgw := &HTTPGateway{ + QueryService: q, + TenancyMgr: tenancy.NewManager(&tenancyOptions), + Logger: zap.NewNop(), + } + + router := &mux.Router{} + if basePath != "" && basePath != "/" { + router = router.PathPrefix(basePath).Subrouter() + } + hgw.RegisterRoutes(router) + + httpServer := httptest.NewServer(router) + t.Cleanup(func() { httpServer.Close() }) + + t.Logf("HTTP Gateway listening on %s", httpServer.URL) + gw.url = httpServer.URL + if basePath != "/" { + gw.url += basePath + } + return gw +} + +func TestHTTPGateway(t *testing.T) { + useHTTPGateway = true + t.Cleanup(func() { useHTTPGateway = false }) + t.Run("TestGRPCGateway", TestGRPCGateway) + t.Run("TestGRPCGatewayWithTenancy", TestGRPCGatewayWithTenancy) + t.Run("TestGRPCGatewayTenancyRejection", TestGRPCGatewayTenancyRejection) +} From d5a13cf35e9c023690a9a7b1334b01744187fead Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Thu, 28 Dec 2023 00:29:35 -0500 Subject: [PATCH 03/15] Reduce number of snapshots Signed-off-by: Yuri Shkuro --- cmd/query/app/apiv3/grpc_gateway_test.go | 11 ++++----- cmd/query/app/apiv3/http_gateway_test.go | 1 - ...AndTLS_FindTraces.json => FindTraces.json} | 0 ..._GetOperations.json => GetOperations.json} | 0 ...dTLS_GetServices.json => GetServices.json} | 0 ...PathAndTLS_GetTrace.json => GetTrace.json} | 0 ...TestGRPCGatewayWithTenancy_FindTraces.json | 24 ------------------- ...tGRPCGatewayWithTenancy_GetOperations.json | 8 ------- ...estGRPCGatewayWithTenancy_GetServices.json | 5 ---- .../TestGRPCGatewayWithTenancy_GetTrace.json | 24 ------------------- .../snapshots/TestGRPCGateway_FindTraces.json | 24 ------------------- .../TestGRPCGateway_GetOperations.json | 8 ------- .../TestGRPCGateway_GetServices.json | 5 ---- .../snapshots/TestGRPCGateway_GetTrace.json | 24 ------------------- 14 files changed, 4 insertions(+), 130 deletions(-) rename cmd/query/app/apiv3/snapshots/{TestGRPCGatewayWithBasePathAndTLS_FindTraces.json => FindTraces.json} (100%) rename cmd/query/app/apiv3/snapshots/{TestGRPCGatewayWithBasePathAndTLS_GetOperations.json => GetOperations.json} (100%) rename cmd/query/app/apiv3/snapshots/{TestGRPCGatewayWithBasePathAndTLS_GetServices.json => GetServices.json} (100%) rename cmd/query/app/apiv3/snapshots/{TestGRPCGatewayWithBasePathAndTLS_GetTrace.json => GetTrace.json} (100%) delete mode 100644 cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithTenancy_FindTraces.json delete mode 100644 cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithTenancy_GetOperations.json delete mode 100644 cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithTenancy_GetServices.json delete mode 100644 cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithTenancy_GetTrace.json delete mode 100644 cmd/query/app/apiv3/snapshots/TestGRPCGateway_FindTraces.json delete mode 100644 cmd/query/app/apiv3/snapshots/TestGRPCGateway_GetOperations.json delete mode 100644 cmd/query/app/apiv3/snapshots/TestGRPCGateway_GetServices.json delete mode 100644 cmd/query/app/apiv3/snapshots/TestGRPCGateway_GetTrace.json diff --git a/cmd/query/app/apiv3/grpc_gateway_test.go b/cmd/query/app/apiv3/grpc_gateway_test.go index bb9698395e1..f5f9a09773c 100644 --- a/cmd/query/app/apiv3/grpc_gateway_test.go +++ b/cmd/query/app/apiv3/grpc_gateway_test.go @@ -24,6 +24,7 @@ import ( "net/http" "net/url" "os" + "path" "path/filepath" "strings" "testing" @@ -60,9 +61,8 @@ const ( // REGENERATE_SNAPSHOTS=true go test -v ./cmd/query/app/apiv3/... var regenerateSnapshots = os.Getenv("REGENERATE_SNAPSHOTS") == "true" -// When run with USE_APIV3_HTTP_GATEWAY=true, the HTTP gateway is -// used for tests instead of grpc-gateway. -var useHTTPGateway = os.Getenv("USE_APIV3_HTTP_GATEWAY") == "true" +// The tests in http_gateway_test.go set this to true to use manual gateway implementation. +var useHTTPGateway = false type testGateway struct { reader *spanstoremocks.Reader @@ -152,7 +152,6 @@ func setupGRPCGateway( func (gw *testGateway) execRequest(t *testing.T, gwReq *gatewayRequest) ([]byte, int) { url := gw.url + gwReq.url - t.Logf("executing request %s", url) req, err := http.NewRequest(http.MethodGet, url, nil) require.NoError(t, err) req.Header.Set("Content-Type", "application/json") @@ -172,9 +171,7 @@ func verifySnapshot(t *testing.T, body []byte) []byte { body, err := json.MarshalIndent(data, "", " ") require.NoError(t, err) - testName := strings.ReplaceAll(t.Name(), "/", "_") - testName = strings.ReplaceAll(testName, "TestHTTPGateway_", "") // use same fixtures - // TODO strip all test names except the last one, the fixtures are likely identical + testName := path.Base(t.Name()) snapshotFile := filepath.Join(snapshotLocation, testName+".json") if regenerateSnapshots { os.WriteFile(snapshotFile, body, 0o644) diff --git a/cmd/query/app/apiv3/http_gateway_test.go b/cmd/query/app/apiv3/http_gateway_test.go index bdfd4142f25..024a5fb582e 100644 --- a/cmd/query/app/apiv3/http_gateway_test.go +++ b/cmd/query/app/apiv3/http_gateway_test.go @@ -47,7 +47,6 @@ func setupHTTPGateway( httpServer := httptest.NewServer(router) t.Cleanup(func() { httpServer.Close() }) - t.Logf("HTTP Gateway listening on %s", httpServer.URL) gw.url = httpServer.URL if basePath != "/" { gw.url += basePath diff --git a/cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithBasePathAndTLS_FindTraces.json b/cmd/query/app/apiv3/snapshots/FindTraces.json similarity index 100% rename from cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithBasePathAndTLS_FindTraces.json rename to cmd/query/app/apiv3/snapshots/FindTraces.json diff --git a/cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithBasePathAndTLS_GetOperations.json b/cmd/query/app/apiv3/snapshots/GetOperations.json similarity index 100% rename from cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithBasePathAndTLS_GetOperations.json rename to cmd/query/app/apiv3/snapshots/GetOperations.json diff --git a/cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithBasePathAndTLS_GetServices.json b/cmd/query/app/apiv3/snapshots/GetServices.json similarity index 100% rename from cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithBasePathAndTLS_GetServices.json rename to cmd/query/app/apiv3/snapshots/GetServices.json diff --git a/cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithBasePathAndTLS_GetTrace.json b/cmd/query/app/apiv3/snapshots/GetTrace.json similarity index 100% rename from cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithBasePathAndTLS_GetTrace.json rename to cmd/query/app/apiv3/snapshots/GetTrace.json diff --git a/cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithTenancy_FindTraces.json b/cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithTenancy_FindTraces.json deleted file mode 100644 index 5883e50eb1f..00000000000 --- a/cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithTenancy_FindTraces.json +++ /dev/null @@ -1,24 +0,0 @@ -{ - "result": { - "resourceSpans": [ - { - "resource": {}, - "scopeSpans": [ - { - "scope": {}, - "spans": [ - { - "endTimeUnixNano": "11651379494838206464", - "name": "foobar", - "spanId": "AAAAAAAAALQ=", - "startTimeUnixNano": "11651379494838206464", - "status": {}, - "traceId": "AAAAAAAAAJYAAAAAAAAAoA==" - } - ] - } - ] - } - ] - } -} \ No newline at end of file diff --git a/cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithTenancy_GetOperations.json b/cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithTenancy_GetOperations.json deleted file mode 100644 index f56d8454189..00000000000 --- a/cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithTenancy_GetOperations.json +++ /dev/null @@ -1,8 +0,0 @@ -{ - "operations": [ - { - "name": "get_users", - "spanKind": "server" - } - ] -} \ No newline at end of file diff --git a/cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithTenancy_GetServices.json b/cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithTenancy_GetServices.json deleted file mode 100644 index 6a631d75f06..00000000000 --- a/cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithTenancy_GetServices.json +++ /dev/null @@ -1,5 +0,0 @@ -{ - "services": [ - "foo" - ] -} \ No newline at end of file diff --git a/cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithTenancy_GetTrace.json b/cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithTenancy_GetTrace.json deleted file mode 100644 index 5883e50eb1f..00000000000 --- a/cmd/query/app/apiv3/snapshots/TestGRPCGatewayWithTenancy_GetTrace.json +++ /dev/null @@ -1,24 +0,0 @@ -{ - "result": { - "resourceSpans": [ - { - "resource": {}, - "scopeSpans": [ - { - "scope": {}, - "spans": [ - { - "endTimeUnixNano": "11651379494838206464", - "name": "foobar", - "spanId": "AAAAAAAAALQ=", - "startTimeUnixNano": "11651379494838206464", - "status": {}, - "traceId": "AAAAAAAAAJYAAAAAAAAAoA==" - } - ] - } - ] - } - ] - } -} \ No newline at end of file diff --git a/cmd/query/app/apiv3/snapshots/TestGRPCGateway_FindTraces.json b/cmd/query/app/apiv3/snapshots/TestGRPCGateway_FindTraces.json deleted file mode 100644 index 5883e50eb1f..00000000000 --- a/cmd/query/app/apiv3/snapshots/TestGRPCGateway_FindTraces.json +++ /dev/null @@ -1,24 +0,0 @@ -{ - "result": { - "resourceSpans": [ - { - "resource": {}, - "scopeSpans": [ - { - "scope": {}, - "spans": [ - { - "endTimeUnixNano": "11651379494838206464", - "name": "foobar", - "spanId": "AAAAAAAAALQ=", - "startTimeUnixNano": "11651379494838206464", - "status": {}, - "traceId": "AAAAAAAAAJYAAAAAAAAAoA==" - } - ] - } - ] - } - ] - } -} \ No newline at end of file diff --git a/cmd/query/app/apiv3/snapshots/TestGRPCGateway_GetOperations.json b/cmd/query/app/apiv3/snapshots/TestGRPCGateway_GetOperations.json deleted file mode 100644 index f56d8454189..00000000000 --- a/cmd/query/app/apiv3/snapshots/TestGRPCGateway_GetOperations.json +++ /dev/null @@ -1,8 +0,0 @@ -{ - "operations": [ - { - "name": "get_users", - "spanKind": "server" - } - ] -} \ No newline at end of file diff --git a/cmd/query/app/apiv3/snapshots/TestGRPCGateway_GetServices.json b/cmd/query/app/apiv3/snapshots/TestGRPCGateway_GetServices.json deleted file mode 100644 index 6a631d75f06..00000000000 --- a/cmd/query/app/apiv3/snapshots/TestGRPCGateway_GetServices.json +++ /dev/null @@ -1,5 +0,0 @@ -{ - "services": [ - "foo" - ] -} \ No newline at end of file diff --git a/cmd/query/app/apiv3/snapshots/TestGRPCGateway_GetTrace.json b/cmd/query/app/apiv3/snapshots/TestGRPCGateway_GetTrace.json deleted file mode 100644 index 5883e50eb1f..00000000000 --- a/cmd/query/app/apiv3/snapshots/TestGRPCGateway_GetTrace.json +++ /dev/null @@ -1,24 +0,0 @@ -{ - "result": { - "resourceSpans": [ - { - "resource": {}, - "scopeSpans": [ - { - "scope": {}, - "spans": [ - { - "endTimeUnixNano": "11651379494838206464", - "name": "foobar", - "spanId": "AAAAAAAAALQ=", - "startTimeUnixNano": "11651379494838206464", - "status": {}, - "traceId": "AAAAAAAAAJYAAAAAAAAAoA==" - } - ] - } - ] - } - ] - } -} \ No newline at end of file From a05917f756c6da52a14128187b9fd4eddd17eefb Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Thu, 28 Dec 2023 00:32:23 -0500 Subject: [PATCH 04/15] comment Signed-off-by: Yuri Shkuro --- cmd/query/app/apiv3/http_gateway.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/query/app/apiv3/http_gateway.go b/cmd/query/app/apiv3/http_gateway.go index 3411ac5f10e..abddc1b004c 100644 --- a/cmd/query/app/apiv3/http_gateway.go +++ b/cmd/query/app/apiv3/http_gateway.go @@ -132,7 +132,6 @@ func (h *HTTPGateway) getTrace(w http.ResponseWriter, r *http.Request) { return } trace, err := h.QueryService.GetTrace(r.Context(), traceID) - // TODO how do we distinguish internal error from bad parameters for FindTrace? if h.tryHandleError(w, err, http.StatusInternalServerError) { return } @@ -186,6 +185,7 @@ func (h *HTTPGateway) findTraces(w http.ResponseWriter, r *http.Request) { } traces, err := h.QueryService.FindTraces(r.Context(), queryParams) + // TODO how do we distinguish internal error from bad parameters for FindTrace? if h.tryHandleError(w, err, http.StatusInternalServerError) { return } From 7b31e4ce8c0a9cb12893f5c9bdedf86d7007902c Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Thu, 28 Dec 2023 00:40:30 -0500 Subject: [PATCH 05/15] simpler Signed-off-by: Yuri Shkuro --- cmd/query/app/apiv3/grpc_gateway_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd/query/app/apiv3/grpc_gateway_test.go b/cmd/query/app/apiv3/grpc_gateway_test.go index f5f9a09773c..a9c290370d4 100644 --- a/cmd/query/app/apiv3/grpc_gateway_test.go +++ b/cmd/query/app/apiv3/grpc_gateway_test.go @@ -151,8 +151,7 @@ func setupGRPCGateway( } func (gw *testGateway) execRequest(t *testing.T, gwReq *gatewayRequest) ([]byte, int) { - url := gw.url + gwReq.url - req, err := http.NewRequest(http.MethodGet, url, nil) + req, err := http.NewRequest(http.MethodGet, gw.url+gwReq.url, nil) require.NoError(t, err) req.Header.Set("Content-Type", "application/json") gwReq.setupRequest(req) From f60b976ad5fcc191100df220344d96591e50e936 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Thu, 28 Dec 2023 00:50:27 -0500 Subject: [PATCH 06/15] Add tracing middleware Signed-off-by: Yuri Shkuro --- cmd/query/app/apiv3/http_gateway.go | 22 ++++++++++------------ cmd/query/app/apiv3/http_gateway_test.go | 2 ++ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/cmd/query/app/apiv3/http_gateway.go b/cmd/query/app/apiv3/http_gateway.go index abddc1b004c..37c0a60c095 100644 --- a/cmd/query/app/apiv3/http_gateway.go +++ b/cmd/query/app/apiv3/http_gateway.go @@ -14,10 +14,12 @@ import ( "github.com/gogo/protobuf/jsonpb" "github.com/gogo/protobuf/proto" "github.com/gorilla/mux" + "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" "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/proto-gen/api_v3" "github.com/jaegertracing/jaeger/storage/spanstore" @@ -37,9 +39,11 @@ type HTTPGateway struct { QueryService *querysvc.QueryService TenancyMgr *tenancy.Manager Logger *zap.Logger + Tracer *jtracer.JTracer } // RegisterRoutes registers HTTP endpoints for APIv3 into provided mux. +// The called can create a subrouter if it needs to prepend a base path. func (h *HTTPGateway) RegisterRoutes(router *mux.Router) { h.addRoute(router, h.getTrace, routeGetTrace).Methods(http.MethodGet) h.addRoute(router, h.findTraces, routeFindTraces).Methods(http.MethodGet) @@ -49,24 +53,21 @@ func (h *HTTPGateway) RegisterRoutes(router *mux.Router) { // addRoute adds a new endpoint to the router with given path and handler function. // This code is mostly copied from ../http_handler. -// TODO add tracing middleware. func (h *HTTPGateway) addRoute( router *mux.Router, f func(http.ResponseWriter, *http.Request), route string, args ...interface{}, ) *mux.Route { - // route := aH.formatRoute(routeFmt, args...) var handler http.Handler = http.HandlerFunc(f) if h.TenancyMgr.Enabled { handler = tenancy.ExtractTenantHTTPHandler(h.TenancyMgr, handler) } - // traceMiddleware := otelhttp.NewHandler( - // otelhttp.WithRouteTag(route, traceResponseHandler(handler)), - // route, - // otelhttp.WithTracerProvider(aH.tracer.OTEL)) - // return router.HandleFunc(route, traceMiddleware.ServeHTTP) - return router.HandleFunc(route, handler.ServeHTTP) + traceMiddleware := otelhttp.NewHandler( + otelhttp.WithRouteTag(route, handler), + route, + otelhttp.WithTracerProvider(h.Tracer.OTEL)) + return router.HandleFunc(route, traceMiddleware.ServeHTTP) } // tryHandleError checks if the passed error is not nil and handles it by writing @@ -118,10 +119,7 @@ func (h *HTTPGateway) returnSpans(spans []*model.Span, w http.ResponseWriter) { } func (h *HTTPGateway) marshalResponse(response proto.Message, w http.ResponseWriter) { - m := &jsonpb.Marshaler{ - EmitDefaults: false, - } - _ = m.Marshal(w, response) + _ = new(jsonpb.Marshaler).Marshal(w, response) } func (h *HTTPGateway) getTrace(w http.ResponseWriter, r *http.Request) { diff --git a/cmd/query/app/apiv3/http_gateway_test.go b/cmd/query/app/apiv3/http_gateway_test.go index 024a5fb582e..ff8dcd8f081 100644 --- a/cmd/query/app/apiv3/http_gateway_test.go +++ b/cmd/query/app/apiv3/http_gateway_test.go @@ -12,6 +12,7 @@ import ( "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" "github.com/jaegertracing/jaeger/pkg/config/tlscfg" + "github.com/jaegertracing/jaeger/pkg/jtracer" "github.com/jaegertracing/jaeger/pkg/tenancy" dependencyStoreMocks "github.com/jaegertracing/jaeger/storage/dependencystore/mocks" spanstoremocks "github.com/jaegertracing/jaeger/storage/spanstore/mocks" @@ -36,6 +37,7 @@ func setupHTTPGateway( QueryService: q, TenancyMgr: tenancy.NewManager(&tenancyOptions), Logger: zap.NewNop(), + Tracer: jtracer.NoOp(), } router := &mux.Router{} From fcb1fddc21431653eed6d91b92538b3eecc12e5a Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Thu, 28 Dec 2023 02:06:53 -0500 Subject: [PATCH 07/15] remove if Signed-off-by: Yuri Shkuro --- cmd/query/app/apiv3/package_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cmd/query/app/apiv3/package_test.go b/cmd/query/app/apiv3/package_test.go index 3326271251b..c56173a61a1 100644 --- a/cmd/query/app/apiv3/package_test.go +++ b/cmd/query/app/apiv3/package_test.go @@ -10,7 +10,5 @@ import ( ) func TestMain(m *testing.M) { - if true { - goleak.VerifyTestMain(m) - } + goleak.VerifyTestMain(m) } From 198e5ae79505400f3aca9df4ea850ec234b11248 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Thu, 28 Dec 2023 10:13:51 -0500 Subject: [PATCH 08/15] tests Signed-off-by: Yuri Shkuro --- cmd/query/app/apiv3/http_gateway.go | 2 +- cmd/query/app/apiv3/http_gateway_test.go | 50 ++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/cmd/query/app/apiv3/http_gateway.go b/cmd/query/app/apiv3/http_gateway.go index 37c0a60c095..15ad79b50ab 100644 --- a/cmd/query/app/apiv3/http_gateway.go +++ b/cmd/query/app/apiv3/http_gateway.go @@ -126,7 +126,7 @@ func (h *HTTPGateway) getTrace(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) traceIDVar := vars[traceIDParam] traceID, err := model.TraceIDFromString(traceIDVar) - if h.tryHandleError(w, err, http.StatusBadRequest) { + if err != nil && h.tryHandleError(w, fmt.Errorf("malformed trace_id: %w", err), http.StatusBadRequest) { return } trace, err := h.QueryService.GetTrace(r.Context(), traceID) diff --git a/cmd/query/app/apiv3/http_gateway_test.go b/cmd/query/app/apiv3/http_gateway_test.go index ff8dcd8f081..405dafa35a7 100644 --- a/cmd/query/app/apiv3/http_gateway_test.go +++ b/cmd/query/app/apiv3/http_gateway_test.go @@ -4,17 +4,23 @@ package apiv3 import ( + "fmt" + "net/http" "net/http/httptest" "testing" "github.com/gorilla/mux" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.uber.org/zap" "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" "github.com/jaegertracing/jaeger/pkg/config/tlscfg" "github.com/jaegertracing/jaeger/pkg/jtracer" "github.com/jaegertracing/jaeger/pkg/tenancy" + "github.com/jaegertracing/jaeger/pkg/testutils" dependencyStoreMocks "github.com/jaegertracing/jaeger/storage/dependencystore/mocks" + "github.com/jaegertracing/jaeger/storage/spanstore" spanstoremocks "github.com/jaegertracing/jaeger/storage/spanstore/mocks" ) @@ -63,3 +69,47 @@ func TestHTTPGateway(t *testing.T) { t.Run("TestGRPCGatewayWithTenancy", TestGRPCGatewayWithTenancy) t.Run("TestGRPCGatewayTenancyRejection", TestGRPCGatewayTenancyRejection) } + +func TestHTTPGatewayTryHandleError(t *testing.T) { + gw := new(HTTPGateway) + assert.False(t, gw.tryHandleError(nil, nil, 0), "returns false if no error") + + w := httptest.NewRecorder() + assert.True(t, gw.tryHandleError(w, spanstore.ErrTraceNotFound, 0), "returns true if error") + assert.Equal(t, http.StatusNotFound, w.Code, "sets status code to 404") + + logger, log := testutils.NewLogger() + gw.Logger = logger + w = httptest.NewRecorder() + const e = "some err" + assert.True(t, gw.tryHandleError(w, fmt.Errorf(e), http.StatusInternalServerError)) + assert.Contains(t, log.String(), e, "logs error if status code is 500") + assert.Contains(t, string(w.Body.String()), e, "writes error message to body") +} + +func TestHTTPGatewayGetTraceErrors(t *testing.T) { + gw := new(HTTPGateway) + reader := &spanstoremocks.Reader{} + gw.QueryService = querysvc.NewQueryService(reader, + &dependencyStoreMocks.Reader{}, + querysvc.QueryServiceOptions{}, + ) + + r, err := http.NewRequest(http.MethodGet, "/api/v3/traces/xyz", nil) + require.NoError(t, err) + w := httptest.NewRecorder() + gw.getTrace(w, r) + assert.Contains(t, w.Body.String(), "malformed trace_id") + + const e = "storage_error" + reader. + On("GetTrace", matchContext, matchTraceID). + Return(nil, fmt.Errorf(e)).Once() + + // TODO this does not work because there is no matcher in the ctx for gorilla + r, err = http.NewRequest(http.MethodGet, "/api/v3/traces/123", nil) + require.NoError(t, err) + w = httptest.NewRecorder() + gw.getTrace(w, r) + assert.Contains(t, w.Body.String(), e) +} From 42e6fac876ab98bfcf370d6cf8cfcc7ff6b5e3d0 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Thu, 28 Dec 2023 11:44:56 -0500 Subject: [PATCH 09/15] tests Signed-off-by: Yuri Shkuro --- cmd/query/app/apiv3/grpc_gateway_test.go | 5 +- cmd/query/app/apiv3/http_gateway.go | 116 +++++++++----- cmd/query/app/apiv3/http_gateway_test.go | 191 ++++++++++++++++++++--- 3 files changed, 250 insertions(+), 62 deletions(-) diff --git a/cmd/query/app/apiv3/grpc_gateway_test.go b/cmd/query/app/apiv3/grpc_gateway_test.go index a9c290370d4..8867c2326c9 100644 --- a/cmd/query/app/apiv3/grpc_gateway_test.go +++ b/cmd/query/app/apiv3/grpc_gateway_test.go @@ -67,6 +67,7 @@ var useHTTPGateway = false type testGateway struct { reader *spanstoremocks.Reader url string + router *mux.Router } type gatewayRequest struct { @@ -296,8 +297,8 @@ func runGatewayFindTraces(t *testing.T, gw *testGateway, setupRequest func(*http q := url.Values{} q.Set("query.service_name", "foobar") - q.Set("query.start_time_min", time.Now().Format(time.RFC3339)) - q.Set("query.start_time_max", time.Now().Format(time.RFC3339)) + q.Set("query.start_time_min", time.Now().Format(time.RFC3339Nano)) + q.Set("query.start_time_max", time.Now().Format(time.RFC3339Nano)) body, statusCode := gw.execRequest(t, &gatewayRequest{ url: "/api/v3/traces?" + q.Encode(), diff --git a/cmd/query/app/apiv3/http_gateway.go b/cmd/query/app/apiv3/http_gateway.go index 15ad79b50ab..8261eac3e67 100644 --- a/cmd/query/app/apiv3/http_gateway.go +++ b/cmd/query/app/apiv3/http_gateway.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "strconv" "time" @@ -22,13 +23,21 @@ import ( "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" ) const ( - traceIDParam = "traceID" + paramTraceID = "trace_id" // get trace by ID + paramServiceName = "query.service_name" // find traces + paramOperationName = "query.operation_name" + paramTimeMin = "query.start_time_min" + paramTimeMax = "query.start_time_max" + paramNumTraces = "query.num_traces" + paramDurationMin = "query.duration_min" + paramDurationMax = "query.duration_max" - routeGetTrace = "/api/v3/traces/{traceID}" + routeGetTrace = "/api/v3/traces/{" + paramTraceID + "}" routeFindTraces = "/api/v3/traces" routeGetServices = "/api/v3/services" routeGetOperations = "/api/v3/operations" @@ -93,7 +102,24 @@ func (h *HTTPGateway) tryHandleError(w http.ResponseWriter, err error, statusCod return true } +// tryParamError is similar to tryHandleError but specifically for reporting malformed params. +func (h *HTTPGateway) tryParamError(w http.ResponseWriter, err error, paramName string) bool { + if err == nil { + return false + } + return h.tryHandleError(w, fmt.Errorf("malformed parameter %s: %w", paramName, err), http.StatusBadRequest) +} + func (h *HTTPGateway) returnSpans(spans []*model.Span, w http.ResponseWriter) { + // modelToOTLP does not easily return an error, so allow mocking it + h.returnSpansTestable(spans, w, modelToOTLP) +} + +func (h *HTTPGateway) returnSpansTestable( + spans []*model.Span, + w http.ResponseWriter, + modelToOTLP func(spans []*model.Span) ([]*tracev1.ResourceSpans, error), +) { resourceSpans, err := modelToOTLP(spans) if h.tryHandleError(w, err, http.StatusInternalServerError) { return @@ -124,9 +150,9 @@ func (h *HTTPGateway) marshalResponse(response proto.Message, w http.ResponseWri func (h *HTTPGateway) getTrace(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) - traceIDVar := vars[traceIDParam] + traceIDVar := vars[paramTraceID] traceID, err := model.TraceIDFromString(traceIDVar) - if err != nil && h.tryHandleError(w, fmt.Errorf("malformed trace_id: %w", err), http.StatusBadRequest) { + if h.tryParamError(w, err, paramTraceID) { return } trace, err := h.QueryService.GetTrace(r.Context(), traceID) @@ -137,61 +163,71 @@ func (h *HTTPGateway) getTrace(w http.ResponseWriter, r *http.Request) { } func (h *HTTPGateway) findTraces(w http.ResponseWriter, r *http.Request) { - query := r.URL.Query() + queryParams, shouldReturn := h.parseFindTracesQuery(r.URL.Query(), w) + if shouldReturn { + return + } + + traces, err := h.QueryService.FindTraces(r.Context(), queryParams) + // TODO how do we distinguish internal error from bad parameters for FindTrace? + if h.tryHandleError(w, err, http.StatusInternalServerError) { + return + } + var spans []*model.Span + for _, trace := range traces { + spans = append(spans, trace.Spans...) + } + h.returnSpans(spans, w) +} + +func (h *HTTPGateway) parseFindTracesQuery(q url.Values, w http.ResponseWriter) (*spanstore.TraceQueryParameters, bool) { queryParams := &spanstore.TraceQueryParameters{ - ServiceName: query.Get("query.service_name"), - OperationName: query.Get("query.operation_name"), + ServiceName: q.Get(paramServiceName), + OperationName: q.Get(paramOperationName), Tags: nil, // most curiously not supported by grpc-gateway } - if n := query.Get("query.num_traces"); n != "" { - numTraces, err := strconv.Atoi(n) - if h.tryHandleError(w, err, http.StatusBadRequest) { - return - } - queryParams.NumTraces = numTraces - } - timeMin := query.Get("query.start_time_min") - timeMax := query.Get("query.start_time_max") + + timeMin := q.Get(paramTimeMin) + timeMax := q.Get(paramTimeMax) if timeMin == "" || timeMax == "" { - err := fmt.Errorf("query.start_time_min and query.start_time_max are required") + err := fmt.Errorf("%s and %s are required", paramTimeMin, paramTimeMax) h.tryHandleError(w, err, http.StatusBadRequest) - return + return nil, true } timeMinParsed, err := time.Parse(time.RFC3339Nano, timeMin) - if h.tryHandleError(w, err, http.StatusBadRequest) { - return + if h.tryParamError(w, err, paramTimeMin) { + return nil, true } - queryParams.StartTimeMin = timeMinParsed timeMaxParsed, err := time.Parse(time.RFC3339Nano, timeMax) - if h.tryHandleError(w, err, http.StatusBadRequest) { - return + if h.tryParamError(w, err, paramTimeMax) { + return nil, true } + queryParams.StartTimeMin = timeMinParsed queryParams.StartTimeMax = timeMaxParsed - if d := query.Get("duration_min"); d != "" { + + if n := q.Get(paramNumTraces); n != "" { + numTraces, err := strconv.Atoi(n) + if h.tryParamError(w, err, paramNumTraces) { + return nil, true + } + queryParams.NumTraces = numTraces + } + + if d := q.Get(paramDurationMin); d != "" { dur, err := time.ParseDuration(d) - if h.tryHandleError(w, err, http.StatusBadRequest) { - return + if h.tryParamError(w, err, paramDurationMin) { + return nil, true } queryParams.DurationMin = dur } - if d := query.Get("duration_max"); d != "" { + if d := q.Get(paramDurationMax); d != "" { dur, err := time.ParseDuration(d) - if h.tryHandleError(w, err, http.StatusBadRequest) { - return + if h.tryParamError(w, err, paramDurationMax) { + return nil, true } queryParams.DurationMax = dur } - - traces, err := h.QueryService.FindTraces(r.Context(), queryParams) - // TODO how do we distinguish internal error from bad parameters for FindTrace? - if h.tryHandleError(w, err, http.StatusInternalServerError) { - return - } - var spans []*model.Span - for _, trace := range traces { - spans = append(spans, trace.Spans...) - } - h.returnSpans(spans, w) + return queryParams, false } func (h *HTTPGateway) getServices(w http.ResponseWriter, r *http.Request) { diff --git a/cmd/query/app/apiv3/http_gateway_test.go b/cmd/query/app/apiv3/http_gateway_test.go index 405dafa35a7..4e3573c7975 100644 --- a/cmd/query/app/apiv3/http_gateway_test.go +++ b/cmd/query/app/apiv3/http_gateway_test.go @@ -7,7 +7,9 @@ import ( "fmt" "net/http" "net/http/httptest" + "net/url" "testing" + "time" "github.com/gorilla/mux" "github.com/stretchr/testify/assert" @@ -15,19 +17,20 @@ import ( "go.uber.org/zap" "github.com/jaegertracing/jaeger/cmd/query/app/querysvc" + "github.com/jaegertracing/jaeger/model" "github.com/jaegertracing/jaeger/pkg/config/tlscfg" "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" ) -func setupHTTPGateway( +func setupHTTPGatewayNoServer( t *testing.T, basePath string, - serverTLS, clientTLS *tlscfg.Options, tenancyOptions tenancy.Options, ) *testGateway { gw := &testGateway{ @@ -46,13 +49,23 @@ func setupHTTPGateway( Tracer: jtracer.NoOp(), } - router := &mux.Router{} + gw.router = &mux.Router{} if basePath != "" && basePath != "/" { - router = router.PathPrefix(basePath).Subrouter() + gw.router = gw.router.PathPrefix(basePath).Subrouter() } - hgw.RegisterRoutes(router) + hgw.RegisterRoutes(gw.router) + return gw +} + +func setupHTTPGateway( + t *testing.T, + basePath string, + serverTLS, clientTLS *tlscfg.Options, + tenancyOptions tenancy.Options, +) *testGateway { + gw := setupHTTPGatewayNoServer(t, basePath, tenancyOptions) - httpServer := httptest.NewServer(router) + httpServer := httptest.NewServer(gw.router) t.Cleanup(func() { httpServer.Close() }) gw.url = httpServer.URL @@ -87,29 +100,167 @@ func TestHTTPGatewayTryHandleError(t *testing.T) { assert.Contains(t, string(w.Body.String()), e, "writes error message to body") } -func TestHTTPGatewayGetTraceErrors(t *testing.T) { - gw := new(HTTPGateway) - reader := &spanstoremocks.Reader{} - gw.QueryService = querysvc.NewQueryService(reader, - &dependencyStoreMocks.Reader{}, - querysvc.QueryServiceOptions{}, +func TestHTTPGatewayOTLPError(t *testing.T) { + w := httptest.NewRecorder() + gw := &HTTPGateway{ + Logger: zap.NewNop(), + } + const simErr = "simulated error" + gw.returnSpansTestable(nil, w, + func(spans []*model.Span) ([]*tracev1.ResourceSpans, error) { + return nil, fmt.Errorf(simErr) + }, ) + assert.Contains(t, w.Body.String(), simErr) +} + +func TestHTTPGatewayGetTraceErrors(t *testing.T) { + gw := setupHTTPGatewayNoServer(t, "", tenancy.Options{}) + // malformed trace id r, err := http.NewRequest(http.MethodGet, "/api/v3/traces/xyz", nil) require.NoError(t, err) w := httptest.NewRecorder() - gw.getTrace(w, r) - assert.Contains(t, w.Body.String(), "malformed trace_id") + gw.router.ServeHTTP(w, r) + assert.Contains(t, w.Body.String(), "malformed parameter trace_id") - const e = "storage_error" - reader. + // error from span reader + const simErr = "simulated error" + gw.reader. On("GetTrace", matchContext, matchTraceID). - Return(nil, fmt.Errorf(e)).Once() + Return(nil, fmt.Errorf(simErr)).Once() - // TODO this does not work because there is no matcher in the ctx for gorilla r, err = http.NewRequest(http.MethodGet, "/api/v3/traces/123", nil) require.NoError(t, err) w = httptest.NewRecorder() - gw.getTrace(w, r) - assert.Contains(t, w.Body.String(), e) + gw.router.ServeHTTP(w, r) + assert.Contains(t, w.Body.String(), simErr) +} + +func TestHTTPGatewayFindTracesErrors(t *testing.T) { + goodTimeV := time.Now().Truncate(time.Microsecond) + goodTime := goodTimeV.Format(time.RFC3339Nano) + timeRangeErr := fmt.Sprintf("%s and %s are required", paramTimeMin, paramTimeMax) + testCases := []struct { + name string + params map[string]string + expErr string + }{ + { + name: "no time range", + expErr: timeRangeErr, + }, + { + name: "no max time", + params: map[string]string{paramTimeMin: goodTime}, + expErr: timeRangeErr, + }, + { + name: "no min time", + params: map[string]string{paramTimeMax: goodTime}, + expErr: timeRangeErr, + }, + { + name: "bax min time", + params: map[string]string{paramTimeMin: "NaN", paramTimeMax: goodTime}, + expErr: paramTimeMin, + }, + { + name: "bax max time", + params: map[string]string{paramTimeMin: goodTime, paramTimeMax: "NaN"}, + expErr: paramTimeMax, + }, + { + name: "bad num_traces", + params: map[string]string{paramTimeMin: goodTime, paramTimeMax: goodTime, paramNumTraces: "NaN"}, + expErr: paramNumTraces, + }, + { + name: "bad min duration", + params: map[string]string{paramTimeMin: goodTime, paramTimeMax: goodTime, paramDurationMin: "NaN"}, + expErr: paramDurationMin, + }, + { + name: "bad max duration", + params: map[string]string{paramTimeMin: goodTime, paramTimeMax: goodTime, paramDurationMax: "NaN"}, + expErr: paramDurationMax, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + q := url.Values{} + for k, v := range tc.params { + q.Set(k, v) + } + r, err := http.NewRequest(http.MethodGet, "/api/v3/traces?"+q.Encode(), nil) + require.NoError(t, err) + w := httptest.NewRecorder() + + gw := setupHTTPGatewayNoServer(t, "", tenancy.Options{}) + gw.router.ServeHTTP(w, r) + assert.Contains(t, w.Body.String(), tc.expErr) + }) + } + t.Run("span reader error", func(t *testing.T) { + q := url.Values{} + q.Set(paramServiceName, "foo") + q.Set(paramOperationName, "bar") + q.Set(paramTimeMax, goodTime) + q.Set(paramTimeMin, goodTime) + q.Set(paramDurationMin, "1s") + q.Set(paramDurationMax, "2s") + q.Set(paramNumTraces, "10") + + const simErr = "simulated error" + r, err := http.NewRequest(http.MethodGet, "/api/v3/traces?"+q.Encode(), nil) + require.NoError(t, err) + w := httptest.NewRecorder() + + gw := setupHTTPGatewayNoServer(t, "", tenancy.Options{}) + gw.reader. + On("FindTraces", matchContext, &spanstore.TraceQueryParameters{ + ServiceName: "foo", + OperationName: "bar", + StartTimeMin: goodTimeV, + StartTimeMax: goodTimeV, + DurationMin: 1 * time.Second, + DurationMax: 2 * time.Second, + NumTraces: 10, + }). + Return(nil, fmt.Errorf(simErr)).Once() + + gw.router.ServeHTTP(w, r) + assert.Contains(t, w.Body.String(), simErr) + }) +} + +func TestHTTPGatewayGetServicesErrors(t *testing.T) { + gw := setupHTTPGatewayNoServer(t, "", tenancy.Options{}) + + const simErr = "simulated error" + gw.reader. + On("GetServices", matchContext). + Return(nil, fmt.Errorf(simErr)).Once() + + r, err := http.NewRequest(http.MethodGet, "/api/v3/services", nil) + require.NoError(t, err) + w := httptest.NewRecorder() + gw.router.ServeHTTP(w, r) + assert.Contains(t, w.Body.String(), simErr) +} + +func TestHTTPGatewayGetOperationsErrors(t *testing.T) { + gw := setupHTTPGatewayNoServer(t, "", tenancy.Options{}) + + qp := spanstore.OperationQueryParameters{ServiceName: "foo", SpanKind: "server"} + const simErr = "simulated error" + gw.reader. + On("GetOperations", matchContext, qp). + Return(nil, fmt.Errorf(simErr)).Once() + + r, err := http.NewRequest(http.MethodGet, "/api/v3/operations?service=foo&span_kind=server", nil) + require.NoError(t, err) + w := httptest.NewRecorder() + gw.router.ServeHTTP(w, r) + assert.Contains(t, w.Body.String(), simErr) } From 74a84f718db7ee70a21fcbfece9733482a199058 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Thu, 28 Dec 2023 11:54:42 -0500 Subject: [PATCH 10/15] fix time rounding Signed-off-by: Yuri Shkuro --- cmd/query/app/apiv3/http_gateway_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/query/app/apiv3/http_gateway_test.go b/cmd/query/app/apiv3/http_gateway_test.go index 4e3573c7975..91574dbfd41 100644 --- a/cmd/query/app/apiv3/http_gateway_test.go +++ b/cmd/query/app/apiv3/http_gateway_test.go @@ -138,7 +138,7 @@ func TestHTTPGatewayGetTraceErrors(t *testing.T) { } func TestHTTPGatewayFindTracesErrors(t *testing.T) { - goodTimeV := time.Now().Truncate(time.Microsecond) + goodTimeV := time.Now().Truncate(time.Nanosecond) // truncated to reset monotonic clock goodTime := goodTimeV.Format(time.RFC3339Nano) timeRangeErr := fmt.Sprintf("%s and %s are required", paramTimeMin, paramTimeMax) testCases := []struct { From b413b72db822dddf5babe7e442319355562c4d39 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Thu, 28 Dec 2023 12:09:08 -0500 Subject: [PATCH 11/15] tests Signed-off-by: Yuri Shkuro --- cmd/query/app/apiv3/grpc_gateway_test.go | 17 ++++----- cmd/query/app/apiv3/http_gateway_test.go | 44 ++++++++++++++---------- 2 files changed, 32 insertions(+), 29 deletions(-) diff --git a/cmd/query/app/apiv3/grpc_gateway_test.go b/cmd/query/app/apiv3/grpc_gateway_test.go index 8867c2326c9..8dedeb69e0b 100644 --- a/cmd/query/app/apiv3/grpc_gateway_test.go +++ b/cmd/query/app/apiv3/grpc_gateway_test.go @@ -22,19 +22,16 @@ import ( "io" "net" "net/http" - "net/url" "os" "path" "path/filepath" "strings" "testing" - "time" gogojsonpb "github.com/gogo/protobuf/jsonpb" gogoproto "github.com/gogo/protobuf/proto" "github.com/gorilla/mux" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "go.uber.org/zap" "google.golang.org/grpc" @@ -291,15 +288,15 @@ func runGatewayGetTrace(t *testing.T, gw *testGateway, setupRequest func(*http.R func runGatewayFindTraces(t *testing.T, gw *testGateway, setupRequest func(*http.Request)) { trace, traceID := makeTestTrace() + q, qp := mockFindQueries() + if !useHTTPGateway { + // grpc-gateway forces inbound timestamps into UTC, so simulate this here + qp.StartTimeMin = qp.StartTimeMin.UTC() + qp.StartTimeMax = qp.StartTimeMax.UTC() + } gw.reader. - On("FindTraces", matchContext, mock.AnythingOfType("*spanstore.TraceQueryParameters")). + On("FindTraces", matchContext, qp). Return([]*model.Trace{trace}, nil).Once() - - q := url.Values{} - q.Set("query.service_name", "foobar") - q.Set("query.start_time_min", time.Now().Format(time.RFC3339Nano)) - q.Set("query.start_time_max", time.Now().Format(time.RFC3339Nano)) - body, statusCode := gw.execRequest(t, &gatewayRequest{ url: "/api/v3/traces?" + q.Encode(), setupRequest: setupRequest, diff --git a/cmd/query/app/apiv3/http_gateway_test.go b/cmd/query/app/apiv3/http_gateway_test.go index 91574dbfd41..0a69b50dce9 100644 --- a/cmd/query/app/apiv3/http_gateway_test.go +++ b/cmd/query/app/apiv3/http_gateway_test.go @@ -137,8 +137,30 @@ func TestHTTPGatewayGetTraceErrors(t *testing.T) { assert.Contains(t, w.Body.String(), simErr) } +func mockFindQueries() (url.Values, *spanstore.TraceQueryParameters) { + goodTime := time.Now().Truncate(time.Nanosecond) // truncated to reset monotonic clock + q := url.Values{} + q.Set(paramServiceName, "foo") + q.Set(paramOperationName, "bar") + q.Set(paramTimeMin, goodTime.Add(-time.Second).Format(time.RFC3339Nano)) + q.Set(paramTimeMax, goodTime.Format(time.RFC3339Nano)) + q.Set(paramDurationMin, "1s") + q.Set(paramDurationMax, "2s") + q.Set(paramNumTraces, "10") + + return q, &spanstore.TraceQueryParameters{ + ServiceName: "foo", + OperationName: "bar", + StartTimeMin: goodTime.Add(-time.Second), + StartTimeMax: goodTime, + DurationMin: 1 * time.Second, + DurationMax: 2 * time.Second, + NumTraces: 10, + } +} + func TestHTTPGatewayFindTracesErrors(t *testing.T) { - goodTimeV := time.Now().Truncate(time.Nanosecond) // truncated to reset monotonic clock + goodTimeV := time.Now() goodTime := goodTimeV.Format(time.RFC3339Nano) timeRangeErr := fmt.Sprintf("%s and %s are required", paramTimeMin, paramTimeMax) testCases := []struct { @@ -202,15 +224,7 @@ func TestHTTPGatewayFindTracesErrors(t *testing.T) { }) } t.Run("span reader error", func(t *testing.T) { - q := url.Values{} - q.Set(paramServiceName, "foo") - q.Set(paramOperationName, "bar") - q.Set(paramTimeMax, goodTime) - q.Set(paramTimeMin, goodTime) - q.Set(paramDurationMin, "1s") - q.Set(paramDurationMax, "2s") - q.Set(paramNumTraces, "10") - + q, qp := mockFindQueries() const simErr = "simulated error" r, err := http.NewRequest(http.MethodGet, "/api/v3/traces?"+q.Encode(), nil) require.NoError(t, err) @@ -218,15 +232,7 @@ func TestHTTPGatewayFindTracesErrors(t *testing.T) { gw := setupHTTPGatewayNoServer(t, "", tenancy.Options{}) gw.reader. - On("FindTraces", matchContext, &spanstore.TraceQueryParameters{ - ServiceName: "foo", - OperationName: "bar", - StartTimeMin: goodTimeV, - StartTimeMax: goodTimeV, - DurationMin: 1 * time.Second, - DurationMax: 2 * time.Second, - NumTraces: 10, - }). + On("FindTraces", matchContext, qp). Return(nil, fmt.Errorf(simErr)).Once() gw.router.ServeHTTP(w, r) From 579209c004963f275ec6c431cb2450b2cb1bc00d Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Fri, 29 Dec 2023 16:46:36 -0500 Subject: [PATCH 12/15] fix tests Signed-off-by: Yuri Shkuro --- cmd/query/app/grpc_handler_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/query/app/grpc_handler_test.go b/cmd/query/app/grpc_handler_test.go index 1708cbf5649..d65ee6932fa 100644 --- a/cmd/query/app/grpc_handler_test.go +++ b/cmd/query/app/grpc_handler_test.go @@ -973,7 +973,7 @@ func TestSearchTenancyGRPC(t *testing.T) { require.NoError(t, err, "could not initiate GetTraceRequest") spanResChunk, err := res.Recv() - assertGRPCError(t, err, codes.PermissionDenied, "missing tenant header") + assertGRPCError(t, err, codes.Unauthenticated, "missing tenant header") assert.Nil(t, spanResChunk) // Next try with tenancy @@ -1003,7 +1003,7 @@ func TestServicesTenancyGRPC(t *testing.T) { // First try without tenancy header _, err := client.GetServices(context.Background(), &api_v2.GetServicesRequest{}) - assertGRPCError(t, err, codes.PermissionDenied, "missing tenant header") + assertGRPCError(t, err, codes.Unauthenticated, "missing tenant header") // Next try with tenancy res, err := client.GetServices(withOutgoingMetadata(t, context.Background(), tm.Header, "acme"), &api_v2.GetServicesRequest{}) @@ -1033,7 +1033,7 @@ func TestSearchTenancyGRPCExplicitList(t *testing.T) { { name: "no header", wantErr: true, - failureCode: codes.PermissionDenied, + failureCode: codes.Unauthenticated, failureMessage: "missing tenant header", }, { @@ -1041,7 +1041,7 @@ func TestSearchTenancyGRPCExplicitList(t *testing.T) { tenancyHeader: "not-the-correct-header", tenant: "mercury", wantErr: true, - failureCode: codes.PermissionDenied, + failureCode: codes.Unauthenticated, failureMessage: "missing tenant header", }, { From ab70193e31b0a76a3ec5ce0ed0fd157c6e8944d9 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Fri, 29 Dec 2023 16:41:07 -0500 Subject: [PATCH 13/15] Separate test report collection from the main target Signed-off-by: Yuri Shkuro --- .github/workflows/ci-unit-tests-go-tip.yml | 8 ++++++++ .github/workflows/ci-unit-tests.yml | 8 ++++++++ Makefile | 2 +- 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci-unit-tests-go-tip.yml b/.github/workflows/ci-unit-tests-go-tip.yml index 168b38115b5..75354453446 100644 --- a/.github/workflows/ci-unit-tests-go-tip.yml +++ b/.github/workflows/ci-unit-tests-go-tip.yml @@ -23,9 +23,17 @@ jobs: - name: Install Go Tip uses: ./.github/actions/setup-go-tip + - name: Install test deps + # even though the same target runs from test-ci, running it separately makes for cleaner log in GH workflow + run: make install-test-tools + - name: Run unit tests run: make test-ci + - name: Prepare unit tests report + if: always() + run: make test-report + - name: Publish Unit Test Summary 📑 uses: EnricoMi/publish-unit-test-result-action@v2 if: always() diff --git a/.github/workflows/ci-unit-tests.yml b/.github/workflows/ci-unit-tests.yml index 0febf9e8cc2..129577bf732 100644 --- a/.github/workflows/ci-unit-tests.yml +++ b/.github/workflows/ci-unit-tests.yml @@ -32,9 +32,17 @@ jobs: go-version: 1.21.x cache-dependency-path: ./go.sum + - name: Install test deps + # even though the same target runs from test-ci, running it separately makes for cleaner log in GH workflow + run: make install-test-tools + - name: Run unit tests run: make test-ci + - name: Prepare unit tests report + if: always() + run: make test-report + - name: Publish Unit Test Summary 📑 uses: EnricoMi/publish-unit-test-result-action@v2 if: always() diff --git a/Makefile b/Makefile index a2736d67d05..986a5ce06b1 100644 --- a/Makefile +++ b/Makefile @@ -453,7 +453,7 @@ install-ci: install-test-tools install-build-tools .PHONY: test-ci test-ci: GOTEST := $(GOTEST_QUIET) -json -test-ci: install-test-tools build-examples cover test-report +test-ci: install-test-tools build-examples cover .PHONY: test-report test-report: From 156d28b94feba43010b27225f72f77fec4794120 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Fri, 29 Dec 2023 17:09:36 -0500 Subject: [PATCH 14/15] disable json test report Signed-off-by: Yuri Shkuro --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 986a5ce06b1..cc65954b08c 100644 --- a/Makefile +++ b/Makefile @@ -452,7 +452,7 @@ install-tools: install-test-tools install-build-tools install-ci: install-test-tools install-build-tools .PHONY: test-ci -test-ci: GOTEST := $(GOTEST_QUIET) -json +#test-ci: GOTEST := $(GOTEST_QUIET) -json test-ci: install-test-tools build-examples cover .PHONY: test-report From 3abcaa2697d265959f1f88024e31a69140586cfa Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Fri, 29 Dec 2023 17:40:09 -0500 Subject: [PATCH 15/15] Fix tests Signed-off-by: Yuri Shkuro --- Makefile | 2 +- cmd/query/app/apiv3/grpc_gateway_test.go | 5 ----- cmd/query/app/apiv3/http_gateway_test.go | 14 +++++++++----- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/Makefile b/Makefile index cc65954b08c..986a5ce06b1 100644 --- a/Makefile +++ b/Makefile @@ -452,7 +452,7 @@ install-tools: install-test-tools install-build-tools install-ci: install-test-tools install-build-tools .PHONY: test-ci -#test-ci: GOTEST := $(GOTEST_QUIET) -json +test-ci: GOTEST := $(GOTEST_QUIET) -json test-ci: install-test-tools build-examples cover .PHONY: test-report diff --git a/cmd/query/app/apiv3/grpc_gateway_test.go b/cmd/query/app/apiv3/grpc_gateway_test.go index 8dedeb69e0b..381308b71ad 100644 --- a/cmd/query/app/apiv3/grpc_gateway_test.go +++ b/cmd/query/app/apiv3/grpc_gateway_test.go @@ -289,11 +289,6 @@ func runGatewayGetTrace(t *testing.T, gw *testGateway, setupRequest func(*http.R func runGatewayFindTraces(t *testing.T, gw *testGateway, setupRequest func(*http.Request)) { trace, traceID := makeTestTrace() q, qp := mockFindQueries() - if !useHTTPGateway { - // grpc-gateway forces inbound timestamps into UTC, so simulate this here - qp.StartTimeMin = qp.StartTimeMin.UTC() - qp.StartTimeMax = qp.StartTimeMax.UTC() - } gw.reader. On("FindTraces", matchContext, qp). Return([]*model.Trace{trace}, nil).Once() diff --git a/cmd/query/app/apiv3/http_gateway_test.go b/cmd/query/app/apiv3/http_gateway_test.go index 0a69b50dce9..86300ff5b50 100644 --- a/cmd/query/app/apiv3/http_gateway_test.go +++ b/cmd/query/app/apiv3/http_gateway_test.go @@ -138,12 +138,16 @@ func TestHTTPGatewayGetTraceErrors(t *testing.T) { } func mockFindQueries() (url.Values, *spanstore.TraceQueryParameters) { - goodTime := time.Now().Truncate(time.Nanosecond) // truncated to reset monotonic clock + // mock performs deep comparison of the timestamps and can fail + // if they are different in the timezone or the monotonic clocks. + // To void that we truncate monotonic clock and force UTC timezone. + time1 := time.Now().UTC().Truncate(time.Nanosecond) + time2 := time1.Add(-time.Second).UTC().Truncate(time.Nanosecond) q := url.Values{} q.Set(paramServiceName, "foo") q.Set(paramOperationName, "bar") - q.Set(paramTimeMin, goodTime.Add(-time.Second).Format(time.RFC3339Nano)) - q.Set(paramTimeMax, goodTime.Format(time.RFC3339Nano)) + q.Set(paramTimeMin, time1.Format(time.RFC3339Nano)) + q.Set(paramTimeMax, time2.Format(time.RFC3339Nano)) q.Set(paramDurationMin, "1s") q.Set(paramDurationMax, "2s") q.Set(paramNumTraces, "10") @@ -151,8 +155,8 @@ func mockFindQueries() (url.Values, *spanstore.TraceQueryParameters) { return q, &spanstore.TraceQueryParameters{ ServiceName: "foo", OperationName: "bar", - StartTimeMin: goodTime.Add(-time.Second), - StartTimeMax: goodTime, + StartTimeMin: time1, + StartTimeMax: time2, DurationMin: 1 * time.Second, DurationMax: 2 * time.Second, NumTraces: 10,