Update grpc & http endpoint to query operations by service & spanKind (…

…jaegertracing#1943) * Update grpc & http endpoint to query operations by service & spanKind - legacy http endpoint will stay unchanged: /services/%s/operations Signed-off-by: Jun Guo <[email protected]> * Better format Signed-off-by: Jun Guo <[email protected]> * Fix unit test Signed-off-by: Jun Guo <[email protected]> * Update changelog and address feedback Signed-off-by: Jun Guo <[email protected]> * Make grpc query server backward-compatible Signed-off-by: Jun Guo <[email protected]>
ilyamor · Nov 26, 2019 · 9cd7a7e · 9cd7a7e
1 parent c57e4b7
commit 9cd7a7e
Show file tree

Hide file tree

Showing 14 changed files with 646 additions and 122 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,52 @@ Changes by Version
 
 ##### Breaking Changes
 
+* Changes to the endpoints that returns a list of operations ([#1943](https://github.com/jaegertracing/jaeger/pull/1943), [@guo0693](https://github.com/guo0693))
+    * Endpoint changes:
+        * Both Http & gRPC servers now take new optional parameter `spanKind` in addition to `service`. When spanKind
+         is absent or empty, operations from all kinds of spans will be returned.
+        * Instead of returning a list of string, both Http & gRPC servers return a list of operation struct. Please 
+        update your client code to process the new response. Example response:
+            ```
+            curl 'http://localhost:6686/api/operations?service=UserService&spanKind=server' | jq
+            {
+                "data": [{
+                    "name": "UserService::getExtendedUser",
+                    "spanKind": "server"
+                },
+                {
+                    "name": "UserService::getUserProfile",
+                    "spanKind": "server"
+                }],
+                "total": 2,
+                "limit": 0,
+                "offset": 0,
+                "errors": null
+            }
+            ```
+        * The legacy http endpoint stay untouched:
+            ```
+            /services/{%s}/operations
+            ```    
+    * Storage plugin changes:
+        * Memory updated to support spanKind on write & read, no migration is required.
+        * [Badger](https://github.com/jaegertracing/jaeger/issues/1922) & [ElasticSearch](https://github.com/jaegertracing/jaeger/issues/1923) 
+        to be implemented:  
+        For now `spanKind` will be set as empty string during read & write, only `name` will be valid operation name.
+        * Cassandra updated to support spanKind on write & read ([#1937](https://github.com/jaegertracing/jaeger/pull/1937), [@guo0693](https://github.com/guo0693)):  
+            If you don't run the migration script, nothing will break, the system will used the old table 
+            `operation_names` and set empty `spanKind` in the response.  
+            Steps to get the updated functionality:
+            1.  You will need to run below command on the host you can use `cqlsh` to connect the the cassandra contact
+             point
+                ```
+                KEYSPACE=test_keyspace TIMEOUT=1000 CQL_CMD='cqlsh host 9042 -u test_user -p test_password' bash
+                ./plugin/storage/cassandra/schema/migration/v002tov003.sh
+                ```
+                The script will create new table `operation_names_v2` and migrate data from the old table.  
+                `spanKind` column will be empty for those data.  
+                At the end, it will ask you whether you want to drop the old table or not.
+            2. Restart ingester & query services so that they begin to use the new table
 ##### New Features
 
 ##### Bug fixes, Minor Improvements

diff --git a/cmd/query/app/grpc_handler.go b/cmd/query/app/grpc_handler.go
@@ -128,15 +128,31 @@ func (g *GRPCHandler) GetServices(ctx context.Context, r *api_v2.GetServicesRequ
 }
 
 // GetOperations is the GRPC handler to fetch operations.
-func (g *GRPCHandler) GetOperations(ctx context.Context, r *api_v2.GetOperationsRequest) (*api_v2.GetOperationsResponse, error) {
-	service := r.Service
-	operations, err := g.queryService.GetOperations(ctx, service)
+func (g *GRPCHandler) GetOperations(
+	ctx context.Context,
+	r *api_v2.GetOperationsRequest,
+) (*api_v2.GetOperationsResponse, error) {
+	operations, err := g.queryService.GetOperations(ctx, spanstore.OperationQueryParameters{
+		ServiceName: r.Service,
+		SpanKind:    r.SpanKind,
+	})
 	if err != nil {
 		g.logger.Error("Error fetching operations", zap.Error(err))
 		return nil, err
 	}
 
-	return &api_v2.GetOperationsResponse{Operations: operations}, nil
+	result := make([]*api_v2.Operation, len(operations))
+	for i, operation := range operations {
+		result[i] = &api_v2.Operation{
+			Name:     operation.Name,
+			SpanKind: operation.SpanKind,
+		}
+	}
+	return &api_v2.GetOperationsResponse{
+		Operations: result,
+		// TODO: remove OperationNames after all clients are updated
+		OperationNames: getUniqueOperationNames(operations),
+	}, nil
 }
 
 // GetDependencies is the GRPC handler to fetch dependencies.

diff --git a/cmd/query/app/grpc_handler_test.go b/cmd/query/app/grpc_handler_test.go
@@ -415,7 +415,12 @@ func TestGetServicesFailureGRPC(t *testing.T) {
 func TestGetOperationsSuccessGRPC(t *testing.T) {
 	withServerAndClient(t, func(server *grpcServer, client *grpcClient) {
 
-		expectedOperations := []spanstore.Operation{{Name: ""}, {Name: "get", SpanKind: "server"}}
+		expectedOperations := []spanstore.Operation{
+			{Name: ""},
+			{Name: "get", SpanKind: "server"},
+			{Name: "get", SpanKind: "client"},
+		}
+		expectedNames := []string{"", "get"}
 		server.spanReader.On("GetOperations",
 			mock.AnythingOfType("*context.valueCtx"),
 			spanstore.OperationQueryParameters{ServiceName: "abc/trifle"},
@@ -427,8 +432,10 @@ func TestGetOperationsSuccessGRPC(t *testing.T) {
 		assert.NoError(t, err)
 		assert.Equal(t, len(expectedOperations), len(res.Operations))
 		for i, actualOp := range res.Operations {
-			assert.Equal(t, expectedOperations[i].Name, actualOp)
+			assert.Equal(t, expectedOperations[i].Name, actualOp.Name)
+			assert.Equal(t, expectedOperations[i].SpanKind, actualOp.SpanKind)
 		}
+		assert.ElementsMatch(t, expectedNames, res.OperationNames)
 	})
 }
 

diff --git a/cmd/query/app/http_handler.go b/cmd/query/app/http_handler.go
@@ -157,13 +157,21 @@ func (aH *APIHandler) getOperationsLegacy(w http.ResponseWriter, r *http.Request
 	vars := mux.Vars(r)
 	// given how getOperationsLegacy is bound to URL route, serviceParam cannot be empty
 	service, _ := url.QueryUnescape(vars[serviceParam])
-	operations, err := aH.queryService.GetOperations(r.Context(), service)
+	// for backwards compatibility, we will retrieve operations with all span kind
+	operations, err := aH.queryService.GetOperations(r.Context(),
+		spanstore.OperationQueryParameters{
+			ServiceName: service,
+			// include all kinds
+			SpanKind: "",
+		})
+
 	if aH.handleError(w, err, http.StatusInternalServerError) {
 		return
 	}
+	operationNames := getUniqueOperationNames(operations)
 	structuredRes := structuredResponse{
-		Data:  operations,
-		Total: len(operations),
+		Data:  operationNames,
+		Total: len(operationNames),
 	}
 	aH.writeJSON(w, r, &structuredRes)
 }
@@ -175,12 +183,24 @@ func (aH *APIHandler) getOperations(w http.ResponseWriter, r *http.Request) {
 			return
 		}
 	}
-	operations, err := aH.queryService.GetOperations(r.Context(), service)
+	spanKind := r.FormValue(spanKindParam)
+	operations, err := aH.queryService.GetOperations(
+		r.Context(),
+		spanstore.OperationQueryParameters{ServiceName: service, SpanKind: spanKind},
+	)
+
 	if aH.handleError(w, err, http.StatusInternalServerError) {
 		return
 	}
+	data := make([]ui.Operation, len(operations))
+	for i, operation := range operations {
+		data[i] = ui.Operation{
+			Name:     operation.Name,
+			SpanKind: operation.SpanKind,
+		}
+	}
 	structuredRes := structuredResponse{
-		Data:  operations,
+		Data:  data,
 		Total: len(operations),
 	}
 	aH.writeJSON(w, r, &structuredRes)

diff --git a/cmd/query/app/http_handler_test.go b/cmd/query/app/http_handler_test.go
@@ -484,16 +484,27 @@ func TestGetOperationsSuccess(t *testing.T) {
 	server, readMock, _ := initializeTestServer()
 	defer server.Close()
 	expectedOperations := []spanstore.Operation{{Name: ""}, {Name: "get", SpanKind: "server"}}
-	readMock.On("GetOperations", mock.AnythingOfType("*context.valueCtx"),
-		spanstore.OperationQueryParameters{ServiceName: "abc/trifle"}).Return(expectedOperations, nil).Once()
+	readMock.On(
+		"GetOperations",
+		mock.AnythingOfType("*context.valueCtx"),
+		spanstore.OperationQueryParameters{ServiceName: "abc/trifle"},
+	).Return(expectedOperations, nil).Once()
+
+	var response struct {
+		Operations []ui.Operation    `json:"data"`
+		Total      int               `json:"total"`
+		Limit      int               `json:"limit"`
+		Offset     int               `json:"offset"`
+		Errors     []structuredError `json:"errors"`
+	}
 
-	var response structuredResponse
 	err := getJSON(server.URL+"/api/operations?service=abc%2Ftrifle", &response)
 	assert.NoError(t, err)
-	for i, s := range response.Data.([]interface{}) {
-		assert.Equal(t, expectedOperations[i].Name, s.(string))
+	assert.Equal(t, len(expectedOperations), len(response.Operations))
+	for i, op := range response.Operations {
+		assert.Equal(t, expectedOperations[i].Name, op.Name)
+		assert.Equal(t, expectedOperations[i].SpanKind, op.SpanKind)
 	}
-
 }
 
 func TestGetOperationsNoServiceName(t *testing.T) {
@@ -522,20 +533,21 @@ func TestGetOperationsLegacySuccess(t *testing.T) {
 	server, readMock, _ := initializeTestServer()
 	defer server.Close()
 	expectedOperationNames := []string{"", "get"}
-	expectedOperations := []spanstore.Operation{{Name: ""}, {Name: "get", SpanKind: "server"}}
+	expectedOperations := []spanstore.Operation{
+		{Name: ""},
+		{Name: "get", SpanKind: "server"},
+		{Name: "get", SpanKind: "client"}}
+
 	readMock.On(
 		"GetOperations",
 		mock.AnythingOfType("*context.valueCtx"),
 		mock.AnythingOfType("spanstore.OperationQueryParameters")).Return(expectedOperations, nil).Once()
 
 	var response structuredResponse
 	err := getJSON(server.URL+"/api/services/abc%2Ftrifle/operations", &response)
+
 	assert.NoError(t, err)
-	actualOperations := make([]string, len(expectedOperations))
-	for i, s := range response.Data.([]interface{}) {
-		actualOperations[i] = s.(string)
-	}
-	assert.Equal(t, expectedOperationNames, actualOperations)
+	assert.ElementsMatch(t, expectedOperationNames, response.Data.([]interface{}))
 }
 
 func TestGetOperationsLegacyStorageFailure(t *testing.T) {

diff --git a/cmd/query/app/query_parser.go b/cmd/query/app/query_parser.go
@@ -40,6 +40,7 @@ const (
 	minDurationParam = "minDuration"
 	maxDurationParam = "maxDuration"
 	serviceParam     = "service"
+	spanKindParam    = "spanKind"
 	endTimeParam     = "end"
 	prettyPrintParam = "prettyPrint"
 )

diff --git a/cmd/query/app/querysvc/query_service.go b/cmd/query/app/querysvc/query_service.go
@@ -79,21 +79,11 @@ func (qs QueryService) GetServices(ctx context.Context) ([]string, error) {
 }
 
 // GetOperations is the queryService implementation of spanstore.Reader.GetOperations
-func (qs QueryService) GetOperations(ctx context.Context, service string) ([]string, error) {
-	operations, err := qs.spanReader.GetOperations(ctx, spanstore.OperationQueryParameters{
-		ServiceName: service,
-	})
-
-	// TODO: remove below and simply return the result from query service
-	if err != nil {
-		return nil, err
-	}
-
-	names := make([]string, len(operations))
-	for i, operation := range operations {
-		names[i] = operation.Name
-	}
-	return names, err
+func (qs QueryService) GetOperations(
+	ctx context.Context,
+	query spanstore.OperationQueryParameters,
+) ([]spanstore.Operation, error) {
+	return qs.spanReader.GetOperations(ctx, query)
 }
 
 // FindTraces is the queryService implementation of spanstore.Reader.FindTraces

diff --git a/cmd/query/app/querysvc/query_service_test.go b/cmd/query/app/querysvc/query_service_test.go
@@ -147,17 +147,19 @@ func TestGetServices(t *testing.T) {
 // Test QueryService.GetOperations() for success.
 func TestGetOperations(t *testing.T) {
 	qs, readMock, _ := initializeTestService()
-	expectedOperationNames := []string{"", "get"}
-	expectedOperations := []spanstore.Operation{{Name: ""}, {Name: "get", SpanKind: "server"}}
-	readMock.On("GetOperations",
+	expectedOperations := []spanstore.Operation{{Name: "", SpanKind: ""}, {Name: "get", SpanKind: ""}}
+	operationQuery := spanstore.OperationQueryParameters{ServiceName: "abc/trifle"}
+	readMock.On(
+		"GetOperations",
 		mock.AnythingOfType("*context.valueCtx"),
-		mock.AnythingOfType("spanstore.OperationQueryParameters")).Return(expectedOperations, nil).Once()
+		operationQuery,
+	).Return(expectedOperations, nil).Once()
 
 	type contextKey string
 	ctx := context.Background()
-	actualOperations, err := qs.GetOperations(context.WithValue(ctx, contextKey("foo"), "bar"), "abc/trifle")
+	actualOperations, err := qs.GetOperations(context.WithValue(ctx, contextKey("foo"), "bar"), operationQuery)
 	assert.NoError(t, err)
-	assert.Equal(t, expectedOperationNames, actualOperations)
+	assert.Equal(t, expectedOperations, actualOperations)
 }
 
 // Test QueryService.FindTraces() for success.

diff --git a/cmd/query/app/util.go b/cmd/query/app/util.go
@@ -0,0 +1,30 @@
+// Copyright (c) 2019 The Jaeger Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package app
+
+import "github.com/jaegertracing/jaeger/storage/spanstore"
+
+func getUniqueOperationNames(operations []spanstore.Operation) []string {
+	// only return unique operation names
+	set := make(map[string]struct{})
+	for _, operation := range operations {
+		set[operation.Name] = struct{}{}
+	}
+	var operationNames []string
+	for operation := range set {
+		operationNames = append(operationNames, operation)
+	}
+	return operationNames
+}
diff --git a/cmd/query/app/util_test.go b/cmd/query/app/util_test.go
@@ -0,0 +1,38 @@
+// Copyright (c) 2019 The Jaeger Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package app
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/jaegertracing/jaeger/storage/spanstore"
+)
+
+func Test_getUniqueOperationNames(t *testing.T) {
+	assert.Equal(t, []string(nil), getUniqueOperationNames(nil))
+
+	operations := []spanstore.Operation{
+		{Name: "operation1", SpanKind: "server"},
+		{Name: "operation1", SpanKind: "client"},
+		{Name: "operation2"},
+	}
+
+	expNames := []string{"operation1", "operation2"}
+	names := getUniqueOperationNames(operations)
+	assert.Equal(t, 2, len(names))
+	assert.ElementsMatch(t, expNames, names)
+}
diff --git a/model/json/model.go b/model/json/model.go
@@ -109,3 +109,9 @@ type DependencyLink struct {
 	Child     string `json:"child"`
 	CallCount uint64 `json:"callCount"`
 }
+
+// Operation defines the data in the operation response when query operation by service and span kind
+type Operation struct {
+	Name     string `json:"name"`
+	SpanKind string `json:"spanKind"`
+}
diff --git a/model/proto/api_v2/query.proto b/model/proto/api_v2/query.proto
@@ -109,10 +109,17 @@ message GetServicesResponse {
 
 message GetOperationsRequest {
   string service = 1;
+  string span_kind = 2;
+}
+
+message Operation {
+    string name = 1;
+    string span_kind = 2;
 }
 
 message GetOperationsResponse {
-  repeated string operations = 1;
+  repeated string operationNames = 1; //deprecated
+  repeated Operation operations = 2;
 }
 
 message GetDependenciesRequest {